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
