{"thread":{"id":"36493","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","startedAt":"2014-04-24T18:40:10Z","lastAt":"2014-04-25T16:39:47Z","messageCount":3,"participants":["Gábor Szeder","Simon Oosthoek","Richard Hansen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239590","messageId":"E1WdOZY-0006ck-F9@iramx2.ira.uni-karlsruhe.de","threadId":"36493","inReplyTo":null,"subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Gábor Szeder","fromEmail":"szeder@ira.uka.de","sentAt":"2014-04-24T18:40:10Z","receivedAt":"2014-04-24T18:40:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Apr 22, 2014 2:53 AM, Junio C Hamano wrote:\n>\n> Richard Hansen writes: \n>\n> > Both bash and zsh subject the value of PS1 to parameter expansion, \n> > command substitution, and arithmetic expansion.  Rather than include \n> > the raw, unescaped branch name in PS1 when running in two- or \n> > three-argument mode, construct PS1 to reference a variable that holds \n> > the branch name.  Because the shells do not recursively expand, this \n> > avoids arbitrary code execution by specially-crafted branch names such \n> > as '$(IFS=_;cmd=sudo_rm_-rf_/;$cmd)'. \n> > \n> > Signed-off-by: Richard Hansen \n>\n> I'd like to see this patch eyeballed by those who have been involved \n> in the script (shortlog and blame tells me they are SZEDER and \n> Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is \n> tagged.\n\nI 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.\n\n> > + # not needed anymore; keep user's \n> > + # environment clean \n> > + unset __git_ps1_upstream_name \n> > + fi\n\nWe already have a lot of stuff in the user's environment beginning with __git, so I don't think the unset is necessary.\n\nBest,\nGábor\n"},{"id":"239634","messageId":"20140425073724.GA9384@xs4all.nl","threadId":"36493","inReplyTo":"E1WdOZY-0006ck-F9@iramx2.ira.uni-karlsruhe.de","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Simon Oosthoek","fromEmail":"s.oosthoek@xs4all.nl","sentAt":"2014-04-25T07:37:24Z","receivedAt":"2014-04-25T07:37:24Z","isPatch":true,"sender":{"key":"s.oosthoek@xs4all.nl","avatar":"https://avatars.githubusercontent.com/u/11677720?v=4"},"body":"* G?bor Szeder [2014-04-24 23:10:10 +0430]:\n\n> > I'd like to see this patch eyeballed by those who have been involved \n> > in the script (shortlog and blame tells me they are SZEDER and \n> > Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is \n> > tagged.\n> \n> 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.\n\nDitto 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)\n\n> \n> > > + # not needed anymore; keep user's \n> > > + # environment clean \n> > > + unset __git_ps1_upstream_name \n> > > + fi\n> \n> We already have a lot of stuff in the user's environment beginning with __git, so I don't think the unset is necessary.\n\nIf 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.\n\nCheers\n\nSimon\n"},{"id":"239668","messageId":"535A8FD3.2070409@bbn.com","threadId":"36493","inReplyTo":"20140425073724.GA9384@xs4all.nl","subject":"Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2014-04-25T16:39:47Z","receivedAt":"2014-04-25T16:39:47Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2014-04-25 03:37, Simon Oosthoek wrote:\n> (though tbh, I think you'd have to be in an automated situation\n> to check out a branch that is basically a command to hack your\n> system, a human would probably figure it too cumbersome, or too\n> fishy)\n\nYou can get in trouble by cloning a malicious repository and cding to\nthe resulting directory. See:\n\n https://github.com/richardhansen/clonepwn\n\nfor a (benign) demonstration. (Note the name of the default branch in\nthat repository -- it's not master.)\n\n> \n>>>> + # not needed anymore; keep user's\n>>>> + # environment clean\n>>>> + unset __git_ps1_upstream_name\n>> \n>> We already have a lot of stuff in the user's environment beginning\n>> with __git, so I don't think the unset is necessary.\n> \n> If people rely on the string being set in their scripts, it can be\n> bad to remove it. But if it's new in this patch,\n\nThe variable is new.\n\n> I don't see the need to keep it. Cruft is bad IMO.\n\nAgreed, although I am willing to remove those three lines if that is the\ncollective preference.\n\n-Richard\n"}]}