Re: input validation in receive-pack

Subsystems: the rest

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: input validation in receive-pack

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:02

Daniel Barkalow [off-list ref] writes:

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

quoted

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.

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 <redacted>
---
 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

From: Daniel Barkalow <hidden>
Date: 2016-06-15 22:44:02

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

quoted hunk

@@ -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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:02

Daniel Barkalow [off-list ref] 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 <redacted>
---
 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);
 	}
 }

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help