Re: [PATCH] submodule: prevent backslash expantion in submodule names

From: Jeff King <peff@peff.net>
Date: Apr 8, 2017, 10:59 UTC
Message-ID: <20170408105901.2osi2zadboqxhf34@sigill.intra.peff.net>
In-Reply-To: <20170407172306.172673-1-bmwill@google.com>

On Fri, Apr 07, 2017 at 10:23:06AM -0700, Brandon Williams wrote:

Show 10 quoted lines

> When attempting to add a submodule with backslashes in its name 'git
> submodule' fails in a funny way.  We can see that some of the
> backslashes are expanded resulting in a bogus path:
> 
> git -C main submodule add ../sub\\with\\backslash
> fatal: repository '/tmp/test/sub\witackslash' does not exist
> fatal: clone of '/tmp/test/sub\witackslash' into submodule path
> 
> To solve this, convert calls to 'read' to 'read -r' in git-submodule.sh
> in order to prevent backslash expantion in submodule names.

This looks sane overall, without digging into the individual read calls.

The reason I mentioned escaping earlier is I wondered what would happen when the submodule starts with a double-quote, or has a newline in the name. Git's normal quoting would include backslash escape sequences, and I wondered if we might be relying on any of these "read" calls to interpret them. But I don't think so, for two reasons.

One, because that quoting also puts double-quotes around the name. So plain "read" would not be sufficient to de-quote for us anyway.

And two, because these are being fed from "submodule--helper", which does not seem to quote in the first place.

So I think your patch is fine there. But it does raise a few concerns. It looks like git-submodule does not cope well with exotic filenames:

  $ git submodule add /some/repo "$(printf 'sub with\nnewline')"
  Cloning into '/home/peff/tmp/sub with
  newline'...
  done.
  error: invalid key (newline): submodule.sub with
  newline.url
  error: invalid key (newline): submodule.sub with
  newline.path
  Failed to register submodule 'sub with
  newline'

I'm not too worried about that. It's a nonsense request, and our config format has no syntactic mechanism to represent that key. So tough luck. But what I am more worried about is:

  $ git submodule--helper list
  160000 576053ed5ad378490974fabe97e4bd59633d2d1e 0	sub with
  newline

That's obviously nonsense that git-submodule.sh is going to choke on. But what happens when the filename is:

  foo\n16000 <sha1> 0\t../../escaped

or something. Can a malicious repository provoke git-submodule.sh to look at or modify files outside the repository?

-Peff