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

[Richard Hansen <rhansen@bbn.com>]

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