Security issue with bash-git-prompt ? #310

chmike · 2017-03-17T21:08:15Z

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 · 2017-03-17T21:13:28Z

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

jayrhynas · 2017-03-17T21:41:56Z

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 · 2017-03-20T08:20:34Z

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

ogr3 · 2017-03-20T12:41:58Z

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 · 2017-03-20T13:23:24Z

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 · 2017-03-20T13:30:28Z

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

jayrhynas · 2017-03-20T13:34:36Z

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 · 2017-03-20T13:52:23Z

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 · 2017-03-20T14:12:17Z

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 · 2017-03-20T14:29:42Z

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 · 2017-03-20T14:42:57Z

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 · 2017-03-20T15:08:49Z

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 · 2017-03-20T15:13:42Z

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.

chmike · 2017-03-20T15:15:08Z

In fact the NEW_PROMPT contains the unevaluated branch name. But it is evaluated when displayed as prompt.

jayrhynas · 2017-03-20T15:15:24Z

@chmike It's just escaping the $ so it doesn't get expanded until the the assignment of PS1="${NEW_PROMPT..."

chmike · 2017-03-20T15:23:19Z

Thanks.

Here is a fix for the branch which is slightly more elegant.

diff --git a/gitprompt.sh b/gitprompt.sh
index 9ff5b42..f2afe9d 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}\${GIT_BRANCH}${ResetColor}"
+    local STATUS=""
 
     # __add_status KIND VALEXPR INSERT
     # eg: __add_status  'STAGED' '-ne 0'
@@ -553,7 +554,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$STATUS$PROMPT_END"
   else
     NEW_PROMPT="$EMPTY_PROMPT"
   fi

chmike · 2017-03-20T15:27:43Z

I'll try the GIT_UPSTREAM now. I would append it escaped at STATUS_PREFIX.

PS: How did you managed to get a colored diff ? That is nicer then mine.

jayrhynas · 2017-03-20T15:33:07Z

@chmike Surround the diff with

```diff
<diff>
```

Not sure how to handle GIT_UPSTREAM (I'm not sure why it isn't showing up when escaped)

ogr3 · 2017-03-20T15:45:43Z

I think it's because GIT_UPSTREAM is a local variable and GIT_BRANCH is exported. So when PS1 is evaluated GIT_UPSTREAM is out of scope.

If you export it you get the problem with color escape codes.

[fredrik@fredriks: ~/Dokument/github/pw3nage] [$(./pw3n) {\[\033[0;34m\]origin/$(./pw3n)\[\033[0;0m\]}|✚ 1] ✔

jayrhynas · 2017-03-20T15:55:29Z

Ah that makes sense. I'm not sure exactly what the rules are for bash evaluation, but I think the best way to evaluate the colors without evaluating the user input is to either filter the upstream name to be safe (no $ or ` etc) and eval the GIT_UPSTREAM as normal, or add the colors for GIT_UPSTREAM at the very end.

chmike · 2017-03-20T16:16:41Z

EDIT: fixed an error with local variable. It now works as expected.

diff --git a/gitprompt.sh b/gitprompt.sh
index 9ff5b42..5aee916 100755
--- a/gitprompt.sh
+++ b/gitprompt.sh
@@ -496,11 +496,10 @@ function updatePrompt() {
     unset GIT_REMOTE
   fi
 
-  local GIT_UPSTREAM="${git_status_fields[2]}"
+  export GIT_UPSTREAM="${git_status_fields[2]}"
+  local GIT_FORMATED_UPSTREAM="${GIT_PROMPT_UPSTREAM//_UPSTREAM_/\$GIT_UPSTREAM}"
   if [[ -z "${__GIT_PROMPT_SHOW_UPSTREAM}" || "^" == "$GIT_UPSTREAM" ]]; then
-    unset GIT_UPSTREAM
-  else
-    GIT_UPSTREAM="${GIT_PROMPT_UPSTREAM//_UPSTREAM_/${GIT_UPSTREAM}}"
+    unset GIT_FORMATED_UPSTREAM
   fi
 
   local GIT_STAGED=${git_status_fields[3]}
@@ -512,7 +511,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}\${GIT_BRANCH}${ResetColor}$GIT_FORMATED_UPSTREAM"
+    local STATUS=""
 
     # __add_status KIND VALEXPR INSERT
     # eg: __add_status  'STAGED' '-ne 0'
@@ -542,7 +542,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 +552,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$STATUS$PROMPT_END"
   else
     NEW_PROMPT="$EMPTY_PROMPT"
   fi

ogr3 · 2017-03-20T16:32:44Z

Good catch to use the unescaped variable in the substitution! Seems to work fine for me, I just did a typo-fix:

GIT_FORMATTED_UPSTREAM

chmike · 2017-03-20T16:40:56Z

Sorry, I Edited my answer. It didn't work when displaying a branch without upstream.
I guess it should be possible to make the code related to UPSTREAM cleaner.
The exported GIT_UPSTREAM should be cleared or undefined when there is no upstream. I didn't know how to code that. Also, we only need to define the local GIT_FORMATED_UPSTREAM variable if there is one. I'm also not sure how to do that correctly. When happens if the variable doesn't exit and it is appended to STATUS_PREFIX ? I have reached the limit of my competence :)

ogr3 · 2017-03-20T16:50:23Z

Your fix works if there is a local repo without upstream - bash won't have a problem with an unset variable - it will just not render anything.

Couldn't you just continue to unset the GIT_UPSTREAM like before?

diff --git a/gitprompt.sh b/gitprompt.sh
index 9ff5b42..03c109a 100755
--- a/gitprompt.sh
+++ b/gitprompt.sh
@@ -496,11 +496,11 @@ function updatePrompt() {
     unset GIT_REMOTE
   fi
 
-  local GIT_UPSTREAM="${git_status_fields[2]}"
+  export GIT_UPSTREAM="${git_status_fields[2]}"
+  local GIT_FORMATTED_UPSTREAM="${GIT_PROMPT_UPSTREAM//_UPSTREAM_/\$GIT_UPSTREAM}"
   if [[ -z "${__GIT_PROMPT_SHOW_UPSTREAM}" || "^" == "$GIT_UPSTREAM" ]]; then
     unset GIT_UPSTREAM
-  else
-    GIT_UPSTREAM="${GIT_PROMPT_UPSTREAM//_UPSTREAM_/${GIT_UPSTREAM}}"
+    unset GIT_FORMATTED_UPSTREAM
   fi
 
   local GIT_STAGED=${git_status_fields[3]}
@@ -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}\${GIT_BRANCH}${ResetColor}${GIT_FORMATTED_UPSTREAM}"
+    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$STATUS$PROMPT_END"
   else
     NEW_PROMPT="$EMPTY_PROMPT"
   fi

ogr3 · 2017-03-20T18:50:46Z

I made a PR with these changes, would like for some more people to test it so that nothing that we haven't thought of breaks.

chmike · 2017-03-20T19:41:46Z

May I suggest a small refactoring ?

local TMP_GIT_UPSTREAM="${git_status_fields[2]}"
if [[ -z "${__GIT_PROMPT_SHOW_UPSTREAM}" ||  "^" == "$TMP_GIT_UPSTREAM" ]]; then
    unset GIT_UPSTREAM
else
    export GIT_UPSTREAM="${TMP_GIT_UPSTREAM}"
    local GIT_FORMATTED_UPSTREAM="${GIT_PROMPT_UPSTREAM//_UPSTREAM_/\$GIT_UPSTREAM}"
fi

ogr3 · 2017-03-20T20:03:23Z

I updated the PR @chmike, thanks for the feedback.

Fixes #310

magicmonty · 2017-03-26T17:54:51Z

Thanks to all for the hints and the fix.

magicmonty added bug up-for-grabs labels Mar 20, 2017

ogr3 mentioned this issue Mar 20, 2017

[security] Fix for #310 #313

Merged

magicmonty closed this as completed in #313 Mar 26, 2017

magicmonty added a commit that referenced this issue Mar 26, 2017

Merge branch 'ogr3-pwned-with-upstream'

c4a0a2d

Fixes #310

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security issue with bash-git-prompt ? #310

Security issue with bash-git-prompt ? #310

chmike commented Mar 17, 2017 •

edited

Loading

chmike commented Mar 17, 2017 •

edited

Loading

jayrhynas commented Mar 17, 2017 •

edited

Loading

magicmonty commented Mar 20, 2017

ogr3 commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017

magicmonty commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

chmike commented Mar 20, 2017

ogr3 commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

ogr3 commented Mar 20, 2017

chmike commented Mar 20, 2017

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

chmike commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

ogr3 commented Mar 20, 2017

jayrhynas commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017 •

edited

Loading

ogr3 commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017

ogr3 commented Mar 20, 2017

ogr3 commented Mar 20, 2017

chmike commented Mar 20, 2017 •

edited

Loading

ogr3 commented Mar 20, 2017

magicmonty commented Mar 26, 2017

Security issue with bash-git-prompt ? #310

Security issue with bash-git-prompt ? #310

Comments

chmike commented Mar 17, 2017 • edited Loading

chmike commented Mar 17, 2017 • edited Loading

jayrhynas commented Mar 17, 2017 • edited Loading

magicmonty commented Mar 20, 2017

ogr3 commented Mar 20, 2017 • edited Loading

chmike commented Mar 20, 2017

magicmonty commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

chmike commented Mar 20, 2017

ogr3 commented Mar 20, 2017 • edited Loading

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

ogr3 commented Mar 20, 2017

chmike commented Mar 20, 2017

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

chmike commented Mar 20, 2017 • edited Loading

chmike commented Mar 20, 2017

jayrhynas commented Mar 20, 2017

ogr3 commented Mar 20, 2017

jayrhynas commented Mar 20, 2017 • edited Loading

chmike commented Mar 20, 2017 • edited Loading

ogr3 commented Mar 20, 2017 • edited Loading

chmike commented Mar 20, 2017

ogr3 commented Mar 20, 2017

ogr3 commented Mar 20, 2017

chmike commented Mar 20, 2017 • edited Loading

ogr3 commented Mar 20, 2017

magicmonty commented Mar 26, 2017

chmike commented Mar 17, 2017 •

edited

Loading

chmike commented Mar 17, 2017 •

edited

Loading

jayrhynas commented Mar 17, 2017 •

edited

Loading

ogr3 commented Mar 20, 2017 •

edited

Loading

ogr3 commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017 •

edited

Loading

jayrhynas commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017 •

edited

Loading

ogr3 commented Mar 20, 2017 •

edited

Loading

chmike commented Mar 20, 2017 •

edited

Loading