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

From: Jeff King <peff@peff.net>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org, sbeller@google.com, jo@durchholz.org
Subject: Re: [PATCH] submodule: prevent backslash expantion in submodule names
Date: Sat, 8 Apr 2017 06:59:01 -0400	[thread overview]
Message-ID: <20170408105901.2osi2zadboqxhf34@sigill.intra.peff.net> (raw)
In-Reply-To: <20170407172306.172673-1-bmwill@google.com>

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

> 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