Re: [PATCH] submodule: prevent backslash expantion in submodule names
Jeff Kingwrites: > 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. Correct. These are c-quoting and "read" does not know what to do with them. > And two, because these are being fed from "submodule--helper", which > does not seem to quote in the first place. Which probably is a bug we can fix safely, as submodule--helper is merely an implementation detail of our toolset, not something the end users' scripts can rely on.
Re: [PATCH] submodule: prevent backslash expantion in submodule names
Am 08.04.2017 um 12:59 schrieb Jeff King: 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. I have tested newlines within the name, these work fine. I also tested double and single quotes within the name, but not at beginning or end. 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' Strange. I'm running essentially the same kind of request, and things work fine. Might be due to me using Python3 instead of bash, or maybe due to different versions of git. If anybody is interested, I can publish my test code on github, it was scheduled to land there anyway. I'm not too worried about that. It's a nonsense request, and our config format has no syntactic mechanism to represent that key. Oh. I've been thinking that the quoted format is exactly for that kind of stuff. Though it might be prone to eol conversion if a submodule name contains crlf sequences. Also, funny behavour. Experience has taught me that funny behaviour, if it isn't exploitable today, may combine with some new funny behaviour in a future version of the same software. So I'm worried even with that. This is starting to look like a can of worms to me... one way to "close the lid" would be if git * defined what's a valid submodule name, * rejected invalid submodule names, and * documented validity rules in the git-submodule docs. YMMV, just my 2 cents :-) Regards, Jo
Re: [PATCH] submodule: prevent backslash expantion in submodule names
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 16 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 0\t../../escaped or something. Can a malicious repository provoke git-submodule.sh to look at or modify files outside the repository? -Peff
Re: [PATCH] submodule: prevent backslash expantion in submodule names
Thanks!