On Mon, Mar 12, 2018 at 03:43:55PM -0700, Jonathan Nieder wrote:
> Hi,
>
> Jeff King wrote:
> > On Thu, Feb 22, 2018 at 01:26:34PM -0800, Jonathan Nieder wrote:
>
> >> Keep in mind that git upload-archive (a read-only command, just like
> >> git upload-pack) also already has the same issues.
> >
> > Yuck. I don't think we've ever made a historical promise about that. But
> > then, I don't think the promise about upload-pack has ever really been
> > documented, except in mailing list discussions.
>
> Sorry to revive this old side-thread. Good news: for a dashed command
> like git-upload-archive, the pager selection code only runs for
> commands with RUN_SETUP or RUN_SETUP_GENTLY:
>
> if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
> !(p->option & DELAY_PAGER_CONFIG))
> use_pager = check_pager_config(p->cmd);
>
> None of upload-pack, receive-pack,git-serve, or upload-archive set
> those flags, so we (narrowly) escape trouble here.
Right, I saw that earlier. But I actually think that is stale from the
days when it wasn't safe to call check_pager_config() too early. So I
could very well see somebody removing it and causing a spooky
vulnerability at a distance.
> Later today I should be able to send a cleanup to make the behavior
> more obvious.
Thanks. I'm still on the fence over the whole builtin concept, but
certainly a "don't ever turn on a pager" flag seems like a reasonable
thing to have.
An alternative approach is some kind of global for "don't trust the
local repo" flag. That could be respected from very low-level code
(e.g., where we read and/or respect the pager command, but also in other
places like hooks, other config that runs arbitrary commands, etc). And
then upload-pack would set that to "do not trust", and other programs
would default to "trust".
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
without worrying about pager.log config, etc. My two concerns with this
approach would be:
1. We have to manually annotate any "dangerous" code to act more
safely when it sees the flag. Which means it's highly likely to
a spot, or to add a new feature which doesn't respect it. And
suddenly that's a security hole. So I'm concerned it may create a
false sense of security and actually make things worse.
2. As a global, I'm not sure how it would interact with multi-repo
processes like submodules. In theory it ought to go into the
repository struct, but it would often need to be set globally
before we've even discovered the repo.
That might be fine, though. It's really more about context than
about a specific repo (so you may say "don't trust this repo", and
that extends to any submodules you happen to access, too).
I dunno. I think (2) is probably OK, but (1) really gives me pause.
-Peff
