[SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

Both bash and zsh subject the value of PS1 to parameter expansion,
command substitution, and arithmetic expansion.  Rather than include
the raw, unescaped branch name in PS1 when running in two- or
three-argument mode, construct PS1 to reference a variable that holds
the branch name.  Because the shells do not recursively expand, this
avoids arbitrary code execution by specially-crafted branch names such
as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
To see the vulnerability in action, follow the instructions at:
    https://github.com/richardhansen/clonepwn

 contrib/completion/git-prompt.sh | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 7b732d2..bd7ff29 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -207,7 +207,18 @@ __git_ps1_show_upstream ()
 			p=" u+${count#*	}-${count%	*}" ;;
 		esac
 		if [[ -n "$count" && -n "$name" ]]; then
-			p="$p $(git rev-parse --abbrev-ref "$upstream" 2>/dev/null)"
+			__git_ps1_upstream_name=$(git rev-parse \
+				--abbrev-ref "$upstream" 2>/dev/null)
+			if [ $pcmode = yes ]; then
+				# see the comments around the
+				# __git_ps1_branch_name variable below
+				p="$p \${__git_ps1_upstream_name}"
+			else
+				p="$p ${__git_ps1_upstream_name}"
+				# not needed anymore; keep user's
+				# environment clean
+				unset __git_ps1_upstream_name
+			fi
 		fi
 	fi
 
@@ -438,8 +449,27 @@ __git_ps1 ()
 		__git_ps1_colorize_gitstring
 	fi
 
+	b=${b##refs/heads/}
+	if [ $pcmode = yes ]; then
+		# In pcmode (and only pcmode) the contents of
+		# $gitstring are subject to expansion by the shell.
+		# Avoid putting the raw ref name in the prompt to
+		# protect the user from arbitrary code execution via
+		# specially crafted ref names (e.g., a ref named
+		# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute
+		# 'sudo rm -rf /' when the prompt is drawn).  Instead,
+		# put the ref name in a new global variable (in the
+		# __git_ps1_* namespace to avoid colliding with the
+		# user's environment) and reference that variable from
+		# PS1.
+		__git_ps1_branch_name=$b
+		# note that the $ is escaped -- the variable will be
+		# expanded later (when it's time to draw the prompt)
+		b="\${__git_ps1_branch_name}"
+	fi
+
 	local f="$w$i$s$u"
-	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
+	local gitstring="$c$b${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
 		if [ "${__git_printf_supports_v-}" != yes ]; then
-- 
1.9.2

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Jeff King]

On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:

> Both bash and zsh subject the value of PS1 to parameter expansion,
> command substitution, and arithmetic expansion.  Rather than include
> the raw, unescaped branch name in PS1 when running in two- or
> three-argument mode, construct PS1 to reference a variable that holds
> the branch name.  Because the shells do not recursively expand, this
> avoids arbitrary code execution by specially-crafted branch names such
> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.

Cute. We already disallow quite a few characters in refnames (including
space, as you probably discovered), and generally enforce that during
ref transfer. I wonder if we should tighten that more as a precuation.
It would be backwards-incompatible, but I wonder if things like "$" and
";" in refnames are actually useful to people.

Did you look into similar exploits with completion? That's probably
slightly less dire (this one hits you as soon as you "cd" into a
malicious clone, whereas completion problems require you to actually hit
<tab>). I'm fairly sure that we miss some quoting on pathnames, for
example. That can lead to bogus completion, but I'm not sure offhand if
it can lead to execution.

-Peff

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

On 2014-04-21 16:24, Jeff King wrote:
> On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:
> 
>> Both bash and zsh subject the value of PS1 to parameter expansion,
>> command substitution, and arithmetic expansion.  Rather than include
>> the raw, unescaped branch name in PS1 when running in two- or
>> three-argument mode, construct PS1 to reference a variable that holds
>> the branch name.  Because the shells do not recursively expand, this
>> avoids arbitrary code execution by specially-crafted branch names such
>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.
> 
> Cute. We already disallow quite a few characters in refnames (including
> space, as you probably discovered), and generally enforce that during
> ref transfer. I wonder if we should tighten that more as a precuation.
> It would be backwards-incompatible, but I wonder if things like "$" and
> ";" in refnames are actually useful to people.

That's a tough call.  I imagine those that legitimately use '$', ';', or
'`' would be annoyed but generally accepting given the security benefit.

I wonder how many repos at sites like GitHub use unusual punctuation in
ref names.

Perhaps the additional character restrictions could be controlled via a
config option.  It would default to the more secure mode but
developers/repo admins could relax it where required.

If imposing additional character restrictions is unpalatable, hooks
could be used to reject funny branch names in shared repos.  But this
would require administrator action -- it's not as secure by default.

> 
> Did you look into similar exploits with completion? That's probably
> slightly less dire (this one hits you as soon as you "cd" into a
> malicious clone, whereas completion problems require you to actually hit
> <tab>). I'm fairly sure that we miss some quoting on pathnames, for
> example. That can lead to bogus completion, but I'm not sure offhand if
> it can lead to execution.

I have not looked at the completion code.

-Richard

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

> Both bash and zsh subject the value of PS1 to parameter expansion,
> command substitution, and arithmetic expansion.  Rather than include
> the raw, unescaped branch name in PS1 when running in two- or
> three-argument mode, construct PS1 to reference a variable that holds
> the branch name.  Because the shells do not recursively expand, this
> avoids arbitrary code execution by specially-crafted branch names such
> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.
>
> Signed-off-by: Richard Hansen <rhansen@bbn.com>

I'd like to see this patch eyeballed by those who have been involved
in the script (shortlog and blame tells me they are SZEDER and
Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is
tagged.

Will queue so that I won't lose it in the meantime.

Thanks.

>  contrib/completion/git-prompt.sh | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 7b732d2..bd7ff29 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -207,7 +207,18 @@ __git_ps1_show_upstream ()
>  			p=" u+${count#*	}-${count%	*}" ;;
>  		esac
>  		if [[ -n "$count" && -n "$name" ]]; then
> -			p="$p $(git rev-parse --abbrev-ref "$upstream" 2>/dev/null)"
> +			__git_ps1_upstream_name=$(git rev-parse \
> +				--abbrev-ref "$upstream" 2>/dev/null)
> +			if [ $pcmode = yes ]; then
> +				# see the comments around the
> +				# __git_ps1_branch_name variable below
> +				p="$p \${__git_ps1_upstream_name}"
> +			else
> +				p="$p ${__git_ps1_upstream_name}"
> +				# not needed anymore; keep user's
> +				# environment clean
> +				unset __git_ps1_upstream_name
> +			fi
>  		fi
>  	fi
>  
> @@ -438,8 +449,27 @@ __git_ps1 ()
>  		__git_ps1_colorize_gitstring
>  	fi
>  
> +	b=${b##refs/heads/}
> +	if [ $pcmode = yes ]; then
> +		# In pcmode (and only pcmode) the contents of
> +		# $gitstring are subject to expansion by the shell.
> +		# Avoid putting the raw ref name in the prompt to
> +		# protect the user from arbitrary code execution via
> +		# specially crafted ref names (e.g., a ref named
> +		# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute
> +		# 'sudo rm -rf /' when the prompt is drawn).  Instead,
> +		# put the ref name in a new global variable (in the
> +		# __git_ps1_* namespace to avoid colliding with the
> +		# user's environment) and reference that variable from
> +		# PS1.
> +		__git_ps1_branch_name=$b
> +		# note that the $ is escaped -- the variable will be
> +		# expanded later (when it's time to draw the prompt)
> +		b="\${__git_ps1_branch_name}"
> +	fi
> +
>  	local f="$w$i$s$u"
> -	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
> +	local gitstring="$c$b${f:+$z$f}$r$p"
>  
>  	if [ $pcmode = yes ]; then
>  		if [ "${__git_printf_supports_v-}" != yes ]; then

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Richard Hansen <rhansen@bbn.com> writes:
>
>> Both bash and zsh subject the value of PS1 to parameter expansion,
>> command substitution, and arithmetic expansion.  Rather than include
>> the raw, unescaped branch name in PS1 when running in two- or
>> three-argument mode, construct PS1 to reference a variable that holds
>> the branch name.  Because the shells do not recursively expand, this
>> avoids arbitrary code execution by specially-crafted branch names such
>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.
>>
>> Signed-off-by: Richard Hansen <rhansen@bbn.com>
>
> I'd like to see this patch eyeballed by those who have been involved
> in the script (shortlog and blame tells me they are SZEDER and
> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is
> tagged.
>
> Will queue so that I won't lose it in the meantime.
>
> Thanks.

Sadly, this does not seem to pass t9903.41 for me.

    $ bash t9903-*.sh -i -v

ends with this: 

    --- expected    2014-04-21 22:31:46.000000000 +0000
    +++ .../t/trash directory.t9903-bash-prompt/actual  ...
    @@ -1 +1 @@
    -BEFORE: (master):AFTER
    \ No newline at end of file
    +BEFORE: (${__git_ps1_branch_name}):AFTER
    \ No newline at end of file
    not ok 41 - prompt - pc mode

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

On 2014-04-21 18:33, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Richard Hansen <rhansen@bbn.com> writes:
>>
>>> Both bash and zsh subject the value of PS1 to parameter expansion,
>>> command substitution, and arithmetic expansion.  Rather than include
>>> the raw, unescaped branch name in PS1 when running in two- or
>>> three-argument mode, construct PS1 to reference a variable that holds
>>> the branch name.  Because the shells do not recursively expand, this
>>> avoids arbitrary code execution by specially-crafted branch names such
>>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.
>>>
>>> Signed-off-by: Richard Hansen <rhansen@bbn.com>
>>
>> I'd like to see this patch eyeballed by those who have been involved
>> in the script (shortlog and blame tells me they are SZEDER and
>> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is
>> tagged.
>>
>> Will queue so that I won't lose it in the meantime.
>>
>> Thanks.
> 
> Sadly, this does not seem to pass t9903.41 for me.
> 
>     $ bash t9903-*.sh -i -v

Oops!  Because git-prompt.sh is in contrib I didn't realize there was a
test for it.

The test will have to change.  I'll think about the best way to adjust
the test and send a reroll.

Thanks,
Richard

[SECURITY PATCH v2] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

Both bash and zsh subject the value of PS1 to parameter expansion,
command substitution, and arithmetic expansion.  Rather than include
the raw, unescaped branch name in PS1 when running in two- or
three-argument mode, construct PS1 to reference a variable that holds
the branch name.  Because the shells do not recursively expand, this
avoids arbitrary code execution by specially-crafted branch names such
as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
Changes since v1:  update t/t9903-bash-prompt.sh

 contrib/completion/git-prompt.sh | 34 +++++++++++++++++++++++++++++--
 t/t9903-bash-prompt.sh           | 44 ++++++++++++++++++++--------------------
 2 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 7b732d2..bd7ff29 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -207,7 +207,18 @@ __git_ps1_show_upstream ()
 			p=" u+${count#*	}-${count%	*}" ;;
 		esac
 		if [[ -n "$count" && -n "$name" ]]; then
-			p="$p $(git rev-parse --abbrev-ref "$upstream" 2>/dev/null)"
+			__git_ps1_upstream_name=$(git rev-parse \
+				--abbrev-ref "$upstream" 2>/dev/null)
+			if [ $pcmode = yes ]; then
+				# see the comments around the
+				# __git_ps1_branch_name variable below
+				p="$p \${__git_ps1_upstream_name}"
+			else
+				p="$p ${__git_ps1_upstream_name}"
+				# not needed anymore; keep user's
+				# environment clean
+				unset __git_ps1_upstream_name
+			fi
 		fi
 	fi
 
@@ -438,8 +449,27 @@ __git_ps1 ()
 		__git_ps1_colorize_gitstring
 	fi
 
+	b=${b##refs/heads/}
+	if [ $pcmode = yes ]; then
+		# In pcmode (and only pcmode) the contents of
+		# $gitstring are subject to expansion by the shell.
+		# Avoid putting the raw ref name in the prompt to
+		# protect the user from arbitrary code execution via
+		# specially crafted ref names (e.g., a ref named
+		# '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)' would execute
+		# 'sudo rm -rf /' when the prompt is drawn).  Instead,
+		# put the ref name in a new global variable (in the
+		# __git_ps1_* namespace to avoid colliding with the
+		# user's environment) and reference that variable from
+		# PS1.
+		__git_ps1_branch_name=$b
+		# note that the $ is escaped -- the variable will be
+		# expanded later (when it's time to draw the prompt)
+		b="\${__git_ps1_branch_name}"
+	fi
+
 	local f="$w$i$s$u"
-	local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p"
+	local gitstring="$c$b${f:+$z$f}$r$p"
 
 	if [ $pcmode = yes ]; then
 		if [ "${__git_printf_supports_v-}" != yes ]; then
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 59f875e..6efd0d9 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -452,53 +452,53 @@ test_expect_success 'prompt - format string starting with dash' '
 '
 
 test_expect_success 'prompt - pc mode' '
-	printf "BEFORE: (master):AFTER" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\nmaster" >expected &&
 	printf "" >expected_output &&
 	(
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
 		test_cmp expected_output "$actual" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}master${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmaster" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual"
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - detached head' '
-	printf "BEFORE: (${c_red}(%s...)${c_clear}):AFTER" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout master" &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_red}*${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmaster" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_green}+${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmaster" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -506,13 +506,13 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmaster" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -521,25 +521,25 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_green}#${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmaster" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		cd otherrepo &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - inside .git directory' '
-	printf "BEFORE: (${c_green}GIT_DIR!${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -547,13 +547,13 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		cd .git &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_lblue}\$${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmaster" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -561,29 +561,29 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 		GIT_PS1_SHOWSTASHSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}master${c_clear} ${c_red}%%${c_clear}):AFTER" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmaster" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" &&
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success 'prompt - zsh color pc mode' '
-	printf "BEFORE: (%%F{green}master%%f):AFTER" >expected &&
+	printf "BEFORE: (%%F{green}\${__git_ps1_branch_name}%%f):AFTER\\nmaster" >expected &&
 	(
 		ZSH_VERSION=5.0.0 &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual"
-		printf "%s" "$PS1" >"$actual"
+		printf "%s\\n%s" "$PS1" "${__git_ps1_branch_name}" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
-- 
1.9.2

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Michael Haggerty]

On 04/21/2014 10:24 PM, Jeff King wrote:
> On Mon, Apr 21, 2014 at 03:07:28PM -0400, Richard Hansen wrote:
> 
>> Both bash and zsh subject the value of PS1 to parameter expansion,
>> command substitution, and arithmetic expansion.  Rather than include
>> the raw, unescaped branch name in PS1 when running in two- or
>> three-argument mode, construct PS1 to reference a variable that holds
>> the branch name.  Because the shells do not recursively expand, this
>> avoids arbitrary code execution by specially-crafted branch names such
>> as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'.
> 
> Cute. We already disallow quite a few characters in refnames (including
> space, as you probably discovered), and generally enforce that during
> ref transfer. I wonder if we should tighten that more as a precuation.
> It would be backwards-incompatible, but I wonder if things like "$" and
> ";" in refnames are actually useful to people.

While we're at it, I think it would be prudent to ban '-' at the
beginning of reference name segments.  For example, reference names like

    refs/heads/--cmd=/sbin/halt
    refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb

are currently both legal, but I think they shouldn't be.  I wouldn't be
surprised if somebody could find a way to exploit
references-named-like-command-line-options.

At a minimum, it is very difficult to write scripts robust against such
names.  Some branch- and tag-oriented commands *require* short names and
don't allow the full reference name including refs/heads/ or refs/tags/
to be specified.  In such cases there is no systematic way to prevent
the names from being seen as command-line options.  And '--' by itself,
which many Unix commands use to separate options from arguments, has a
different meaning in Gitland.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> While we're at it, I think it would be prudent to ban '-' at the
> beginning of reference name segments.  For example, reference names like
>
>     refs/heads/--cmd=/sbin/halt
>     refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb
>
> are currently both legal, but I think they shouldn't be.

I think we forbid these at the Porcelain level ("git branch", "git
checkout -b" and "git tag" should not let you create "-aBranch"),
while leaving the plumbing lax to allow people experimenting with
their repositories.

It may be sensible to discuss and agree on what exactly should be
forbidden (we saw "leading dash", "semicolon and dollar anywhere"
so far in the discussion) and plan for transition to forbid them
everywhere in a next big version bump (it is too late for 2.0).

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

On 2014-04-22 13:38, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> While we're at it, I think it would be prudent to ban '-' at the
>> beginning of reference name segments.  For example, reference names like
>>
>>     refs/heads/--cmd=/sbin/halt
>>     refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb
>>
>> are currently both legal, but I think they shouldn't be.
> 
> I think we forbid these at the Porcelain level ("git branch", "git
> checkout -b" and "git tag" should not let you create "-aBranch"),
> while leaving the plumbing lax to allow people experimenting with
> their repositories.
> 
> It may be sensible to discuss and agree on what exactly should be
> forbidden (we saw "leading dash", "semicolon and dollar anywhere"
> so far in the discussion)

Also backquote anywhere.

> and plan for transition to forbid them
> everywhere in a next big version bump (it is too late for 2.0).

Would it be acceptable to have a config option to forbid these in a
non-major version bump?  Does parsing config files add too much overhead
for this to be feasible?

If it's OK to have a config option, then here's one possible transition
path (probably flawed, but my intent is to bootstrap discussion):

  1. Add an option to forbid dangerous characters.  The option defaults
     to disabled for compatibility.  If the option is unset, print a
     warning upon encountering a ref name that would be forbidden.
  2. Later, flip the default to enabled.
  3. Later, in the weeks/months leading up to the next major version
     release, print the warning even if the config option is set to
     disabled.

Thanks,
Richard

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

>> and plan for transition to forbid them
>> everywhere in a next big version bump (it is too late for 2.0).
>
> Would it be acceptable to have a config option to forbid these in a
> non-major version bump?  

Of course ;-) Because we try very hard to avoid a "flag day" change,
any "plan for transition" inevitably has to include what we need to
do _before_ the big version bump.

> If it's OK to have a config option, then here's one possible transition
> path (probably flawed, but my intent is to bootstrap discussion):
>
>   1. Add an option to forbid dangerous characters.  The option defaults
>      to disabled for compatibility.  If the option is unset, print a
>      warning upon encountering a ref name that would be forbidden.
>   2. Later, flip the default to enabled.
>   3. Later, in the weeks/months leading up to the next major version
>      release, print the warning even if the config option is set to
>      disabled.

Sounds fairly conservative and nice.  We may want to treat creating
a new such ref and using an existing such ref differently, though,
and that might give us a better/smoother transition (as you are, I
am just thinking aloud).

For example, it might be sufficient to do these two things:

 (1) upon an attempt to use an existing such ref, warn and encourage
     renaming of the ref.

 (2) upon an attempt to create a new one, error it out.

in the first step, and in either case, tell the user about the
loosening variable.

Going that route may shorten the time until the initial safety.

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Richard Hansen]

On 2014-04-25 03:37, Simon Oosthoek wrote:
> (though tbh, I think you'd have to be in an automated situation
> to check out a branch that is basically a command to hack your
> system, a human would probably figure it too cumbersome, or too
> fishy)

You can get in trouble by cloning a malicious repository and cding to
the resulting directory.  See:

    https://github.com/richardhansen/clonepwn

for a (benign) demonstration.  (Note the name of the default branch in
that repository -- it's not master.)

> 
>>>> + # not needed anymore; keep user's
>>>> + # environment clean
>>>> + unset __git_ps1_upstream_name
>> 
>> We already have a lot of stuff in the user's environment beginning
>> with __git, so I don't think the unset is necessary.
> 
> If people rely on the string being set in their scripts, it can be
> bad to remove it. But if it's new in this patch,

The variable is new.

> I don't see the need to keep it. Cruft is bad IMO.

Agreed, although I am willing to remove those three lines if that is the
collective preference.

-Richard

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Simon Oosthoek]

* G?bor Szeder <szeder@ira.uka.de> [2014-04-24 23:10:10 +0430]:

> > I'd like to see this patch eyeballed by those who have been involved 
> > in the script (shortlog and blame tells me they are SZEDER and 
> > Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is 
> > tagged.
> 
> I think this is a sensible thing to do.  However, for now I can only check the patch on my phone, hence I can't say any more (e.g. acked or reviewed by) than that, unfortunately.

Ditto for me, though I've gone so far as to try it (it works for me). At the time I wrote the patch I honestly forgot to think about the security implications and from the description, this is closing a hole. There are situations where you're not in control of a branch name (though tbh, I think you'd have to be in an automated situation to check out a branch that is basically a command to hack your system, a human would probably figure it too cumbersome, or too fishy)

> 
> > > + # not needed anymore; keep user's 
> > > + # environment clean 
> > > + unset __git_ps1_upstream_name 
> > > + fi
> 
> We already have a lot of stuff in the user's environment beginning with __git, so I don't think the unset is necessary.

If people rely on the string being set in their scripts, it can be bad to remove it. But if it's new in this patch, I don't see the need to keep it. Cruft is bad IMO.

Cheers

Simon

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

[Gábor Szeder]

Hi,

On Apr 22, 2014 2:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Richard Hansen <rhansen@bbn.com> writes: 
>
> > Both bash and zsh subject the value of PS1 to parameter expansion, 
> > command substitution, and arithmetic expansion.  Rather than include 
> > the raw, unescaped branch name in PS1 when running in two- or 
> > three-argument mode, construct PS1 to reference a variable that holds 
> > the branch name.  Because the shells do not recursively expand, this 
> > avoids arbitrary code execution by specially-crafted branch names such 
> > as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'. 
> > 
> > Signed-off-by: Richard Hansen <rhansen@bbn.com> 
>
> I'd like to see this patch eyeballed by those who have been involved 
> in the script (shortlog and blame tells me they are SZEDER and 
> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is 
> tagged.

I think this is a sensible thing to do.  However, for now I can only check the patch on my phone, hence I can't say any more (e.g. acked or reviewed by) than that, unfortunately.

> > + # not needed anymore; keep user's 
> > + # environment clean 
> > + unset __git_ps1_upstream_name 
> > + fi

We already have a lot of stuff in the user's environment beginning with __git, so I don't think the unset is necessary.

Best,
Gábor