Re: input validation in receive-pack

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Jan 2, 2008, 07:51 UTC
Message-ID: <20080102075152.GA24401@auto.tuwien.ac.at>
In-Reply-To: <7vzlvp3oya.fsf@gitster.siamese.dyndns.org>

On Tue, Jan 01, 2008 at 07:07:25PM -0800, Junio C Hamano wrote:

Show 16 quoted lines

> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:
> > Some lines above:
> > |        if (!prefixcmp(name, "refs/") && check_ref_format(name + 5)) {
> > |                error("refusing to create funny ref '%s' remotely", name);
> > |                return "funny refname";
> > |        }
> >
> > Is this code really correct?
> 
> Interesting.  Things have been this way forever, I think.  I do
> not offhand see any reason not to refuse refs outside refs/, so
> you can try 
> 
> 	if (prefixcmp(name, "refs/") || check_ref_format(name +	5))
> 
> and see what happens.

I tried this and it passed the test suite.

>  Some people may however want to push to
> HEAD (that is ".git/HEAD" which is outside ".git/refs"), though.

If pushing to HEAD is allowed, it bypasses the fast-forward check.

As minimum, if (check_ref_format(name)) should be safer, as it rejects totally invalid refnames (especially ../[...]).

Are there more refs outside "refs/", which somebody would want to push to? If not, the following patch could work:

if (!strcmp(name, "HEAD") && 
   (prefixcmp(name, "refs/") || check_ref_format(name+5))) {
  [..error..]
}

if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
    !is_null_sha1(old_sha1) &&
    (!prefixcmp(name, "refs/heads/")||!strcmp(name, "HEAD")) {
[...]
}

mfg Martin Kögler