Security issue with bash-git-prompt ?

[chmike]

I have seen this report of possible hack with some shell git prompts. I didn't test if it works with bash-git-prompt.

The trick is to name a git branch as '$(./nastyScript)'. When the shell displays the branch name, the shell will execute nastyScript which should be in the local directory.

Is bash-git-prompt exposed to this risk ?

[chmike]

SrslyJosh just reported on HackerNews that bash-git-prompt is vulnerable.

[jayrhynas]

The official git-prompt script addresses this issue by storing the branch name in a variable and only substituting the variable once into the final PS1 assignment.

Edit: It actually still uses some indirection but achieves the same result.

[magicmonty]

As I currently have not that much time I would need some help here, as this is quite a serious issue.

[ogr3]

The first naive approach would be to use replaceSymbols on all branch names and filter out anything but a-z,0-9 and slash, see #312

You could allow more characters (like A-Z) This one at least makes the original pwnage go away. Needs some code review though.

[chmike]

Some people use '-' and '.' in their branch names. This proposal would create a problem for all these people.

The solution should be to not interpret the string containing the branch name.

[magicmonty]

How about escaping special characters like $ etc. if found?

[jayrhynas]

AFAICT the escaping would only last through one level of substitution. replaceSymbols would need to be modified to handle that, and GIT_BRANCH and GIT_UPSTREAM (and others?) would need to only be substituted into PS1 at the end.

[chmike]

Here is a simple test we can use by using a simple environment variable substitution.
git co -b '${EDITOR}'

This requires that you have the environment variable EDITOR defined.
The branch name of the prompt is the value of EDITOR.

A good news is that gitstatus.py and gitstatus.sh are ok.

As expected, $ is not the only problem. A branch name with back ticks is also a problem.

git co -b '`ls`'

The prompt shows the content of the directory where is should show the branch name.

I had the same problem when using * for untracked branches. The * was interpreted as a glob and replaced by the list of file names in the directory.

Are these substitutions performed in the bash-git-prompt script or when $ps1 is substituted by the shell ?

[ogr3]

Yes I know that the correct way would be to make sure that the branch name is not part of any eval - that will be more tricky to implement though, so the quick fix is to remove $ and backtick I guess.

The biggest problem is that we make an explicit eval of the STATUS variable on several occasions when updating the prompt. See the __add_status function.

[chmike]

Ok. Just found it. So the ideal solution would be to get rid of this eval. It is unsafe to use eval with user input. We can't be sure if removing $ and ` is enough.

[jayrhynas]

Something like this might work

diff --git a/gitprompt.sh b/gitprompt.sh
index 9ff5b42..735b595 100755
--- a/gitprompt.sh
+++ b/gitprompt.sh
@@ -512,7 +512,8 @@ function updatePrompt() {
 
   local NEW_PROMPT="$EMPTY_PROMPT"
   if [[ -n "$git_status_fields" ]]; then
-    local STATUS="${PROMPT_LEADING_SPACE}${GIT_PROMPT_PREFIX}${GIT_PROMPT_BRANCH}${GIT_BRANCH}${ResetColor}"
+    local STATUS_PREFIX="${PROMPT_LEADING_SPACE}${GIT_PROMPT_PREFIX}${GIT_PROMPT_BRANCH}"
+    local STATUS=""
 
     # __add_status KIND VALEXPR INSERT
     # eg: __add_status  'STAGED' '-ne 0'
@@ -542,7 +543,6 @@ function updatePrompt() {
       eval "STATUS=\"$STATUS$1\""
     }
 
-    __add_status        '$GIT_UPSTREAM'
     __chk_gitvar_status 'REMOTE'     '-n'
     __add_status        "$GIT_PROMPT_SEPARATOR"
     __chk_gitvar_status 'STAGED'     '-ne 0'
@@ -553,7 +553,7 @@ function updatePrompt() {
     __chk_gitvar_status 'CLEAN'      '-eq 1'   -
     __add_status        "$ResetColor$GIT_PROMPT_SUFFIX"
 
-    NEW_PROMPT="$(gp_add_virtualenv_to_prompt)$PROMPT_START$($prompt_callback)$STATUS$PROMPT_END"
+    NEW_PROMPT="$(gp_add_virtualenv_to_prompt)$PROMPT_START$($prompt_callback)$STATUS_PREFIX\${GIT_BRANCH}${ResetColor}\${GIT_UPSTREAM}$STATUS$PROMPT_END"
   else
     NEW_PROMPT="$EMPTY_PROMPT"
   fi

[ogr3]

The approach works for the current branch, but not for upstream for me. Upstream won't show.

You can test it by setting GIT_PROMPT_SHOW_UPSTREAM=1

[chmike]

I'm not an expert with bash programming.
What is the effect of the backslash in front of the \${GIT_BRANCH} and \${GIT_UPSTREAM} ?
It's not easy to do a gooqle search on \

Why can't we leave the ${GIT_BRANCH}${ResetColor} at the end of the STATUS_PREFIX variable ?
I tried it and it didn't work.