input validation in receive-pack

input validation in receive-pack

[Martin Koegler]

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?

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? All refnames starting with "refs/" may
only contain allowed characters, while all other may contain any
characters (except \0 and \n)?

For the updating code path, lock_any_ref_for_update calls
check_ref_format, so the error will happen latter. For the delete code
path, the refname seems not to be checked.

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.

mfg Martin Kögler

Re: input validation in receive-pack

[Junio C Hamano]

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.

Re: input validation in receive-pack

[Daniel Barkalow]

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

> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:
> 
> > 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*

Re: input validation in receive-pack

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 1 Jan 2008, Junio C Hamano wrote:
>
>> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:
>> 
>> > 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.

You are right.

builtin-commit.c uses it to lock "HEAD", and update_ref() calls
it to update any ref so we cannot reject -2.  However, I do not
think allowing wildcard is useful for any caller.

-- >8 --
lock_any_ref_for_update(): reject wildcard return from check_ref_format

Recent check_ref_format() returns -3 as well as -1 (general
error) and -2 (less than two levels).  The caller was explicitly
checking for -1, to allow "HEAD" but still disallow bogus refs.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c |   27 ++++++++++++++++++---------
 refs.h |    5 ++++-
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 759924d..7484a46 100644
--- a/refs.c
+++ b/refs.c
@@ -613,32 +613,37 @@ int check_ref_format(const char *ref)
 		while ((ch = *cp++) == '/')
 			; /* tolerate duplicated slashes */
 		if (!ch)
-			return -1; /* should not end with slashes */
+			/* should not end with slashes */
+			return CHECK_REF_FORMAT_ERROR;
 
 		/* we are at the beginning of the path component */
 		if (ch == '.')
-			return -1;
+			return CHECK_REF_FORMAT_ERROR;
 		bad_type = bad_ref_char(ch);
 		if (bad_type) {
-			return (bad_type == 2 && !*cp) ? -3 : -1;
+			return (bad_type == 2 && !*cp)
+				? CHECK_REF_FORMAT_WILDCARD
+				: CHECK_REF_FORMAT_ERROR;
 		}
 
 		/* scan the rest of the path component */
 		while ((ch = *cp++) != 0) {
 			bad_type = bad_ref_char(ch);
 			if (bad_type) {
-				return (bad_type == 2 && !*cp) ? -3 : -1;
+				return (bad_type == 2 && !*cp)
+					? CHECK_REF_FORMAT_WILDCARD
+					: CHECK_REF_FORMAT_ERROR;
 			}
 			if (ch == '/')
 				break;
 			if (ch == '.' && *cp == '.')
-				return -1;
+				return CHECK_REF_FORMAT_ERROR;
 		}
 		level++;
 		if (!ch) {
 			if (level < 2)
-				return -2; /* at least of form "heads/blah" */
-			return 0;
+				return CHECK_REF_FORMAT_ONELEVEL;
+			return CHECK_REF_FORMAT_OK;
 		}
 	}
 }
@@ -816,9 +821,13 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 
 struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
 {
-	if (check_ref_format(ref) == -1)
+	switch (check_ref_format(ref)) {
+	case CHECK_REF_FORMAT_ERROR:
+	case CHECK_REF_FORMAT_WILDCARD:
 		return NULL;
-	return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
+	default:
+		return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
+	}
 }
 
 static struct lock_file packlock;
diff --git a/refs.h b/refs.h
index 9dc8aa0..9cd16f8 100644
--- a/refs.h
+++ b/refs.h
@@ -52,7 +52,10 @@ int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);
  */
 extern int for_each_reflog(each_ref_fn, void *);
 
-/** Returns 0 if target has the right format for a ref. **/
+#define CHECK_REF_FORMAT_OK 0
+#define CHECK_REF_FORMAT_ERROR (-1)
+#define CHECK_REF_FORMAT_ONELEVEL (-2)
+#define CHECK_REF_FORMAT_WILDCARD (-3)
 extern int check_ref_format(const char *target);
 
 /** rename ref, return 0 on success **/

Re: input validation in receive-pack

[Daniel Barkalow]

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

> @@ -816,9 +821,13 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
>  
>  struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
>  {
> -	if (check_ref_format(ref) == -1)
> +	switch (check_ref_format(ref)) {
> +	case CHECK_REF_FORMAT_ERROR:
> +	case CHECK_REF_FORMAT_WILDCARD:
>  		return NULL;
> -	return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
> +	default:
> +		return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);

It might be wise to make "default" the return NULL case, and list the two 
okay cases explicitly, so it doesn't need to be changed if 
check_ref_format() someday gets additional "okay for some purposes" 
values.

Aside from that, it looks good, except that builtin-send-pack.c and 
fast-import.c should probably use the symbolic constants, too. (All other 
callers only check whether the value is true or not).

	-Daniel
*This .sig left intentionally blank*

Re: input validation in receive-pack

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> It might be wise to make "default" the return NULL case, and list the two 
> okay cases explicitly, so it doesn't need to be changed if 
> check_ref_format() someday gets additional "okay for some purposes" 
> values.

Sounds sensible.

> Aside from that, it looks good, except that builtin-send-pack.c and 
> fast-import.c should probably use the symbolic constants, too. (All other 
> callers only check whether the value is true or not).

Also sensible.

-- >8 --
Update callers of check_ref_format()

This updates send-pack and fast-import to use symbolic constants
for checking the return values from check_ref_format(), and also
futureproof the logic in lock_any_ref_for_update() to explicitly
name the case that is usually considered an error but is Ok for
this particular use.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-send-pack.c |   10 ++++++----
 fast-import.c       |    5 +++--
 refs.c              |    6 +++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 25ae1fe..8afb1d0 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -541,10 +541,12 @@ static void verify_remote_names(int nr_heads, const char **heads)
 		remote = remote ? (remote + 1) : heads[i];
 		switch (check_ref_format(remote)) {
 		case 0: /* ok */
-		case -2: /* ok but a single level -- that is fine for
-			  * a match pattern.
-			  */
-		case -3: /* ok but ends with a pattern-match character */
+		case CHECK_REF_FORMAT_ONELEVEL:
+			/* ok but a single level -- that is fine for
+			 * a match pattern.
+			 */
+		case CHECK_REF_FORMAT_WILDCARD:
+			/* ok but ends with a pattern-match character */
 			continue;
 		}
 		die("remote part of refspec is not a valid name in %s",
diff --git a/fast-import.c b/fast-import.c
index 4646c05..74597c9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -642,8 +642,9 @@ static struct branch *new_branch(const char *name)
 	if (b)
 		die("Invalid attempt to create duplicate branch: %s", name);
 	switch (check_ref_format(name)) {
-	case  0: break; /* its valid */
-	case -2: break; /* valid, but too few '/', allow anyway */
+	case 0: break; /* its valid */
+	case CHECK_REF_FORMAT_ONELEVEL:
+		break; /* valid, but too few '/', allow anyway */
 	default:
 		die("Branch name doesn't conform to GIT standards: %s", name);
 	}
diff --git a/refs.c b/refs.c
index 7484a46..58f6d17 100644
--- a/refs.c
+++ b/refs.c
@@ -822,10 +822,10 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)
 struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)
 {
 	switch (check_ref_format(ref)) {
-	case CHECK_REF_FORMAT_ERROR:
-	case CHECK_REF_FORMAT_WILDCARD:
-		return NULL;
 	default:
+		return NULL;
+	case 0:
+	case CHECK_REF_FORMAT_ONELEVEL:
 		return lock_ref_sha1_basic(ref, old_sha1, flags, NULL);
 	}
 }

Re: input validation in receive-pack

[Martin Koegler]

On Tue, Jan 01, 2008 at 07:07:25PM -0800, Junio C Hamano wrote:
> 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

Re: input validation in receive-pack

[Junio C Hamano]

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

> Are there more refs outside "refs/", which somebody would want to push to?

As anything outside refs/ and not HEAD are subject to gc, I do
not think it should even be allowed.

But this is, as everybody should be aware, very late in 1.5.4
cycle, so I'd rather avoid bahviour change to tighten things
before post-1.5.4 opens.