Thread (329 messages) 329 messages, 12 authors, 2018-03-14

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

From: Jeff King <hidden>
Date: 2018-03-12 23:52:06

On Mon, Mar 12, 2018 at 04:37:47PM -0700, Jonathan Nieder wrote:
Jeff King wrote:
quoted
We could even give it an environment variable, which would allow
something like:

  tar xf maybe-evil.git.tar
  cd maybe-evil
  export GIT_TRUST_REPO=false
  git log
[...]
As an internal implementation detail, this is so obviously fragile
that it wouldn't give me any feeling of security. ;-)  So it should be
strictly an improvement.

As a public-facing feature, I suspect it's a bad idea for exactly that
reason.
So that pretty much kills off the GIT_TRUST_REPO idea, I guess.
FWIW for pager specifically I am going for a whitelisting approach:
new commands would have to explicitly set ALLOW_PAGER if they want to
respect pager config.  That doesn't guarantee people think about it
again as things evolve but it should at least help with getting the
right setting for new plumbing.
I suspect we'd be about as well off with the "don't trust the repo"
internal flag. Touching the ALLOW_PAGER setup code is about as likely to
set off red flags for the developers (or reviewers) as code that checks
the "trust" flag.

Forcing a whitelist on ALLOW_PAGER _is_ more likely to catch people
adding new commands. But I don't think we actually want to add more
commands to the "safe to run in a malicious repo" list. It's already a
slightly sketchy concept. This is really all about upload-pack and its
existing promises.

But ALLOW_PAGER would _just_ fix the pager issue. When we inevitably
find another problem spot, it won't help us there. But a global "trust"
flag might.

I dunno. I guess I'm OK with either approach, but it seems like the
global trust flag has more room to grow.

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help