{"thread":{"id":"36464","subject":"[SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","startedAt":"2014-04-21T19:07:28Z","lastAt":"2014-04-22T19:47:57Z","messageCount":11,"participants":["Richard Hansen","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239226","messageId":"1398107248-32140-1-git-send-email-rhansen@bbn.com","threadId":"36464","inReplyTo":null,"subject":"[SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-21T19:07:28Z","receivedAt":"2014-04-21T19:07:28Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Both bash and zsh subject the value of PS1 to parameter expansion,\ncommand substitution, and arithmetic expansion.  Rather than include\nthe raw, unescaped branch name in PS1 when running in two- or\nthree-argument mode, construct PS1 to reference a variable that holds\nthe branch name.  Because the shells do not recursively expand, this\navoids arbitrary code execution by specially-crafted branch names such\nas '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\nTo see the vulnerability in action, follow the instructions at:\n    https://github.com/richardhansen/clonepwn\n\n contrib/completion/git-prompt.sh | 34 ++++++++++++++++++++++++++++++++--\n 1 file changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 7b732d2..bd7ff29 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -207,7 +207,18 @@ __git_ps1_show_upstream ()\n \t\t\tp=\" u+${count#*\t}-${count%\t*}\" ;;\n \t\tesac\n \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n-\t\t\tp=\"$p $(git rev-parse --abbrev-ref \"$upstream\" 2>/dev/null)\"\n+\t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n+\t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n+\t\t\tif [ $pcmode = yes ]; then\n+\t\t\t\t# see the comments around the\n+\t\t\t\t# __git_ps1_branch_name variable below\n+\t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n+\t\t\telse\n+\t\t\t\tp=\"$p ${__git_ps1_upstream_name}\"\n+\t\t\t\t# not needed anymore; keep user's\n+\t\t\t\t# environment clean\n+\t\t\t\tunset __git_ps1_upstream_name\n+\t\t\tfi\n \t\tfi\n \tfi\n \n@@ -438,8 +449,27 @@ __git_ps1 ()\n \t\t__git_ps1_colorize_gitstring\n \tfi\n \n+\tb=${b##refs/heads/}\n+\tif [ $pcmode = yes ]; then\n+\t\t# In pcmode (and only pcmode) the contents of\n+\t\t# $gitstring are subject to expansion by the shell.\n+\t\t# Avoid putting the raw ref name in the prompt to\n+\t\t# protect the user from arbitrary code execution via\n+\t\t# specially crafted ref names (e.g., a ref named\n+\t\t# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute\n+\t\t# 'sudo rm -rf /' when the prompt is drawn).  Instead,\n+\t\t# put the ref name in a new global variable (in the\n+\t\t# __git_ps1_* namespace to avoid colliding with the\n+\t\t# user's environment) and reference that variable from\n+\t\t# PS1.\n+\t\t__git_ps1_branch_name=$b\n+\t\t# note that the $ is escaped -- the variable will be\n+\t\t# expanded later (when it's time to draw the prompt)\n+\t\tb=\"\\${__git_ps1_branch_name}\"\n+\tfi\n+\n \tlocal f=\"$w$i$s$u\"\n-\tlocal gitstring=\"$c${b##refs/heads/}${f:+$z$f}$r$p\"\n+\tlocal gitstring=\"$c$b${f:+$z$f}$r$p\"\n \n \tif [ $pcmode = yes ]; then\n \t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\n-- \n1.9.2\n"},{"id":"239228","messageId":"20140421202454.GA6062@sigill.intra.peff.net","threadId":"36464","inReplyTo":"1398107248-32140-1-git-send-email-rhansen@bbn.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-21T20:24:54Z","receivedAt":"2014-04-21T20:24:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:\n\n> Both bash and zsh subject the value of PS1 to parameter expansion,\n> command substitution, and arithmetic expansion.  Rather than include\n> the raw, unescaped branch name in PS1 when running in two- or\n> three-argument mode, construct PS1 to reference a variable that holds\n> the branch name.  Because the shells do not recursively expand, this\n> avoids arbitrary code execution by specially-crafted branch names such\n> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n\nCute. We already disallow quite a few characters in refnames (including\nspace, as you probably discovered), and generally enforce that during\nref transfer. I wonder if we should tighten that more as a precuation.\nIt would be backwards-incompatible, but I wonder if things like \"$\" and\n\";\" in refnames are actually useful to people.\n\nDid you look into similar exploits with completion? That's probably\nslightly less dire (this one hits you as soon as you \"cd\" into a\nmalicious clone, whereas completion problems require you to actually hit\n<tab>). I'm fairly sure that we miss some quoting on pathnames, for\nexample. That can lead to bogus completion, but I'm not sure offhand if\nit can lead to execution.\n\n-Peff\n"},{"id":"239236","messageId":"53558886.5080102@bbn.com","threadId":"36464","inReplyTo":"20140421202454.GA6062@sigill.intra.peff.net","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-21T21:07:18Z","receivedAt":"2014-04-21T21:07:18Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-04-21 16:24, Jeff King wrote:\n> On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:\n> \n>> Both bash and zsh subject the value of PS1 to parameter expansion,\n>> command substitution, and arithmetic expansion.  Rather than include\n>> the raw, unescaped branch name in PS1 when running in two- or\n>> three-argument mode, construct PS1 to reference a variable that holds\n>> the branch name.  Because the shells do not recursively expand, this\n>> avoids arbitrary code execution by specially-crafted branch names such\n>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n> \n> Cute. We already disallow quite a few characters in refnames (including\n> space, as you probably discovered), and generally enforce that during\n> ref transfer. I wonder if we should tighten that more as a precuation.\n> It would be backwards-incompatible, but I wonder if things like \"$\" and\n> \";\" in refnames are actually useful to people.\n\nThat's a tough call.  I imagine those that legitimately use '$', ';', or\n'`' would be annoyed but generally accepting given the security benefit.\n\nI wonder how many repos at sites like GitHub use unusual punctuation in\nref names.\n\nPerhaps the additional character restrictions could be controlled via a\nconfig option.  It would default to the more secure mode but\ndevelopers/repo admins could relax it where required.\n\nIf imposing additional character restrictions is unpalatable, hooks\ncould be used to reject funny branch names in shared repos.  But this\nwould require administrator action -- it's not as secure by default.\n\n> \n> Did you look into similar exploits with completion? That's probably\n> slightly less dire (this one hits you as soon as you \"cd\" into a\n> malicious clone, whereas completion problems require you to actually hit\n> <tab>). I'm fairly sure that we miss some quoting on pathnames, for\n> example. That can lead to bogus completion, but I'm not sure offhand if\n> it can lead to execution.\n\nI have not looked at the completion code.\n\n-Richard\n"},{"id":"239254","messageId":"xmqq61m28gqj.fsf@gitster.dls.corp.google.com","threadId":"36464","inReplyTo":"1398107248-32140-1-git-send-email-rhansen@bbn.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-21T22:23:00Z","receivedAt":"2014-04-21T22:23:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n> Both bash and zsh subject the value of PS1 to parameter expansion,\n> command substitution, and arithmetic expansion.  Rather than include\n> the raw, unescaped branch name in PS1 when running in two- or\n> three-argument mode, construct PS1 to reference a variable that holds\n> the branch name.  Because the shells do not recursively expand, this\n> avoids arbitrary code execution by specially-crafted branch names such\n> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n>\n> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n\nI'd like to see this patch eyeballed by those who have been involved\nin the script (shortlog and blame tells me they are SZEDER and\nSimon, CC'ed), so that we can hopefully merge it by the time -rc1 is\ntagged.\n\nWill queue so that I won't lose it in the meantime.\n\nThanks.\n\n>  contrib/completion/git-prompt.sh | 34 ++++++++++++++++++++++++++++++++--\n>  1 file changed, 32 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 7b732d2..bd7ff29 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -207,7 +207,18 @@ __git_ps1_show_upstream ()\n>  \t\t\tp=\" u+${count#*\t}-${count%\t*}\" ;;\n>  \t\tesac\n>  \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n> -\t\t\tp=\"$p $(git rev-parse --abbrev-ref \"$upstream\" 2>/dev/null)\"\n> +\t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n> +\t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n> +\t\t\tif [ $pcmode = yes ]; then\n> +\t\t\t\t# see the comments around the\n> +\t\t\t\t# __git_ps1_branch_name variable below\n> +\t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n> +\t\t\telse\n> +\t\t\t\tp=\"$p ${__git_ps1_upstream_name}\"\n> +\t\t\t\t# not needed anymore; keep user's\n> +\t\t\t\t# environment clean\n> +\t\t\t\tunset __git_ps1_upstream_name\n> +\t\t\tfi\n>  \t\tfi\n>  \tfi\n>  \n> @@ -438,8 +449,27 @@ __git_ps1 ()\n>  \t\t__git_ps1_colorize_gitstring\n>  \tfi\n>  \n> +\tb=${b##refs/heads/}\n> +\tif [ $pcmode = yes ]; then\n> +\t\t# In pcmode (and only pcmode) the contents of\n> +\t\t# $gitstring are subject to expansion by the shell.\n> +\t\t# Avoid putting the raw ref name in the prompt to\n> +\t\t# protect the user from arbitrary code execution via\n> +\t\t# specially crafted ref names (e.g., a ref named\n> +\t\t# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute\n> +\t\t# 'sudo rm -rf /' when the prompt is drawn).  Instead,\n> +\t\t# put the ref name in a new global variable (in the\n> +\t\t# __git_ps1_* namespace to avoid colliding with the\n> +\t\t# user's environment) and reference that variable from\n> +\t\t# PS1.\n> +\t\t__git_ps1_branch_name=$b\n> +\t\t# note that the $ is escaped -- the variable will be\n> +\t\t# expanded later (when it's time to draw the prompt)\n> +\t\tb=\"\\${__git_ps1_branch_name}\"\n> +\tfi\n> +\n>  \tlocal f=\"$w$i$s$u\"\n> -\tlocal gitstring=\"$c${b##refs/heads/}${f:+$z$f}$r$p\"\n> +\tlocal gitstring=\"$c$b${f:+$z$f}$r$p\"\n>  \n>  \tif [ $pcmode = yes ]; then\n>  \t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\n"},{"id":"239256","messageId":"xmqq1twq8g91.fsf@gitster.dls.corp.google.com","threadId":"36464","inReplyTo":"xmqq61m28gqj.fsf@gitster.dls.corp.google.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-21T22:33:30Z","receivedAt":"2014-04-21T22:33:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Richard Hansen <rhansen@bbn.com> writes:\n>\n>> Both bash and zsh subject the value of PS1 to parameter expansion,\n>> command substitution, and arithmetic expansion.  Rather than include\n>> the raw, unescaped branch name in PS1 when running in two- or\n>> three-argument mode, construct PS1 to reference a variable that holds\n>> the branch name.  Because the shells do not recursively expand, this\n>> avoids arbitrary code execution by specially-crafted branch names such\n>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n>>\n>> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n>\n> I'd like to see this patch eyeballed by those who have been involved\n> in the script (shortlog and blame tells me they are SZEDER and\n> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is\n> tagged.\n>\n> Will queue so that I won't lose it in the meantime.\n>\n> Thanks.\n\nSadly, this does not seem to pass t9903.41 for me.\n\n    $ bash t9903-*.sh -i -v\n\nends with this: \n\n    --- expected    2014-04-21 22:31:46.000000000 +0000\n    +++ .../t/trash directory.t9903-bash-prompt/actual  ...\n    @@ -1 +1 @@\n    -BEFORE: (master):AFTER\n    \\ No newline at end of file\n    +BEFORE: (${__git_ps1_branch_name}):AFTER\n    \\ No newline at end of file\n    not ok 41 - prompt - pc mode\n"},{"id":"239273","messageId":"5355A280.6020409@bbn.com","threadId":"36464","inReplyTo":"xmqq1twq8g91.fsf@gitster.dls.corp.google.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-21T22:58:08Z","receivedAt":"2014-04-21T22:58:08Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-04-21 18:33, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Richard Hansen <rhansen@bbn.com> writes:\n>>\n>>> Both bash and zsh subject the value of PS1 to parameter expansion,\n>>> command substitution, and arithmetic expansion.  Rather than include\n>>> the raw, unescaped branch name in PS1 when running in two- or\n>>> three-argument mode, construct PS1 to reference a variable that holds\n>>> the branch name.  Because the shells do not recursively expand, this\n>>> avoids arbitrary code execution by specially-crafted branch names such\n>>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n>>>\n>>> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n>>\n>> I'd like to see this patch eyeballed by those who have been involved\n>> in the script (shortlog and blame tells me they are SZEDER and\n>> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is\n>> tagged.\n>>\n>> Will queue so that I won't lose it in the meantime.\n>>\n>> Thanks.\n> \n> Sadly, this does not seem to pass t9903.41 for me.\n> \n>     $ bash t9903-*.sh -i -v\n\nOops!  Because git-prompt.sh is in contrib I didn't realize there was a\ntest for it.\n\nThe test will have to change.  I'll think about the best way to adjust\nthe test and send a reroll.\n\nThanks,\nRichard\n"},{"id":"239275","messageId":"1398124389-16627-1-git-send-email-rhansen@bbn.com","threadId":"36464","inReplyTo":"5355A280.6020409@bbn.com","subject":"[SECURITY PATCH v2] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-21T23:53:09Z","receivedAt":"2014-04-21T23:53:09Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Both bash and zsh subject the value of PS1 to parameter expansion,\ncommand substitution, and arithmetic expansion.  Rather than include\nthe raw, unescaped branch name in PS1 when running in two- or\nthree-argument mode, construct PS1 to reference a variable that holds\nthe branch name.  Because the shells do not recursively expand, this\navoids arbitrary code execution by specially-crafted branch names such\nas '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\nChanges since v1:  update t/t9903-bash-prompt.sh\n\n contrib/completion/git-prompt.sh | 34 +++++++++++++++++++++++++++++--\n t/t9903-bash-prompt.sh           | 44 ++++++++++++++++++++--------------------\n 2 files changed, 54 insertions(+), 24 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 7b732d2..bd7ff29 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -207,7 +207,18 @@ __git_ps1_show_upstream ()\n \t\t\tp=\" u+${count#*\t}-${count%\t*}\" ;;\n \t\tesac\n \t\tif [[ -n \"$count\" && -n \"$name\" ]]; then\n-\t\t\tp=\"$p $(git rev-parse --abbrev-ref \"$upstream\" 2>/dev/null)\"\n+\t\t\t__git_ps1_upstream_name=$(git rev-parse \\\n+\t\t\t\t--abbrev-ref \"$upstream\" 2>/dev/null)\n+\t\t\tif [ $pcmode = yes ]; then\n+\t\t\t\t# see the comments around the\n+\t\t\t\t# __git_ps1_branch_name variable below\n+\t\t\t\tp=\"$p \\${__git_ps1_upstream_name}\"\n+\t\t\telse\n+\t\t\t\tp=\"$p ${__git_ps1_upstream_name}\"\n+\t\t\t\t# not needed anymore; keep user's\n+\t\t\t\t# environment clean\n+\t\t\t\tunset __git_ps1_upstream_name\n+\t\t\tfi\n \t\tfi\n \tfi\n \n@@ -438,8 +449,27 @@ __git_ps1 ()\n \t\t__git_ps1_colorize_gitstring\n \tfi\n \n+\tb=${b##refs/heads/}\n+\tif [ $pcmode = yes ]; then\n+\t\t# In pcmode (and only pcmode) the contents of\n+\t\t# $gitstring are subject to expansion by the shell.\n+\t\t# Avoid putting the raw ref name in the prompt to\n+\t\t# protect the user from arbitrary code execution via\n+\t\t# specially crafted ref names (e.g., a ref named\n+\t\t# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute\n+\t\t# 'sudo rm -rf /' when the prompt is drawn).  Instead,\n+\t\t# put the ref name in a new global variable (in the\n+\t\t# __git_ps1_* namespace to avoid colliding with the\n+\t\t# user's environment) and reference that variable from\n+\t\t# PS1.\n+\t\t__git_ps1_branch_name=$b\n+\t\t# note that the $ is escaped -- the variable will be\n+\t\t# expanded later (when it's time to draw the prompt)\n+\t\tb=\"\\${__git_ps1_branch_name}\"\n+\tfi\n+\n \tlocal f=\"$w$i$s$u\"\n-\tlocal gitstring=\"$c${b##refs/heads/}${f:+$z$f}$r$p\"\n+\tlocal gitstring=\"$c$b${f:+$z$f}$r$p\"\n \n \tif [ $pcmode = yes ]; then\n \t\tif [ \"${__git_printf_supports_v-}\" != yes ]; then\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex 59f875e..6efd0d9 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -452,53 +452,53 @@ test_expect_success 'prompt - format string starting with dash' '\n '\n \n test_expect_success 'prompt - pc mode' '\n-\tprintf \"BEFORE: (master):AFTER\" >expected &&\n+\tprintf \"BEFORE: (\\${__git_ps1_branch_name}):AFTER\\\\nmaster\" >expected &&\n \tprintf \"\" >expected_output &&\n \t(\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" >\"$actual\" &&\n \t\ttest_cmp expected_output \"$actual\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - branch name' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear}):AFTER\\\\nmaster\" >expected &&\n \t(\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" >\"$actual\"\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - detached head' '\n-\tprintf \"BEFORE: (${c_red}(%s...)${c_clear}):AFTER\" $(git log -1 --format=\"%h\" b1^) >expected &&\n+\tprintf \"BEFORE: (${c_red}\\${__git_ps1_branch_name}${c_clear}):AFTER\\\\n(%s...)\" $(git log -1 --format=\"%h\" b1^) >expected &&\n \tgit checkout b1^ &&\n \ttest_when_finished \"git checkout master\" &&\n \t(\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_red}*${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\\\nmaster\" >expected &&\n \techo \"dirty\" >file &&\n \ttest_when_finished \"git reset --hard\" &&\n \t(\n \t\tGIT_PS1_SHOWDIRTYSTATE=y &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_green}+${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\\\nmaster\" >expected &&\n \techo \"dirty\" >file &&\n \ttest_when_finished \"git reset --hard\" &&\n \tgit add -u &&\n@@ -506,13 +506,13 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt\n \t\tGIT_PS1_SHOWDIRTYSTATE=y &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\\\nmaster\" >expected &&\n \techo \"dirty index\" >file &&\n \ttest_when_finished \"git reset --hard\" &&\n \tgit add -u &&\n@@ -521,25 +521,25 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\tGIT_PS1_SHOWDIRTYSTATE=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_green}#${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\\\nmaster\" >expected &&\n \t(\n \t\tGIT_PS1_SHOWDIRTYSTATE=y &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\tcd otherrepo &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - inside .git directory' '\n-\tprintf \"BEFORE: (${c_green}GIT_DIR!${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear}):AFTER\\\\nGIT_DIR!\" >expected &&\n \techo \"dirty\" >file &&\n \ttest_when_finished \"git reset --hard\" &&\n \t(\n@@ -547,13 +547,13 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\tcd .git &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - stash status indicator' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_lblue}\\$${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_lblue}\\$${c_clear}):AFTER\\\\nmaster\" >expected &&\n \techo 2 >file &&\n \tgit stash &&\n \ttest_when_finished \"git stash drop\" &&\n@@ -561,29 +561,29 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '\n \t\tGIT_PS1_SHOWSTASHSTATE=y &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '\n-\tprintf \"BEFORE: (${c_green}master${c_clear} ${c_red}%%${c_clear}):AFTER\" >expected &&\n+\tprintf \"BEFORE: (${c_green}\\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\\\nmaster\" >expected &&\n \t(\n \t\tGIT_PS1_SHOWUNTRACKEDFILES=y &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" &&\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n \n test_expect_success 'prompt - zsh color pc mode' '\n-\tprintf \"BEFORE: (%%F{green}master%%f):AFTER\" >expected &&\n+\tprintf \"BEFORE: (%%F{green}\\${__git_ps1_branch_name}%%f):AFTER\\\\nmaster\" >expected &&\n \t(\n \t\tZSH_VERSION=5.0.0 &&\n \t\tGIT_PS1_SHOWCOLORHINTS=y &&\n \t\t__git_ps1 \"BEFORE:\" \":AFTER\" >\"$actual\"\n-\t\tprintf \"%s\" \"$PS1\" >\"$actual\"\n+\t\tprintf \"%s\\\\n%s\" \"$PS1\" \"${__git_ps1_branch_name}\" >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n-- \n1.9.2\n"},{"id":"239316","messageId":"53562A96.6000002@alum.mit.edu","threadId":"36464","inReplyTo":"20140421202454.GA6062@sigill.intra.peff.net","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-22T08:38:46Z","receivedAt":"2014-04-22T08:38:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/21/2014 10:24 PM, Jeff King wrote:\n> On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:\n> \n>> Both bash and zsh subject the value of PS1 to parameter expansion,\n>> command substitution, and arithmetic expansion.  Rather than include\n>> the raw, unescaped branch name in PS1 when running in two- or\n>> three-argument mode, construct PS1 to reference a variable that holds\n>> the branch name.  Because the shells do not recursively expand, this\n>> avoids arbitrary code execution by specially-crafted branch names such\n>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.\n> \n> Cute. We already disallow quite a few characters in refnames (including\n> space, as you probably discovered), and generally enforce that during\n> ref transfer. I wonder if we should tighten that more as a precuation.\n> It would be backwards-incompatible, but I wonder if things like \"$\" and\n> \";\" in refnames are actually useful to people.\n\nWhile we're at it, I think it would be prudent to ban '-' at the\nbeginning of reference name segments.  For example, reference names like\n\n    refs/heads/--cmd=/sbin/halt\n    refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb\n\nare currently both legal, but I think they shouldn't be.  I wouldn't be\nsurprised if somebody could find a way to exploit\nreferences-named-like-command-line-options.\n\nAt a minimum, it is very difficult to write scripts robust against such\nnames.  Some branch- and tag-oriented commands *require* short names and\ndon't allow the full reference name including refs/heads/ or refs/tags/\nto be specified.  In such cases there is no systematic way to prevent\nthe names from being seen as command-line options.  And '--' by itself,\nwhich many Unix commands use to separate options from arguments, has a\ndifferent meaning in Gitland.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239355","messageId":"xmqqy4yx5knw.fsf@gitster.dls.corp.google.com","threadId":"36464","inReplyTo":"53562A96.6000002@alum.mit.edu","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T17:38:43Z","receivedAt":"2014-04-22T17:38:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> While we're at it, I think it would be prudent to ban '-' at the\n> beginning of reference name segments.  For example, reference names like\n>\n>     refs/heads/--cmd=/sbin/halt\n>     refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb\n>\n> are currently both legal, but I think they shouldn't be.\n\nI think we forbid these at the Porcelain level (\"git branch\", \"git\ncheckout -b\" and \"git tag\" should not let you create \"-aBranch\"),\nwhile leaving the plumbing lax to allow people experimenting with\ntheir repositories.\n\nIt may be sensible to discuss and agree on what exactly should be\nforbidden (we saw \"leading dash\", \"semicolon and dollar anywhere\"\nso far in the discussion) and plan for transition to forbid them\neverywhere in a next big version bump (it is too late for 2.0).\n"},{"id":"239359","messageId":"5356B71A.6070500@bbn.com","threadId":"36464","inReplyTo":"xmqqy4yx5knw.fsf@gitster.dls.corp.google.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-22T18:38:18Z","receivedAt":"2014-04-22T18:38:18Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-04-22 13:38, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> While we're at it, I think it would be prudent to ban '-' at the\n>> beginning of reference name segments.  For example, reference names like\n>>\n>>     refs/heads/--cmd=/sbin/halt\n>>     refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb\n>>\n>> are currently both legal, but I think they shouldn't be.\n> \n> I think we forbid these at the Porcelain level (\"git branch\", \"git\n> checkout -b\" and \"git tag\" should not let you create \"-aBranch\"),\n> while leaving the plumbing lax to allow people experimenting with\n> their repositories.\n> \n> It may be sensible to discuss and agree on what exactly should be\n> forbidden (we saw \"leading dash\", \"semicolon and dollar anywhere\"\n> so far in the discussion)\n\nAlso backquote anywhere.\n\n> and plan for transition to forbid them\n> everywhere in a next big version bump (it is too late for 2.0).\n\nWould it be acceptable to have a config option to forbid these in a\nnon-major version bump?  Does parsing config files add too much overhead\nfor this to be feasible?\n\nIf it's OK to have a config option, then here's one possible transition\npath (probably flawed, but my intent is to bootstrap discussion):\n\n  1. Add an option to forbid dangerous characters.  The option defaults\n     to disabled for compatibility.  If the option is unset, print a\n     warning upon encountering a ref name that would be forbidden.\n  2. Later, flip the default to enabled.\n  3. Later, in the weeks/months leading up to the next major version\n     release, print the warning even if the config option is set to\n     disabled.\n\nThanks,\nRichard\n"},{"id":"239374","messageId":"xmqqr44p4042.fsf@gitster.dls.corp.google.com","threadId":"36464","inReplyTo":"5356B71A.6070500@bbn.com","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T19:47:57Z","receivedAt":"2014-04-22T19:47:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n>> and plan for transition to forbid them\n>> everywhere in a next big version bump (it is too late for 2.0).\n>\n> Would it be acceptable to have a config option to forbid these in a\n> non-major version bump?  \n\nOf course ;-) Because we try very hard to avoid a \"flag day\" change,\nany \"plan for transition\" inevitably has to include what we need to\ndo _before_ the big version bump.\n\n> If it's OK to have a config option, then here's one possible transition\n> path (probably flawed, but my intent is to bootstrap discussion):\n>\n>   1. Add an option to forbid dangerous characters.  The option defaults\n>      to disabled for compatibility.  If the option is unset, print a\n>      warning upon encountering a ref name that would be forbidden.\n>   2. Later, flip the default to enabled.\n>   3. Later, in the weeks/months leading up to the next major version\n>      release, print the warning even if the config option is set to\n>      disabled.\n\nSounds fairly conservative and nice.  We may want to treat creating\na new such ref and using an existing such ref differently, though,\nand that might give us a better/smoother transition (as you are, I\nam just thinking aloud).\n\nFor example, it might be sufficient to do these two things:\n\n (1) upon an attempt to use an existing such ref, warn and encourage\n     renaming of the ref.\n\n (2) upon an attempt to create a new one, error it out.\n\nin the first step, and in either case, tell the user about the\nloosening variable.\n\nGoing that route may shorten the time until the initial safety.\n"}]}