On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote: > On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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 mhagger@xxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html