Re: input validation in receive-pack

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

In the function "static const char *update(struct command *cmd)" in
receive-pack.c:

|        if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
|            !is_null_sha1(old_sha1) &&
|            !prefixcmp(name, "refs/heads/")) {
|                struct commit *old_commit, *new_commit;
|                struct commit_list *bases, *ent;
|
|                old_commit = (struct commit *)parse_object(old_sha1);
|                new_commit = (struct commit *)parse_object(new_sha1);
|                bases = get_merge_bases(old_commit, new_commit, 1);
|                for (ent = bases; ent; ent = ent->next)
|                        if (!hashcmp(old_sha1, ent->item->object.sha1))
|                                break;
|                free_commit_list(bases);
|                if (!ent) {
|                        error("denying non-fast forward %s"
|                              " (you should pull first)", name);
|                        return "non-fast forward";
|                }
|        }

As far as I understand the code, it assumes, that sha1 values provided
by the client really point to a commit. Shouldn't there be a check for
the object type?

Yes, 11031d7e9f34f6a20ff4a4bd4fa3e5e3c0024a57 seems to have been
a bit sloppy.  The codepath to delete a ref may need to be lax
(see 28391a80a94d2b59d1d21f8264fe5dab91d77249) but there is no
excuse not to be strict when updating.

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.  Some people may however want to push to
HEAD (that is ".git/HEAD" which is outside ".git/refs"), though.

In the update code path, the check is done in refs.c:
| struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
| {
|         if (check_ref_format(ref) == -1)
|                 return NULL;
|         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
| }

check_ref_format may also return -2 (less than two name levels) and -3
(* at the end), which are ignored. Is it really intended, that
receive-pack can create such refs.

Misconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that
changed check_ref_format() without looking at what its callers
are checking, I think.

On Tue, 1 Jan 2008, Junio C Hamano wrote:

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

quoted

In the update code path, the check is done in refs.c:
| struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
| {
|         if (check_ref_format(ref) == -1)
|                 return NULL;
|         return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
| }

check_ref_format may also return -2 (less than two name levels) and -3
(* at the end), which are ignored. Is it really intended, that
receive-pack can create such refs.

Misconversion in 8558fd9ece4c8250a037a6d5482a8040d600ef47 that
changed check_ref_format() without looking at what its callers
are checking, I think.

When I got to it, it was already accepting -2. It clearly shouldn't accept 
-3 (and I don't know why I missed it; I was probably misinterpreting the 
original logic there.

	-Daniel
*This .sig left intentionally blank*

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

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

quoted

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