Subject: Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

From: Michael Haggerty <[email-masked]>
Date: Wed, 20 Aug 2014 20:34:43 +0200

On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote:
> On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty <[email-masked]> wrote:
>> I'm a little worried that abandoning *all* refname checks could open us
>> up to somehow trying to delete a "reference" with a name like
>> "../../../../etc/passwd".  Either such names have to be prohibited
>> somehow, or we have to be very sure that they can only come from trusted
>> sources.
> 
> I only set this flag from builtin/branch.c so it should only be used
> when a user runs 'git branch -D' from the command line.
> All other places where we delete branches we should still be checking
> the rename for badness.
> 
> That said, unless the "rules for good refname" changes in the future,
> which is unlikely, is should be exceptionally rare that a user ends up
> with a bad refname in the first place.
> Perhaps my example I gave was bad since if you manually create bad
> refs using echo > .git/refs/heads/...  then you should probably know
> how to fix it too and thus maybe we do not need this patch in the
> first place.
> 
> Do you want me to delete this patch and resend this part of the series
> ? Or is the 'only works for branch -D from the commandline' sufficient
> ?
> I have no strong feelings either way so I will just follow what you decide.

I think that if you run the refname through normalize_path_copy_len()
and that function returns (1) without an error, (2) without modifying
its argument, and (3) the result does not begin with a
has_dos_drive_prefix() or is_dir_sep(), then we should be safe against
directory traversal attacks.  I suggest doing this kind of check even if
not doing the full check_refname_format() check.

Michael

-- 
Michael Haggerty
[email-masked]