{"thread":{"id":"45629","subject":"problem with backslash in directory name","startedAt":"2017-04-07T06:19:22Z","lastAt":"2017-04-17T03:09:56Z","messageCount":11,"participants":["Joachim Durchholz","Jeff King","Brandon Williams","Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"316324","messageId":"9e3af7d6-a2c1-2673-53cc-d4c5105d9051@durchholz.org","threadId":"45629","inReplyTo":null,"subject":"problem with backslash in directory name","fromName":"Joachim Durchholz","fromEmail":"jo@durchholz.org","sentAt":"2017-04-07T06:12:49Z","receivedAt":"2017-04-07T06:19:22Z","isPatch":false,"sender":{"key":"jo@durchholz.org","avatar":null},"body":"Hi all,\n\nI'm having a problem with submodules that reside in directories that \n(unwisely) contain a backslash in their name.\n\n\nTranscript:\n\n### Arrange\n\n$ git init main\nInitialized empty Git repository in /tmp/test/main/.git/\n\n$ git init sub\\\\with\\\\backslash\nInitialized empty Git repository in /tmp/test/sub\\with\\backslash/.git/\n# This looks okay: the shell interpreted \\\\ as \\,\n# so we get sub\\with\\backslash\n\n# Create a log entry in sub\\with\\backslash\n# (it can't be added as a submodule otherwise)\n# ((actually I think it's a misfeature, my current use case would be\n# easier if git didn't insist on having a log in submodules))\n$ touch sub\\\\with\\\\backslash/empty.file\n$ git -C sub\\\\with\\\\backslash add empty.file\n$ git -C sub\\\\with\\\\backslash commit -m \"Added empty.file\"\n[master (root-commit) a27a485] Added empty.file\n  1 file changed, 0 insertions(+), 0 deletions(-)\n  create mode 100644 empty.file\n\n### Act/Assert\n\n$ git -C main submodule add ../sub\\\\with\\\\backslash\nfatal: repository '/tmp/test/sub\\witackslash' does not exist\nfatal: clone of '/tmp/test/sub\\witackslash' into submodule path \n'sub\\with\\backslash' failed\n# The first \"fatal:\" line talks about \"witackslash\"\n# Um... \"ackslash\"? Now that's a nice nickname for a CVE :-)\n\n# Okay, let's see what's actually in that message\n$ git -C main submodule add ../sub\\\\with\\\\backslash 2>&1 | xxd\n00000000: 6661 7461 6c3a 2072 6570 6f73 6974 6f72  fatal: repositor\n00000010: 7920 272f 746d 702f 7465 7374 2f73 7562  y '/tmp/test/sub\n00000020: 5c77 6974 6808 6163 6b73 6c61 7368 2720  \\with.ackslash'\n00000030: 646f 6573 206e 6f74 2065 7869 7374 0a66  does not exist.f\n00000040: 6174 616c 3a20 636c 6f6e 6520 6f66 2027  atal: clone of '\n00000050: 2f74 6d70 2f74 6573 742f 7375 625c 7769  /tmp/test/sub\\wi\n00000060: 7468 0861 636b 736c 6173 6827 2069 6e74  th.ackslash' int\n00000070: 6f20 7375 626d 6f64 756c 6520 7061 7468  o submodule path\n00000080: 2027 7375 625c 7769 7468 5c62 6163 6b73   'sub\\with\\backs\n00000090: 6c61 7368 2720 6661 696c 6564 0a         lash' failed.\n\n# Yeah, there's a 0x08 at offset 0x25.\n# It's pretty strange that it is eliding the w following the \\b,\n# not the h preceding it.\n\n\nSo... something inside \"git submodule add\" is replacing the \\b with a \nbackspace control code.\n\n\nNext I tried something nasty:\n\n$ mv sub\\\\with\\\\backslash 'sub: $(bc)'\ngit -C main submodule add '../sub: $(bc)'\nCloning into ' $(bc)'...\ndone.\n\nWhatever that \"something\" is, it is not doing shell expansion, otherwise \nit would have started an interactive calculator session.\nPhew :-)\nI'm still a bit uneasy because I don't know what other escape sequences \nmight get interpreted, and what their effects are.\n"},{"id":"316325","messageId":"20170407063057.yjbqavu4uiipslpc@sigill.intra.peff.net","threadId":"45629","inReplyTo":"9e3af7d6-a2c1-2673-53cc-d4c5105d9051@durchholz.org","subject":"Re: problem with backslash in directory name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-07T06:30:57Z","receivedAt":"2017-04-07T06:31:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 07, 2017 at 08:12:49AM +0200, Joachim Durchholz wrote:\n\n> So... something inside \"git submodule add\" is replacing the \\b with a\n> backspace control code.\n> [...]\n> Whatever that \"something\" is, it is not doing shell expansion, otherwise it\n> would have started an interactive calculator session.\n\nProbably it's \"read\" which does backslash expansion, but nothing else.\nJust grepping git-submodule.sh, some of the \"read\" calls should probably\nbe \"read -r\" (I also don't know how some of those loops would cope with\na submodule name that needed quoting).\n\n-Peff\n"},{"id":"316328","messageId":"06bad45d-ad0b-aafe-47e8-172e44ab69cc@durchholz.org","threadId":"45629","inReplyTo":"20170407063057.yjbqavu4uiipslpc@sigill.intra.peff.net","subject":"Re: problem with backslash in directory name","fromName":"Joachim Durchholz","fromEmail":"jo@durchholz.org","sentAt":"2017-04-07T08:24:14Z","receivedAt":"2017-04-07T08:24:23Z","isPatch":false,"sender":{"key":"jo@durchholz.org","avatar":null},"body":"Am 07.04.2017 um 08:30 schrieb Jeff King:\n> Probably it's \"read\" which does backslash expansion, but nothing else.\n> Just grepping git-submodule.sh, some of the \"read\" calls should probably\n> be \"read -r\"\n\nhttp://wiki.bash-hackers.org/commands/builtin/read has this to say:\n\n> Essentially all you need to know about -r is to ALWAYS use it. The\n > exact behavior you get without -r is completely useless even for weird\n > purposes. It basically allows the escaping of input which matches\n > something in IFS, and also escapes line continuations. It's explained\n > pretty well in the POSIX read[1] spec.\n\n[1] \nhttp://pubs.opengroup.org/onlinepubs/9699919799/utilities/read.html#tag_20_109\n\n(That's the kind of stuff that makes me shy of using shell scripts - \nalways yet another surprise in waiting...)\n"},{"id":"316329","messageId":"0a51432b-b9c9-32c5-c064-5fc9aaddebc7@durchholz.org","threadId":"45629","inReplyTo":"20170407063057.yjbqavu4uiipslpc@sigill.intra.peff.net","subject":"Re: problem with backslash in directory name","fromName":"Joachim Durchholz","fromEmail":"jo@durchholz.org","sentAt":"2017-04-07T08:40:53Z","receivedAt":"2017-04-07T08:41:05Z","isPatch":false,"sender":{"key":"jo@durchholz.org","avatar":null},"body":"Am 07.04.2017 um 08:30 schrieb Jeff King:\n> I also don't know how some of those loops would cope with\n> a submodule name that needed quoting).\n\n\"git submodule add\" worked fine with most of the following names:\n\n     \"sub\"\n     # potentially confusing the shell\n     \"sub with blanks\",\n     \"sub with\\nnewline\",\n     \"sub with'single quote\",\n     \"sub with\\\"double quote\",\n     \"sub with\\\\backslash\",\n     \"sub with\\bbackspace\",\n     \"sub with\\thorizontal tab\",\n     # potentially confusing git's configuration format\n     \"sub with #\",\n     \"sub with =\"\n\n(That's Python 3 literals in case somebody is wondering. I'm using \nPython to unit test a shell script, just so I can catch this sort of \nstuff...)\n"},{"id":"316364","messageId":"20170407165314.GB142670@google.com","threadId":"45629","inReplyTo":"20170407063057.yjbqavu4uiipslpc@sigill.intra.peff.net","subject":"Re: problem with backslash in directory name","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-07T16:53:14Z","receivedAt":"2017-04-07T16:53:22Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/07, Jeff King wrote:\n> On Fri, Apr 07, 2017 at 08:12:49AM +0200, Joachim Durchholz wrote:\n> \n> > So... something inside \"git submodule add\" is replacing the \\b with a\n> > backspace control code.\n> > [...]\n> > Whatever that \"something\" is, it is not doing shell expansion, otherwise it\n> > would have started an interactive calculator session.\n> \n> Probably it's \"read\" which does backslash expansion, but nothing else.\n> Just grepping git-submodule.sh, some of the \"read\" calls should probably\n> be \"read -r\" (I also don't know how some of those loops would cope with\n> a submodule name that needed quoting).\n\nSo I blindly converted all \"read\" calls to \"read -r\" and tested against\nthe case Joachim ran into and it seems to solve the issues.  All test\nstill pass too (though that may not mean too much).\n\n-- \nBrandon Williams\n"},{"id":"316365","messageId":"CAGZ79ka7PEWy2iA8V9s11n2kyD=vHyS2y1K3SJzDq-5HsY7how@mail.gmail.com","threadId":"45629","inReplyTo":"20170407165314.GB142670@google.com","subject":"Re: problem with backslash in directory name","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-07T16:55:05Z","receivedAt":"2017-04-07T16:55:11Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> Probably it's \"read\" which does backslash expansion, but nothing else.\n>> Just grepping git-submodule.sh, some of the \"read\" calls should probably\n>> be \"read -r\" (I also don't know how some of those loops would cope with\n>> a submodule name that needed quoting).\n>\n> So I blindly converted all \"read\" calls to \"read -r\" and tested against\n> the case Joachim ran into and it seems to solve the issues.  All test\n> still pass too (though that may not mean too much).\n\n... because we may not have tests with weird names in submodule path.\nThanks for the conversion!\n\nStefan\n"},{"id":"316366","messageId":"20170407172306.172673-1-bmwill@google.com","threadId":"45629","inReplyTo":"CAGZ79ka7PEWy2iA8V9s11n2kyD=vHyS2y1K3SJzDq-5HsY7how@mail.gmail.com","subject":"[PATCH] submodule: prevent backslash expantion in submodule names","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-07T17:23:06Z","receivedAt":"2017-04-07T17:23:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When attempting to add a submodule with backslashes in its name 'git\nsubmodule' fails in a funny way.  We can see that some of the\nbackslashes are expanded resulting in a bogus path:\n\ngit -C main submodule add ../sub\\\\with\\\\backslash\nfatal: repository '/tmp/test/sub\\witackslash' does not exist\nfatal: clone of '/tmp/test/sub\\witackslash' into submodule path\n\nTo solve this, convert calls to 'read' to 'read -r' in git-submodule.sh\nin order to prevent backslash expantion in submodule names.\n\nReported-by: Joachim Durchholz <jo@durchholz.org>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n git-submodule.sh           | 14 +++++++-------\n t/t7400-submodule-basic.sh | 14 ++++++++++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 6ec35e5fc..c0d0e9a4c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -332,7 +332,7 @@ cmd_foreach()\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" ||\n \t\techo \"#unmatched\" $?\n \t} |\n-\twhile read mode sha1 stage sm_path\n+\twhile read -r mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tif test -e \"$sm_path\"/.git\n@@ -441,7 +441,7 @@ cmd_deinit()\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n \t\techo \"#unmatched\" $?\n \t} |\n-\twhile read mode sha1 stage sm_path\n+\twhile read -r mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n@@ -605,7 +605,7 @@ cmd_update()\n \t\t\"$@\" || echo \"#unmatched\" $?\n \t} | {\n \terr=\n-\twhile read mode sha1 stage just_cloned sm_path\n+\twhile read -r mode sha1 stage just_cloned sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \n@@ -847,7 +847,7 @@ cmd_summary() {\n \t# Get modified modules cared by user\n \tmodules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- \"$@\" |\n \t\tsane_egrep '^:([0-7]* )?160000' |\n-\t\twhile read mod_src mod_dst sha1_src sha1_dst status sm_path\n+\t\twhile read -r mod_src mod_dst sha1_src sha1_dst status sm_path\n \t\tdo\n \t\t\t# Always show modules deleted or type-changed (blob<->module)\n \t\t\tif test \"$status\" = D || test \"$status\" = T\n@@ -873,7 +873,7 @@ cmd_summary() {\n \tgit $diff_cmd $cached --ignore-submodules=dirty --raw $head -- $modules |\n \tsane_egrep '^:([0-7]* )?160000' |\n \tcut -c2- |\n-\twhile read mod_src mod_dst sha1_src sha1_dst status name\n+\twhile read -r mod_src mod_dst sha1_src sha1_dst status name\n \tdo\n \t\tif test -z \"$cached\" &&\n \t\t\ttest $sha1_dst = 0000000000000000000000000000000000000000\n@@ -1020,7 +1020,7 @@ cmd_status()\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n \t\techo \"#unmatched\" $?\n \t} |\n-\twhile read mode sha1 stage sm_path\n+\twhile read -r mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n@@ -1100,7 +1100,7 @@ cmd_sync()\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n \t\techo \"#unmatched\" $?\n \t} |\n-\twhile read mode sha1 stage sm_path\n+\twhile read -r mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex cf77a3a35..c2706fe47 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -273,6 +273,20 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \ttest_cmp empty untracked\n '\n \n+test_expect_success 'submodule add with \\\\ in path' '\n+\ttest_when_finished \"rm -rf parent sub\\\\with\\\\backslash\" &&\n+\n+\t# Initialize a repo with a backslash in its name\n+\tgit init sub\\\\with\\\\backslash &&\n+\ttouch sub\\\\with\\\\backslash/empty.file &&\n+\tgit -C sub\\\\with\\\\backslash add empty.file &&\n+\tgit -C sub\\\\with\\\\backslash commit -m \"Added empty.file\" &&\n+\n+\t# Add that repository as a submodule\n+\tgit init parent &&\n+\tgit -C parent submodule add ../sub\\\\with\\\\backslash\n+'\n+\n test_expect_success 'submodule add in subdirectory' '\n \techo \"refs/heads/master\" >expect &&\n \t>empty &&\n-- \n2.12.2.715.g7642488e1d-goog\n\n"},{"id":"316367","messageId":"ab8959a3-0a26-b60e-fb11-ee743b740780@durchholz.org","threadId":"45629","inReplyTo":"20170407172306.172673-1-bmwill@google.com","subject":"Re: [PATCH] submodule: prevent backslash expantion in submodule names","fromName":"Joachim Durchholz","fromEmail":"jo@durchholz.org","sentAt":"2017-04-07T17:35:16Z","receivedAt":"2017-04-07T17:35:23Z","isPatch":true,"sender":{"key":"jo@durchholz.org","avatar":null},"body":"Thanks!\n"},{"id":"316396","messageId":"20170408105901.2osi2zadboqxhf34@sigill.intra.peff.net","threadId":"45629","inReplyTo":"20170407172306.172673-1-bmwill@google.com","subject":"Re: [PATCH] submodule: prevent backslash expantion in submodule names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-08T10:59:01Z","receivedAt":"2017-04-08T10:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 07, 2017 at 10:23:06AM -0700, Brandon Williams wrote:\n\n> When attempting to add a submodule with backslashes in its name 'git\n> submodule' fails in a funny way.  We can see that some of the\n> backslashes are expanded resulting in a bogus path:\n> \n> git -C main submodule add ../sub\\\\with\\\\backslash\n> fatal: repository '/tmp/test/sub\\witackslash' does not exist\n> fatal: clone of '/tmp/test/sub\\witackslash' into submodule path\n> \n> To solve this, convert calls to 'read' to 'read -r' in git-submodule.sh\n> in order to prevent backslash expantion in submodule names.\n\nThis looks sane overall, without digging into the individual read calls.\n\nThe reason I mentioned escaping earlier is I wondered what would happen\nwhen the submodule starts with a double-quote, or has a newline in the\nname. Git's normal quoting would include backslash escape sequences, and\nI wondered if we might be relying on any of these \"read\" calls to\ninterpret them. But I don't think so, for two reasons.\n\nOne, because that quoting also puts double-quotes around the name. So\nplain \"read\" would not be sufficient to de-quote for us anyway.\n\nAnd two, because these are being fed from \"submodule--helper\", which\ndoes not seem to quote in the first place.\n\nSo I think your patch is fine there. But it does raise a few concerns.\nIt looks like git-submodule does not cope well with exotic filenames:\n\n  $ git submodule add /some/repo \"$(printf 'sub with\\nnewline')\"\n  Cloning into '/home/peff/tmp/sub with\n  newline'...\n  done.\n  error: invalid key (newline): submodule.sub with\n  newline.url\n  error: invalid key (newline): submodule.sub with\n  newline.path\n  Failed to register submodule 'sub with\n  newline'\n\nI'm not too worried about that. It's a nonsense request, and our config\nformat has no syntactic mechanism to represent that key. So tough luck.\nBut what I am more worried about is:\n\n  $ git submodule--helper list\n  160000 576053ed5ad378490974fabe97e4bd59633d2d1e 0\tsub with\n  newline\n\nThat's obviously nonsense that git-submodule.sh is going to choke on.\nBut what happens when the filename is:\n\n  foo\\n16000 <sha1> 0\\t../../escaped\n\nor something. Can a malicious repository provoke git-submodule.sh to\nlook at or modify files outside the repository?\n\n-Peff\n"},{"id":"316420","messageId":"d8ef00f6-4ec0-dfcb-dc49-71f27538eb2f@durchholz.org","threadId":"45629","inReplyTo":"20170408105901.2osi2zadboqxhf34@sigill.intra.peff.net","subject":"Re: [PATCH] submodule: prevent backslash expantion in submodule names","fromName":"Joachim Durchholz","fromEmail":"jo@durchholz.org","sentAt":"2017-04-08T20:32:41Z","receivedAt":"2017-04-08T20:32:49Z","isPatch":true,"sender":{"key":"jo@durchholz.org","avatar":null},"body":"Am 08.04.2017 um 12:59 schrieb Jeff King:\n> The reason I mentioned escaping earlier is I wondered what would happen\n> when the submodule starts with a double-quote, or has a newline in the\n> name.\n\nI have tested newlines within the name, these work fine.\n\nI also tested double and single quotes within the name, but not at \nbeginning or end.\n\n> So I think your patch is fine there. But it does raise a few concerns.\n> It looks like git-submodule does not cope well with exotic filenames:\n>\n>   $ git submodule add /some/repo \"$(printf 'sub with\\nnewline')\"\n>   Cloning into '/home/peff/tmp/sub with\n>   newline'...\n>   done.\n>   error: invalid key (newline): submodule.sub with\n>   newline.url\n>   error: invalid key (newline): submodule.sub with\n>   newline.path\n>   Failed to register submodule 'sub with\n>   newline'\n\nStrange. I'm running essentially the same kind of request, and things \nwork fine.\nMight be due to me using Python3 instead of bash, or maybe due to \ndifferent versions of git.\n\nIf anybody is interested, I can publish my test code on github, it was \nscheduled to land there anyway.\n\n> I'm not too worried about that.  It's a nonsense request, and our config\n> format has no syntactic mechanism to represent that key.\n\nOh. I've been thinking that the quoted format is exactly for that kind \nof stuff.\nThough it might be prone to eol conversion if a submodule name contains \ncrlf sequences.\n\nAlso, funny behavour. Experience has taught me that funny behaviour, if \nit isn't exploitable today, may combine with some new funny behaviour in \na future version of the same software. So I'm worried even with that.\n\nThis is starting to look like a can of worms to me... one way to \"close \nthe lid\" would be if git\n* defined what's a valid submodule name,\n* rejected invalid submodule names, and\n* documented validity rules in the git-submodule docs.\nYMMV, just my 2 cents :-)\n\nRegards,\nJo\n"},{"id":"316981","messageId":"xmqqr30rk9wl.fsf@gitster.mtv.corp.google.com","threadId":"45629","inReplyTo":"20170408105901.2osi2zadboqxhf34@sigill.intra.peff.net","subject":"Re: [PATCH] submodule: prevent backslash expantion in submodule names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-17T03:09:14Z","receivedAt":"2017-04-17T03:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The reason I mentioned escaping earlier is I wondered what would happen\n> when the submodule starts with a double-quote, or has a newline in the\n> name. Git's normal quoting would include backslash escape sequences, and\n> I wondered if we might be relying on any of these \"read\" calls to\n> interpret them. But I don't think so, for two reasons.\n>\n> One, because that quoting also puts double-quotes around the name. So\n> plain \"read\" would not be sufficient to de-quote for us anyway.\n\nCorrect. These are c-quoting and \"read\" does not know what to do\nwith them.\n\n> And two, because these are being fed from \"submodule--helper\", which\n> does not seem to quote in the first place.\n\nWhich probably is a bug we can fix safely, as submodule--helper is\nmerely an implementation detail of our toolset, not something the\nend users' scripts can rely on.\n"}]}