All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jeff King <peff@peff.net>, Richard Hansen <rhansen@bbn.com>
Cc: git@vger.kernel.org, sitaramc@gmail.com
Subject: Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1
Date: Tue, 22 Apr 2014 10:38:46 +0200	[thread overview]
Message-ID: <53562A96.6000002@alum.mit.edu> (raw)
In-Reply-To: <20140421202454.GA6062@sigill.intra.peff.net>

On 04/21/2014 10:24 PM, 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.

While we're at it, I think it would be prudent to ban '-' at the
beginning of reference name segments.  For example, reference names like

    refs/heads/--cmd=/sbin/halt
    refs/tags/--exec=forkbomb(){forkbomb|forkbomb&};forkbomb

are currently both legal, but I think they shouldn't be.  I wouldn't be
surprised if somebody could find a way to exploit
references-named-like-command-line-options.

At a minimum, it is very difficult to write scripts robust against such
names.  Some branch- and tag-oriented commands *require* short names and
don't allow the full reference name including refs/heads/ or refs/tags/
to be specified.  In such cases there is no systematic way to prevent
the names from being seen as command-line options.  And '--' by itself,
which many Unix commands use to separate options from arguments, has a
different meaning in Gitland.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

  parent reply	other threads:[~2014-04-22  8:38 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-21 19:07 [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 Richard Hansen
2014-04-21 20:24 ` Jeff King
2014-04-21 21:07   ` Richard Hansen
2014-04-22  8:38   ` Michael Haggerty [this message]
2014-04-22 17:38     ` Junio C Hamano
2014-04-22 18:38       ` Richard Hansen
2014-04-22 19:47         ` Junio C Hamano
2014-04-21 22:23 ` Junio C Hamano
2014-04-21 22:33   ` Junio C Hamano
2014-04-21 22:58     ` Richard Hansen
2014-04-21 23:53       ` [SECURITY PATCH v2] " Richard Hansen
  -- strict thread matches above, loose matches on Subject: below --
2014-04-24 18:40 [SECURITY PATCH] " Gábor Szeder
2014-04-25  7:37 ` Simon Oosthoek
2014-04-25 16:39   ` Richard Hansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53562A96.6000002@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=rhansen@bbn.com \
    --cc=sitaramc@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.