On Wed, Aug 20, 2014 at 11:34 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > 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. Good idea. Let me add this. > > 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