# Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 3 messages from 2014-04-24 to 2014-04-25. Participants: Gábor Szeder, Simon Oosthoek, Richard Hansen. Thread: https://gitlist.dev/t/36493 ## Gábor Szeder, 2014-04-24 18:40 Subject: Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 Message-ID: URL: https://gitlist.dev/e/E1WdOZY-0006ck-F9%40iramx2.ira.uni-karlsruhe.de ``` Hi, On Apr 22, 2014 2:53 AM, Junio C Hamano wrote: > > Richard Hansen 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 > > 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 ``` ## Simon Oosthoek, 2014-04-25 07:37 Subject: Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 Message-ID: <20140425073724.GA9384@xs4all.nl> URL: https://gitlist.dev/e/20140425073724.GA9384%40xs4all.nl In-Reply-To: ``` * G?bor Szeder [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 ``` ## Richard Hansen, 2014-04-25 16:39 Subject: Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 Message-ID: <535A8FD3.2070409@bbn.com> URL: https://gitlist.dev/e/535A8FD3.2070409%40bbn.com In-Reply-To: <20140425073724.GA9384@xs4all.nl> ``` 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 ```