Re: [PATCH] create_ref_entry(): move check_refname_format() call to callers

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/30/2012 07:14 PM, Junio C Hamano wrote:
> Michael Haggerty<mhagger@alum.mit.edu>  writes:
>
>> There are not two but three classes of refnames:
>>
>> 1. Fully legal refnames (according to the rules of check-ref-format).
>>
>> 2. Refnames that might cause parsing trouble in some circumstances but
>> could otherwise be tolerated (with a warning) in the hope that the
>> user can delete them before they cause further confusion.  These
>> include all illegal refnames that aren't covered by (3).
>>
>> 3. Refnames that are so pathological that we don't want to let them
>> into our code at all, under any circumstances.  This category, I
>> think, includes refnames that include "/../" (because they could cause
>> our code to walk up the filesystem) or LF (because if written to a
>> packed-refs file they would make it unreadable) and perhaps "//"
>> (because they would confuse the loose reference code and the
>> hierarchical reference cache code).  And depending on how much you
>> trust shell scripts to do quoting correctly, other patterns might also
>> be risky.
>>[...]
>> Treating category (3) the same as category (2) was more or less the
>> status quo before the changes, but I think it is dangerous, especially
>> when dealing with references that might come from remote sources (do
>> you disagree?).
>
> I actually do. [...example involving class (2) refname omitted...]
>
> What kind of "danger" do you have in mind?

I agree with you about how class (2) refnames can be handled, namely on 
a best-effort basis, without paranoia.  The "danger" that I worry about 
is if we treat class (3) refnames the same non-paranoid way that we 
treat class (2) refnames.  To be sure, my worries are at the level of 
"it seems like a lot of data paths have to be trusted and I have not 
personally verified that there is no danger and I have a vivid 
imagination" rather than "I have figured out how to implement an exploit".

For example, have all of the following code paths been audited to make 
sure that they cannot introduce class (3) refnames into a repository 
(including via symbolic refs with class (3) targets) even in the face of 
a malicious remote?  Can we (and do we want to) rely on this level of 
vigilance being sustained in the future?

* git clone (for all transport types)?

* git push (for all transport types)?

* git fast-import?

* git am?

* All kinds of remote helpers (git svn, etc.) and import scripts (e.g., 
the malicious refs might come directly from a Subversion repository)?

...no doubt there are others.

What if a malicious repository is copied using rsync (i.e., not "git 
clone rsync:..." but straight rsync)?  Granted, the victim would have 
worse problems if he didn't delete any version of $GIT/config that was 
contained in the copy before using it.

Banning class (3) references at lib level would provide a second level 
of defense against errors in the outer level.  Allowing "class 3" 
references at the lib level, by comparison, means putting a lot of 
confidence in the auditing of all of the ref-creating code paths.

It's true that a loose reference cannot (by construction) contain 
constructs like "/../" or "//".  And packed refs are not stored via 
filesystem paths, so such constructs are not a direct hazard.  But there 
are lots of ways that a packed ref can be turned into a loose ref, and 
the process of creating a loose ref from a class (3) packed ref could 
cause trouble.

I propose that I implement a REFNAME_NONSTRICT flag which relaxes the 
normal rules as follows:

* ".." is only forbidden if it is a complete refname component

* All characters (including control characters, '?', '*', and '[') are 
allowed, except for NUL, SP, LF, ':', and '\'.

* The character sequence "@{" is *not* prohibited.

The reason I suggest excluding ':' and '\' is because of their 
significance in DOS pathnames; let me know if you think this is unnecessary.

Do you want these changes in master or in maint (or even older)?

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/