{"thread":{"id":"42655","subject":"[PATCH] git-sh-setup.sh: fix missing double quotes variables","startedAt":"2016-06-18T19:37:31Z","lastAt":"2016-06-19T21:18:18Z","messageCount":11,"participants":["LE Manh Cuong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289581","messageId":"20160618193710.32265-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":null,"subject":"[PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-18T19:37:10Z","receivedAt":"2016-06-18T19:37:31Z","isPatch":true,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"Leaving shell variables un-quotes can lead to security vulnerabilities. In:\n\n : ${x=.}\n\n`$x` is always expanded, cause `glob+split` on its result. There're some\nglobs is too expensive to expand, like:\n\n x='/*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/*' sh -c ':\n ${x=.}'\n\nRun it and our machine will hang/crash (especially in Linux).\n\n`LESS`, `LV` and `GIT_OBJECT_DIRECTORY` variables in `git-sh-setup` are\nvulnerable with this case.\n\nFix this vulnerability by quoting those variables.\n\nSigned-off-by: LE Manh Cuong \n---\n git-sh-setup.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex c48139a..85db5f1 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -160,8 +160,8 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: ${LESS=-FRX}\n-\t: ${LV=-c}\n+\t: \"${LESS=-FRX}\"\n+\t: \"${LV=-c}\"\n \texport LESS LV\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n@@ -344,7 +344,7 @@ git_dir_init () {\n \t\techo >&2 \"Unable to determine absolute path of git directory\"\n \t\texit 1\n \t}\n-\t: ${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\n+\t: \"${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\"\n }\n \n if test -z \"$NONGIT_OK\"\n-- \n2.9.0\n\n"},{"id":"289582","messageId":"20160618201220.1407-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":"20160618193710.32265-1-cuong.manhle.vn@gmail.com","subject":"","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-18T20:12:19Z","receivedAt":"2016-06-18T20:12:45Z","isPatch":false,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"Sorry, the example in commit message is wrong, due to editor auto wrapping line.\n\n"},{"id":"289583","messageId":"20160618202603.1962-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":"20160618193710.32265-1-cuong.manhle.vn@gmail.com","subject":"[PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-18T20:26:03Z","receivedAt":"2016-06-18T20:26:17Z","isPatch":true,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"Leaving shell variables un-quotes can lead to security vulnerabilities. In:\n\n : ${x=.}\n\n`$x` is always expanded, cause `glob+split` on its result. There're some\nglobs is too expensive to expand, like:\n\n x='/*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/*' sh -c ': ${x=.}'\n\nRun it and our machine will hang/crash (especially in Linux).\n\n`LESS`, `LV` and `GIT_OBJECT_DIRECTORY` variables in `git-sh-setup` are\nvulnerable with this case.\n\nFix this vulnerability by quoting those variables.\n\nSigned-off-by: LE Manh Cuong \n---\n git-sh-setup.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex c48139a..85db5f1 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -160,8 +160,8 @@ git_pager() {\n \telse\n \t\tGIT_PAGER=cat\n \tfi\n-\t: ${LESS=-FRX}\n-\t: ${LV=-c}\n+\t: \"${LESS=-FRX}\"\n+\t: \"${LV=-c}\"\n \texport LESS LV\n \n \teval \"$GIT_PAGER\" '\"$@\"'\n@@ -344,7 +344,7 @@ git_dir_init () {\n \t\techo >&2 \"Unable to determine absolute path of git directory\"\n \t\texit 1\n \t}\n-\t: ${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\n+\t: \"${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\"\n }\n \n if test -z \"$NONGIT_OK\"\n-- \n2.9.0\n\n"},{"id":"289596","messageId":"xmqqshwax8ah.fsf@gitster.mtv.corp.google.com","threadId":"42655","inReplyTo":"20160618193710.32265-1-cuong.manhle.vn@gmail.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T01:43:34Z","receivedAt":"2016-06-19T01:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"LE Manh Cuong writes:\n\n> Leaving shell variables un-quotes can lead to security vulnerabilities. In:\n>\n> : ${x=.}\n>\n> `$x` is always expanded, cause `glob+split` on its result. There're some\n> globs is too expensive to expand, like:\n>\n> x='/*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/*' sh -c ':\n> ${x=.}'\n>\n> Run it and our machine will hang/crash (especially in Linux).\n>\n> `LESS`, `LV` and `GIT_OBJECT_DIRECTORY` variables in `git-sh-setup` are\n> vulnerable with this case.\n>\n> Fix this vulnerability by quoting those variables.\n>\n> Signed-off-by: LE Manh Cuong \n> ---\n\nThat is \"interesting\".\n\nGiven that LESS, LV and GIT_OBJECT_DIRECTORY are expected to be free\nof any \"expensive to expand\" strings, I am not sure if this actually\nmatters, though. And more importantly, these are what the end users\nare expected to set to whatever sensible values for them.\n\nYou would not be lying if you said that Git lets people who want to\ndo strange things shoot their feet off; I do not think that hardly\ndeserves to be called \"vulnerability\", though.\n\nHaving said all that, I do not mind preventing people from shooting\ntheir foot off, and the change in this patch certainly would not\nhurt.\n\nThanks.\n\n> git-sh-setup.sh | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index c48139a..85db5f1 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -160,8 +160,8 @@ git_pager() {\n> \telse\n> \t\tGIT_PAGER=cat\n> \tfi\n> -\t: ${LESS=-FRX}\n> -\t: ${LV=-c}\n> +\t: \"${LESS=-FRX}\"\n> +\t: \"${LV=-c}\"\n> \texport LESS LV\n> \n> \teval \"$GIT_PAGER\" '\"$@\"'\n> @@ -344,7 +344,7 @@ git_dir_init () {\n> \t\techo >&2 \"Unable to determine absolute path of git directory\"\n> \t\texit 1\n> \t}\n> -\t: ${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\n> +\t: \"${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\"\n> }\n> \n> if test -z \"$NONGIT_OK\"\n"},{"id":"289599","messageId":"20160619024554.2983-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":"xmqqshwax8ah.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-19T02:45:54Z","receivedAt":"2016-06-19T02:46:17Z","isPatch":true,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"> Given that LESS, LV and GIT_OBJECT_DIRECTORY are expected to be free\n> of any \"expensive to expand\" strings, I am not sure if this actually\n> matters, though. And more importantly, these are what the end users\n> are expected to set to whatever sensible values for them.\n\nThe problem is the end users want a \"string\". In shell, it means you want\nscalar context instead of list context, which is where the vulnerability\noccured.\n\nUsing `$var` is actually `glob(split($var))`. While using\n`\"$var\"` means the shell interpreted $var content as string. That's also\nwhat you do with `test`, `[...]`, redirection `>\"$file\"` (which is mentioned\nin Git conding contention).\n\nThat's a mistake usage to introduce that \"bug\" to the end user.\n(I invite you to read the excelent question/answer about this problem\nat http://unix.stackexchange.com/q/171346/38906)\n\n> You would not be lying if you said that Git lets people who want to\n> do strange things shoot their feet off; I do not think that hardly\n> deserves to be called \"vulnerability\", though.\n>\n> Having said all that, I do not mind preventing people from shooting\n> their foot off, and the change in this patch certainly would not\n> hurt.\n\nIt's not only people shooting their foot, but also from malicious user.\nGiven that `curl url | sudo sh/bash` is often found in many instructions,\nan end user may not be noticed about the environment variable injection\nfrom their side.\n\nIMHO, it's better if git can protect the end users in this situation.\n"},{"id":"289602","messageId":"xmqqinx5yijm.fsf@gitster.mtv.corp.google.com","threadId":"42655","inReplyTo":"20160619024554.2983-1-cuong.manhle.vn@gmail.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T03:16:45Z","receivedAt":"2016-06-19T03:16:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"LE Manh Cuong writes:\n\n> It's not only people shooting their foot, but also from malicious user.\n> Given that `curl url | sudo sh/bash` is often found in many instructions,\n> an end user may not be noticed about the environment variable injection\n> from their side.\n>\n> IMHO, it's better if git can protect the end users in this situation.\n\nHuh? For those who run `curl url | sudo sh`, I do not think the\nincoming script setting and exporting LV to an arbitrary value and\nruning Git is not the top thing they need worry about.\n\nWhile I think enclosing the string in dq is an improvement (as I\nsaid already), I still do think your use of the v-word is making a\nmountain out of an anthill.\n"},{"id":"289621","messageId":"20160619113154.3228-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":"xmqqinx5yijm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-19T11:31:54Z","receivedAt":"2016-06-19T11:32:07Z","isPatch":true,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"> While I think enclosing the string in dq is an improvement (as I\n> said already), I still do think your use of the v-word is making a\n> mountain out of an anthill.\n\nWell, sorry if that word is annoyed.\n\nSo, is this patch fine or I must re-phrase the title?\n"},{"id":"289632","messageId":"xmqqvb15vz3y.fsf@gitster.mtv.corp.google.com","threadId":"42655","inReplyTo":"xmqqinx5yijm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T17:59:29Z","receivedAt":"2016-06-19T18:00:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> LE Manh Cuong writes:\n>\n>> It's not only people shooting their foot, but also from malicious user.\n>> Given that `curl url | sudo sh/bash` is often found in many instructions,\n>> an end user may not be noticed about the environment variable injection\n>> from their side.\n>>\n>> IMHO, it's better if git can protect the end users in this situation.\n>\n> Huh? For those who run `curl url | sudo sh`, I do not think the\n> incoming script setting and exporting LV to an arbitrary value and\n> runing Git is not the top thing they need worry about.\n>\n> While I think enclosing the string in dq is an improvement (as I\n> said already), I still do think your use of the v-word is making a\n> mountain out of an anthill.\n\nI failed to say why I found the dq is an improvement, but that\nshould be in the log message of this commit. Off the top of my\nhead, something like:\n\n\tWe often make sure an environment variable is set to\n\tsomething, either set by the user (in which case we do not\n\tmolest it) or set it to our default value (otherwise), with\n\n\t\t: ${VAR=default value}\n\n\ti.e. running the no-op command \":\" with ${VAR} as its\n\tparameters (or the default value we supply), relying on that\n\t\":\" is a no-op.\n\n\tThis pattern, even though it is no-op from correctness point\n\tof view, still can be expensive if the existing value in VAR\n\thas shell glob (because they will be expanded against\n\tfilesystem entities) and IFS whitespaces (because the value\n\tneed to be split into multiple parameters). Our invocation\n\tof \":\" command does not care if the parameter given to it is\n\tafter the value in VAR goes through these processing.\n\n\tEnclosing the whole thing in double-quote, i.e.\n\n\t\t: \"${VAR=default value}\"\n\n\tavoids paying the unnecessary cost, so let's do so.\n\n"},{"id":"289634","messageId":"20160619180928.16255-1-cuong.manhle.vn@gmail.com","threadId":"42655","inReplyTo":"xmqqvb15vz3y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"LE Manh Cuong","fromEmail":"cuong.manhle.vn@gmail.com","sentAt":"2016-06-19T18:09:28Z","receivedAt":"2016-06-19T18:11:19Z","isPatch":true,"sender":{"key":"cuong.manhle.vn@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4304764?v=4"},"body":"It's really a good commit message, better than mine.\n\nSo must I make another patch or you will update the commit message?\n\nPS: This is the first time I submit a patch to Git, so forgive me\nif I made any silly questions.\n"},{"id":"289643","messageId":"xmqq37o8x514.fsf@gitster.mtv.corp.google.com","threadId":"42655","inReplyTo":"20160619180928.16255-1-cuong.manhle.vn@gmail.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T21:06:15Z","receivedAt":"2016-06-19T21:07:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"LE Manh Cuong writes:\n\n> It's really a good commit message, better than mine.\n>\n> So must I make another patch or you will update the commit message?\n>\n> PS: This is the first time I submit a patch to Git, so forgive me\n> if I made any silly questions.\n\nI can locally amend by replacing the log message when I queue your\noriginal patch, now we seem to have agreed how it should read.\n\nThanks.\n"},{"id":"289644","messageId":"xmqqr3bsvpwu.fsf@gitster.mtv.corp.google.com","threadId":"42655","inReplyTo":"xmqq37o8x514.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-sh-setup.sh: fix missing double quotes variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-19T21:18:09Z","receivedAt":"2016-06-19T21:18:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> LE Manh Cuong writes:\n>\n>> It's really a good commit message, better than mine.\n>>\n>> So must I make another patch or you will update the commit message?\n>>\n>> PS: This is the first time I submit a patch to Git, so forgive me\n>> if I made any silly questions.\n>\n> I can locally amend by replacing the log message when I queue your\n> original patch, now we seem to have agreed how it should read.\n>\n> Thanks.\n\nBy the way, here is the hits from\n\n\t$ git grep '^[ ]*: ${' master\n\nAll of them in t/t4100/* and contrib/examples do not need touching.\nBut others are in the same boat as what you updated in this patch,\ni.e. \"wasteful -- the no-op invocation do not need its $VAR\nexpanded\".\n\ncontrib/examples/git-reset.sh:: ${rev=HEAD}\ncontrib/hooks/post-receive-email:: ${diffopts:=\"--stat --summary --find-copies-harder\"}\ngit-mergetool--lib.sh:: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}\ngit-quiltimport.sh:: ${QUILT_PATCHES:=patches}\ngit-quiltimport.sh:: ${QUILT_SERIES:=$QUILT_PATCHES/series}\ngit-rebase--interactive.sh:: ${comment_char:=#}\ngit-sh-setup.sh:\t: ${LESS=-FRX}\ngit-sh-setup.sh:\t: ${LV=-c}\ngit-sh-setup.sh:\t: ${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\ngit-submodule.sh:: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}\ngit-web--browse.sh:\t: ${browser_path:=\"$1\"}\nt/t1014-read-tree-confusing.sh:\t: ${pretty:=$path}\nt/t1450-fsck.sh:\t\t: ${pretty:=$path}\nt/t4100/t-apply-2.patch: : ${GIT_DIR=.git}\nt/t4100/t-apply-2.patch: : ${GIT_OBJECT_DIRECTORY=\"${SHA1_FILE_DIRECTORY-\"$GIT_DIR/objects\"}\"}\nt/t4100/t-apply-6.patch: : ${GIT_DIR=.git}\nt/t4100/t-apply-6.patch: : ${GIT_OBJECT_DIRECTORY=\"${SHA1_FILE_DIRECTORY-\"$GIT_DIR/objects\"}\"}\nt/test-lib.sh:: ${ASAN_OPTIONS=detect_leaks=0}\nt/valgrind/analyze.sh:: ${TEST_OUTPUT_DIRECTORY=$(dirname \"$0\")/..}\n"}]}