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

On Wed, Aug 20, 2014 at 11:34 AM, Michael Haggerty [off-list ref] wrote:

On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote:

quoted

On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty [off-list ref] wrote:

quoted

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.

Good idea.
Let me add this.

Michael

--
Michael Haggerty
mhagger@alum.mit.edu