All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Hansen <rhansen@bbn.com>
To: Junio C Hamano <gitster@pobox.com>,
	Michael Haggerty <mhagger@alum.mit.edu>
Cc: Jeff King <peff@peff.net>, 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 14:38:18 -0400	[thread overview]
Message-ID: <5356B71A.6070500@bbn.com> (raw)
In-Reply-To: <xmqqy4yx5knw.fsf@gitster.dls.corp.google.com>

On 2014-04-22 13:38, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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 think we forbid these at the Porcelain level ("git branch", "git
> checkout -b" and "git tag" should not let you create "-aBranch"),
> while leaving the plumbing lax to allow people experimenting with
> their repositories.
> 
> It may be sensible to discuss and agree on what exactly should be
> forbidden (we saw "leading dash", "semicolon and dollar anywhere"
> so far in the discussion)

Also backquote anywhere.

> and plan for transition to forbid them
> everywhere in a next big version bump (it is too late for 2.0).

Would it be acceptable to have a config option to forbid these in a
non-major version bump?  Does parsing config files add too much overhead
for this to be feasible?

If it's OK to have a config option, then here's one possible transition
path (probably flawed, but my intent is to bootstrap discussion):

  1. Add an option to forbid dangerous characters.  The option defaults
     to disabled for compatibility.  If the option is unset, print a
     warning upon encountering a ref name that would be forbidden.
  2. Later, flip the default to enabled.
  3. Later, in the weeks/months leading up to the next major version
     release, print the warning even if the config option is set to
     disabled.

Thanks,
Richard

  reply	other threads:[~2014-04-22 18: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
2014-04-22 17:38     ` Junio C Hamano
2014-04-22 18:38       ` Richard Hansen [this message]
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=5356B71A.6070500@bbn.com \
    --to=rhansen@bbn.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=peff@peff.net \
    --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.