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

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

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