patchcreate_ref_entry(): move check_refname_format() call to callers

From: Michael Haggerty <mhagger@alum.mit.edu>

This allows the caller to decide what flags to use for check_refname_format().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

On 04/27/2012 05:06 PM, Junio C Hamano wrote:

I will work on providing more infrastructure for checking refnames at varying levels of strictness, but I don't know enough about the code paths to be able to find the places where the strictness levels need tweaking.

For this to work, the various callers of check_refname_format() will have to be able to influence the level of strictness that they want to enforce. This patch is one trivial step in that direction.

 refs.c |   23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

On Sun, Apr 29, 2012 at 08:18:08AM +0200, mhagger@alum.mit.edu wrote:

It seems like the create_ref_entry code paths should _always_ just be issuing warnings, as they are about reading existing refs, no? The die() side should only come when we are writing refs.

So something like this patch:

Jeff King <peff@peff.net> writes:

That matches my observation.  Michael, are we missing something?

On 04/29/2012 01:58 PM, Jeff King wrote:

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.

I would like to be able to distinguish between (2) and (3) before deciding what to do in any specific cases. References in category (2) can probably be accepted (with a warning) in old data but we should not allow the user to create new ones. References in category (3) are more critical; I see three options for dealing with them: die(), emit a warning then drop them on the floor, or emit a warning but handle them somehow (for example, by URL-encoding them).

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?).

Regarding create_ref_entry(), there are three callers:

* read_packed_refs(): Only used to read references from local
   packed-refs files.  Seems uncritical in terms of security, so for now
   I suppose we could change this caller to emit a warning but use the
   reference anyway.  (It would not be a good idea to emit a warning but
   *not* use the reference, because the next time the packed-refs file
   is rewritten the reference would be lost.)

* get_ref_dir(): Only used to read loose references from the local
   filesystem.  Seems uncritical in terms of security, so for now
   I suppose we could change this caller to emit a warning but use the
   reference anyway.  Alternately, we could emit a warning but not use
   the reference; this would not result in any data loss because nobody
   would have a reason to remove the loose reference file.

* add_packed_ref(): Used by write_remote_refs() to insert references
   from a cloned repository into the local packed-refs file.  Here I
   think we have to be paranoid about accepting refnames in category
   (3).  For example, it might be reasonable to emit a warning
   mentioning the illegal reference name *and the SHA1 that it referred
   to* then to drop it on the floor.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

I can buy that we would want to reject /../ or // from the command line even when we are not creating such a ref (e.g. "git symbolic-ref HEAD refs/heads/../heads/foo"). Note that even though we won't read any loose ref of that form from the repository ever (unless your recent ref API updates broke it ;-), we _could_ see them in .git/packed-refs if the user manually edited the file. In such a case, I would say that recovering from such a corruption is totally up to the user, just like it is up to you after you edit the bits on the disk platter to confuse the filesystem code.

But if we were to do so, class (3) needs to be tightly controlled. For example, a ref "refs/heads/m${LF}aster", e.g.

    $ git update-ref 'refs/heads/m
    aster' HEAD

may have been usable with an ancient git, and as long as the you never packed your refs, you should be able to see it with 'git branch', and you should be able to fix it with 'git branch master "m${LF}aster"'.

There is no probably about it. We should warn and accept---anything else is a regression---and refuse to create.

As long as class (3) is tightly controlled, die() until the corruption is sorted out (if it is coming to us by reading from existing data) would be the best option to avoid spreading the breakage further.

I actually do. You say "parsing trouble" in (2), but what are the examples of "parsing trouble" you have in mind? "refs/heads/a..b" is an illegal name per check-ref-format and is (and should be) in class (2), but as long as we do not create such a ref, if we happen to be able to read it (maybe "git rev-parse HEAD >.git/refs/heads/a..b" dropped it there), the user should be able to see it with "git branch", and should be able to recover with "git branch -m a..b a-dot-dot-b". Until the user does so, we cannot sensibly respond to "git log a..b" (which is the "parsing trouble" you are referring to, I think), of course, but that is to be expected.

What kind of "danger" do you have in mind?

There is no _could_. This must show it with a warning and let the caller use its value.

Likewise.

The caller should be checking the names and rejecting the attempt to create a ref with a bogus name.

I think create_ref_entry(), if the reader does not pay much attention to the "_entry" part of its name, gives a false impression that this is about "creating a ref", but it isn't. It is actually about keeping an in-core image of the outside world, and the one that affects the outside world (either loose or packed refs) should be making sure the names are safe ones to use.

Thanks for a good summary.

On 04/30/2012 07:14 PM, Junio C Hamano wrote:

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> writes:

Auditing is one thing, but perhaps the right solution to that issue is to refactor the existing code so that we have only a handful (preferrably one) API entry point that is used to create a new ref (not to be confused with create_ref_entry(), which is not necessarily about creating a ref)?

The UI layer may place additional restrictions to the source data used to eventually lead to a ref creation (e.g. your updated "git branch" may forbid you to create a branch with the name of an existing tag, perhaps), but after passing its check, the API to create a new ref will do mandatory "check-ref-format" check.

On 04/30/2012 11:11 PM, Junio C Hamano wrote:

Yes, definitely. And more broadly, I want refs.{c,h} to become the *only* mechanism for working with refs.

Michael