[RFC PATCH 0/6] Git 3.0: restrict hex object IDs to lowercase only

[RFC PATCH 0/6] Git 3.0: restrict hex object IDs to lowercase only

[brian m. carlson]

As far as I can tell, Git has always emitted hex object IDs in
lowercase, but our object ID parser accepts both uppercase and
lowercase.  This leads to much software relying on hex object IDs being
broken because it doesn't handle uppercase object IDs and this can even
lead to security problems when people assume that an object ID has a
unique hex form.

This series proposes to remove the ability to use uppercase hex in
object IDs in Git 3.0.  It is RFC simply because it's not clear if
there's the desire to do this, although the series should be fully
functional.

As further evidence of why we should do this, I'll note that there is
exactly one testcase in our testsuite that fails due to this change
(fixed in the last patch) and it's not clear that it fails
intentionally.  If we decide not to adopt this series, it would probably
be prudent to add some additional tests for the uppercase variant of hex
object IDs.

brian m. carlson (6):
  hex: add functionality for lowercase-only hex
  hex: allow specifying hex type with hex2chr
  hex: make hex_to_bytes accept kind of hex to use
  hex: label usages of hex parsing for object IDs
  object-name: use hexval
  hex: allow only lowercase object IDs in breaking changes mode

 Documentation/BreakingChanges.adoc |  5 ++++
 builtin/index-pack.c               |  2 +-
 color.c                            |  2 +-
 diagnose.c                         |  2 +-
 hex-ll.c                           | 39 ++++++++++++++++++++++++++++--
 hex-ll.h                           | 24 +++++++++++++-----
 hex.c                              |  2 +-
 http-push.c                        |  5 ++--
 mailinfo.c                         |  2 +-
 notes.c                            |  5 ++--
 object-file.c                      |  2 +-
 object-name.c                      | 13 +++-------
 pkt-line.c                         |  8 +++---
 ref-filter.c                       |  2 +-
 strbuf.c                           |  2 +-
 t/t1503-rev-parse-verify.sh        |  5 ++++
 t/t5324-split-commit-graph.sh      |  4 +--
 url.c                              |  2 +-
 urlmatch.c                         |  2 +-
 19 files changed, 90 insertions(+), 38 deletions(-)

[RFC PATCH 1/6] hex: add functionality for lowercase-only hex

[brian m. carlson]

We currently allow both upper and lower case for all hex values in Git.
However, in a future commit, we'll want to change that to allow only
lowercase values in some cases.  To prepare for that case, provide a
table to convert hex values using lowercase only and an enum to let us
choose which we want, wiring it up to the hexval function.

For now, keep things completely the same by specifying only the
variant that accepts both lowercase and uppercase to avoid changing
behavior.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 color.c    |  2 +-
 hex-ll.c   | 37 ++++++++++++++++++++++++++++++++++++-
 hex-ll.h   | 14 ++++++++++----
 pkt-line.c |  8 ++++----
 4 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/color.c b/color.c
index 00b53f97ac..9015d0faf1 100644
--- a/color.c
+++ b/color.c
@@ -72,7 +72,7 @@ static int get_hex_color(const char **inp, int width, unsigned char *out)
 	unsigned int val;
 
 	assert(width == 1 || width == 2);
-	val = (hexval(in[0]) << 4) | hexval(in[width - 1]);
+	val = (hexval(in[0], HEX_KIND_MIXED) << 4) | hexval(in[width - 1], HEX_KIND_MIXED);
 	if (val & ~0xff)
 		return -1;
 	*inp += width;
diff --git a/hex-ll.c b/hex-ll.c
index 4d7ece1de5..fa85e91827 100644
--- a/hex-ll.c
+++ b/hex-ll.c
@@ -36,10 +36,45 @@ const signed char hexval_table[256] = {
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
 };
 
+const signed char hexval_lc_table[256] = {
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 00-07 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 08-0f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 10-17 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 18-1f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 20-27 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 28-2f */
+	  0,  1,  2,  3,  4,  5,  6,  7,		/* 30-37 */
+	  8,  9, -1, -1, -1, -1, -1, -1,		/* 38-3f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 40-47 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 48-4f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 50-57 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 58-5f */
+	 -1, 10, 11, 12, 13, 14, 15, -1,		/* 60-67 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 68-67 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 70-77 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 78-7f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 80-87 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 88-8f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 90-97 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 98-9f */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* a0-a7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* a8-af */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* b0-b7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* b8-bf */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* c0-c7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* c8-cf */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* d0-d7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* d8-df */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* e0-e7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* e8-ef */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f0-f7 */
+	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
+};
+
 int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
 {
 	for (; len; len--, hex += 2) {
-		unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
+		unsigned int val = (hexval(hex[0], HEX_KIND_MIXED) << 4) | hexval(hex[1], HEX_KIND_MIXED);
 
 		if (val & ~0xff)
 			return -1;
diff --git a/hex-ll.h b/hex-ll.h
index a381fa8556..da1b5239b2 100644
--- a/hex-ll.h
+++ b/hex-ll.h
@@ -1,10 +1,16 @@
 #ifndef HEX_LL_H
 #define HEX_LL_H
 
+enum hexkind {
+	HEX_KIND_MIXED = 0,
+	HEX_KIND_LOWER = 1,
+};
+
 extern const signed char hexval_table[256];
-static inline unsigned int hexval(unsigned char c)
+extern const signed char hexval_lc_table[256];
+static inline unsigned int hexval(unsigned char c, enum hexkind kind)
 {
-	return hexval_table[c];
+	return kind == HEX_KIND_MIXED ? hexval_table[c] : hexval_lc_table[c];
 }
 
 /*
@@ -13,8 +19,8 @@ static inline unsigned int hexval(unsigned char c)
  */
 static inline int hex2chr(const char *s)
 {
-	unsigned int val = hexval(s[0]);
-	return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
+	unsigned int val = hexval(s[0], HEX_KIND_MIXED);
+	return (val & ~0xf) ? val : (val << 4) | hexval(s[1], HEX_KIND_MIXED);
 }
 
 /*
diff --git a/pkt-line.c b/pkt-line.c
index 3fc3e9ea70..338075558c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -378,10 +378,10 @@ int packet_length(const char lenbuf_hex[4], size_t size)
 {
 	if (size < 4)
 		BUG("buffer too small");
-	return	hexval(lenbuf_hex[0]) << 12 |
-		hexval(lenbuf_hex[1]) <<  8 |
-		hexval(lenbuf_hex[2]) <<  4 |
-		hexval(lenbuf_hex[3]);
+	return	hexval(lenbuf_hex[0], HEX_KIND_MIXED) << 12 |
+		hexval(lenbuf_hex[1], HEX_KIND_MIXED) <<  8 |
+		hexval(lenbuf_hex[2], HEX_KIND_MIXED) <<  4 |
+		hexval(lenbuf_hex[3], HEX_KIND_MIXED);
 }
 
 static const char *find_packfile_uri_path(const char *buffer)

[RFC PATCH 2/6] hex: allow specifying hex type with hex2chr

[brian m. carlson]

We have several places where we use hex2chr.  One of those is parsing
object IDs, but others decode quoted-printable or percent encoding.  All
of them accept both uppercase and lowercase hex.

In a future commit, we'll change some of these cases, so make hex2chr
accept the kind of encoding to use: lowercase only hex or any kind of
hex.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 hex-ll.h     | 6 +++---
 hex.c        | 2 +-
 mailinfo.c   | 2 +-
 ref-filter.c | 2 +-
 strbuf.c     | 2 +-
 url.c        | 2 +-
 urlmatch.c   | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hex-ll.h b/hex-ll.h
index da1b5239b2..26847c7b2f 100644
--- a/hex-ll.h
+++ b/hex-ll.h
@@ -17,10 +17,10 @@ static inline unsigned int hexval(unsigned char c, enum hexkind kind)
  * Convert two consecutive hexadecimal digits into a char.  Return a
  * negative value on error.  Don't run over the end of short strings.
  */
-static inline int hex2chr(const char *s)
+static inline int hex2chr(const char *s, enum hexkind kind)
 {
-	unsigned int val = hexval(s[0], HEX_KIND_MIXED);
-	return (val & ~0xf) ? val : (val << 4) | hexval(s[1], HEX_KIND_MIXED);
+	unsigned int val = hexval(s[0], kind);
+	return (val & ~0xf) ? val : (val << 4) | hexval(s[1], kind);
 }
 
 /*
diff --git a/hex.c b/hex.c
index f02832140d..6150bdcbf8 100644
--- a/hex.c
+++ b/hex.c
@@ -9,7 +9,7 @@ static int get_hash_hex_algop(const char *hex, unsigned char *hash,
 			      const struct git_hash_algo *algop)
 {
 	for (size_t i = 0; i < algop->rawsz; i++) {
-		int val = hex2chr(hex);
+		int val = hex2chr(hex, HEX_KIND_MIXED);
 		if (val < 0)
 			return -1;
 		*hash++ = val;
diff --git a/mailinfo.c b/mailinfo.c
index 13949ff31e..85c3119048 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -396,7 +396,7 @@ static int decode_q_segment(struct strbuf *out, const struct strbuf *q_seg,
 			int ch, d = *in;
 			if (d == '\n' || !d)
 				break; /* drop trailing newline */
-			ch = hex2chr(in);
+			ch = hex2chr(in, HEX_KIND_MIXED);
 			if (ch >= 0) {
 				strbuf_addch(out, ch);
 				in += 2;
diff --git a/ref-filter.c b/ref-filter.c
index 29aca08ce7..884bcd8fc5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3567,7 +3567,7 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 			if (cp[1] == '%')
 				cp++;
 			else {
-				int ch = hex2chr(cp + 1);
+				int ch = hex2chr(cp + 1, HEX_KIND_MIXED);
 				if (0 <= ch) {
 					strbuf_addch(s, ch);
 					cp += 3;
diff --git a/strbuf.c b/strbuf.c
index 44955669e8..88d23f8ac5 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -457,7 +457,7 @@ size_t strbuf_expand_literal(struct strbuf *sb, const char *placeholder)
 		return 1;
 	case 'x':
 		/* %x00 == NUL, %x0a == LF, etc. */
-		ch = hex2chr(placeholder + 1);
+		ch = hex2chr(placeholder + 1, HEX_KIND_MIXED);
 		if (ch < 0)
 			return 0;
 		strbuf_addch(sb, ch);
diff --git a/url.c b/url.c
index a59818278f..b4d72f784a 100644
--- a/url.c
+++ b/url.c
@@ -62,7 +62,7 @@ static char *url_decode_internal(const char **query, int len,
 		}
 
 		if (c == '%' && (len < 0 || len >= 3)) {
-			int val = hex2chr(q + 1);
+			int val = hex2chr(q + 1, HEX_KIND_MIXED);
 			if (0 < val) {
 				strbuf_addch(out, val);
 				q += 3;
diff --git a/urlmatch.c b/urlmatch.c
index 20bc2d009c..989f1d794b 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -50,7 +50,7 @@ static int append_normalized_escapes(struct strbuf *buf,
 		if (ch == '%') {
 			if (from_len < 2)
 				return 0;
-			ch = hex2chr(from);
+			ch = hex2chr(from, HEX_KIND_MIXED);
 			if (ch < 0)
 				return 0;
 			from += 2;

[RFC PATCH 3/6] hex: make hex_to_bytes accept kind of hex to use

[brian m. carlson]

Similarly to the previous commit, introduce an option for hex_to_bytes
to allow us to specify the kind of hex to use: lowercase only or not.
For now, everything remains the same as before, but we will change
things in a future commit.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/index-pack.c | 2 +-
 diagnose.c           | 2 +-
 hex-ll.c             | 4 ++--
 hex-ll.h             | 2 +-
 http-push.c          | 5 +++--
 notes.c              | 5 +++--
 object-file.c        | 2 +-
 7 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index bc86925ad0..9660e5967b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1866,7 +1866,7 @@ static void repack_local_links(void)
 	while (strbuf_getline_lf(&line, out) != EOF) {
 		unsigned char binary[GIT_MAX_RAWSZ];
 		if (line.len != the_hash_algo->hexsz ||
-		    !hex_to_bytes(binary, line.buf, line.len))
+		    !hex_to_bytes(binary, line.buf, line.len, HEX_KIND_MIXED))
 			die(_("index-pack: Expecting full hex object ID lines only from pack-objects."));
 
 		/*
diff --git a/diagnose.c b/diagnose.c
index 5092bf80d3..fc11cea229 100644
--- a/diagnose.c
+++ b/diagnose.c
@@ -112,7 +112,7 @@ static void loose_objs_stats(struct strbuf *buf, const char *path)
 	while ((e = readdir_skip_dot_and_dotdot(dir)) != NULL)
 		if (get_dtype(e, &count_path, 0) == DT_DIR &&
 		    strlen(e->d_name) == 2 &&
-		    !hex_to_bytes(&c, e->d_name, 1)) {
+		    !hex_to_bytes(&c, e->d_name, 1, HEX_KIND_MIXED)) {
 			strbuf_setlen(&count_path, base_path_len);
 			strbuf_addf(&count_path, "%s/", e->d_name);
 			total += (count = count_files(&count_path));
diff --git a/hex-ll.c b/hex-ll.c
index fa85e91827..b2e9684693 100644
--- a/hex-ll.c
+++ b/hex-ll.c
@@ -71,10 +71,10 @@ const signed char hexval_lc_table[256] = {
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
 };
 
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len, enum hexkind kind)
 {
 	for (; len; len--, hex += 2) {
-		unsigned int val = (hexval(hex[0], HEX_KIND_MIXED) << 4) | hexval(hex[1], HEX_KIND_MIXED);
+		unsigned int val = (hexval(hex[0], kind) << 4) | hexval(hex[1], kind);
 
 		if (val & ~0xff)
 			return -1;
diff --git a/hex-ll.h b/hex-ll.h
index 26847c7b2f..fe698f0c76 100644
--- a/hex-ll.h
+++ b/hex-ll.h
@@ -28,6 +28,6 @@ static inline int hex2chr(const char *s, enum hexkind kind)
  * values to `binary` as `len` bytes. Return 0 on success, or -1 if
  * the input does not consist of hex digits).
  */
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len, enum hexkind kind);
 
 #endif
diff --git a/http-push.c b/http-push.c
index 94a1fac9ab..0cc990d395 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1030,12 +1030,13 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 	if (strlen(path) != the_hash_algo->hexsz + 1)
 		return -1;
 
-	if (hex_to_bytes(oid->hash, path, 1))
+	if (hex_to_bytes(oid->hash, path, 1, HEX_KIND_MIXED))
 		return -1;
 	path += 2;
 	path++; /* skip '/' */
 
-	return hex_to_bytes(oid->hash + 1, path, the_hash_algo->rawsz - 1);
+	return hex_to_bytes(oid->hash + 1, path, the_hash_algo->rawsz - 1,
+			    HEX_KIND_MIXED);
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)
diff --git a/notes.c b/notes.c
index ec9c2cb150..99b8b15d81 100644
--- a/notes.c
+++ b/notes.c
@@ -428,7 +428,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 				goto handle_non_note;
 
 			if (hex_to_bytes(object_oid.hash + prefix_len, entry.path,
-					 hashsz - prefix_len))
+					 hashsz - prefix_len, HEX_KIND_MIXED))
 				goto handle_non_note; /* entry.path is not a SHA1 */
 
 			memset(object_oid.hash + hashsz, 0, GIT_MAX_RAWSZ - hashsz);
@@ -442,7 +442,8 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 				/* internal nodes must be trees */
 				goto handle_non_note;
 
-			if (hex_to_bytes(object_oid.hash + len++, entry.path, 1))
+			if (hex_to_bytes(object_oid.hash + len++, entry.path, 1,
+					 HEX_KIND_MIXED))
 				goto handle_non_note; /* entry.path is not a SHA1 */
 
 			/*
diff --git a/object-file.c b/object-file.c
index 7ff2b730ac..8427b2802a 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1473,7 +1473,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		strbuf_add(path, de->d_name, namelen);
 		if (namelen == algop->hexsz - 2 &&
 		    !hex_to_bytes(oid.hash + 1, de->d_name,
-				  algop->rawsz - 1)) {
+				  algop->rawsz - 1, HEX_KIND_MIXED)) {
 			oid_set_algo(&oid, algop);
 			memset(oid.hash + algop->rawsz, 0,
 			       GIT_MAX_RAWSZ - algop->rawsz);

[RFC PATCH 4/6] hex: label usages of hex parsing for object IDs

[brian m. carlson]

In preparation for a future change, label the hex parsing we're doing
for object IDs by defining a constant called HEX_KIND_OID.  This is
currently the same as HEX_KIND_MIXED, so there is no functional change
here.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 diagnose.c    | 2 +-
 hex-ll.h      | 2 ++
 hex.c         | 2 +-
 http-push.c   | 4 ++--
 notes.c       | 2 +-
 object-file.c | 2 +-
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/diagnose.c b/diagnose.c
index fc11cea229..9c652d36a6 100644
--- a/diagnose.c
+++ b/diagnose.c
@@ -112,7 +112,7 @@ static void loose_objs_stats(struct strbuf *buf, const char *path)
 	while ((e = readdir_skip_dot_and_dotdot(dir)) != NULL)
 		if (get_dtype(e, &count_path, 0) == DT_DIR &&
 		    strlen(e->d_name) == 2 &&
-		    !hex_to_bytes(&c, e->d_name, 1, HEX_KIND_MIXED)) {
+		    !hex_to_bytes(&c, e->d_name, 1, HEX_KIND_OID)) {
 			strbuf_setlen(&count_path, base_path_len);
 			strbuf_addf(&count_path, "%s/", e->d_name);
 			total += (count = count_files(&count_path));
diff --git a/hex-ll.h b/hex-ll.h
index fe698f0c76..9da76f17e8 100644
--- a/hex-ll.h
+++ b/hex-ll.h
@@ -6,6 +6,8 @@ enum hexkind {
 	HEX_KIND_LOWER = 1,
 };
 
+#define HEX_KIND_OID HEX_KIND_MIXED
+
 extern const signed char hexval_table[256];
 extern const signed char hexval_lc_table[256];
 static inline unsigned int hexval(unsigned char c, enum hexkind kind)
diff --git a/hex.c b/hex.c
index 6150bdcbf8..4e1e81af3f 100644
--- a/hex.c
+++ b/hex.c
@@ -9,7 +9,7 @@ static int get_hash_hex_algop(const char *hex, unsigned char *hash,
 			      const struct git_hash_algo *algop)
 {
 	for (size_t i = 0; i < algop->rawsz; i++) {
-		int val = hex2chr(hex, HEX_KIND_MIXED);
+		int val = hex2chr(hex, HEX_KIND_OID);
 		if (val < 0)
 			return -1;
 		*hash++ = val;
diff --git a/http-push.c b/http-push.c
index 0cc990d395..132d26d6a1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1030,13 +1030,13 @@ static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 	if (strlen(path) != the_hash_algo->hexsz + 1)
 		return -1;
 
-	if (hex_to_bytes(oid->hash, path, 1, HEX_KIND_MIXED))
+	if (hex_to_bytes(oid->hash, path, 1, HEX_KIND_OID))
 		return -1;
 	path += 2;
 	path++; /* skip '/' */
 
 	return hex_to_bytes(oid->hash + 1, path, the_hash_algo->rawsz - 1,
-			    HEX_KIND_MIXED);
+			    HEX_KIND_OID);
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)
diff --git a/notes.c b/notes.c
index 99b8b15d81..7e9e3eb2d2 100644
--- a/notes.c
+++ b/notes.c
@@ -443,7 +443,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 				goto handle_non_note;
 
 			if (hex_to_bytes(object_oid.hash + len++, entry.path, 1,
-					 HEX_KIND_MIXED))
+					 HEX_KIND_OID))
 				goto handle_non_note; /* entry.path is not a SHA1 */
 
 			/*
diff --git a/object-file.c b/object-file.c
index 8427b2802a..4bcff66442 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1473,7 +1473,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		strbuf_add(path, de->d_name, namelen);
 		if (namelen == algop->hexsz - 2 &&
 		    !hex_to_bytes(oid.hash + 1, de->d_name,
-				  algop->rawsz - 1, HEX_KIND_MIXED)) {
+				  algop->rawsz - 1, HEX_KIND_OID)) {
 			oid_set_algo(&oid, algop);
 			memset(oid.hash + algop->rawsz, 0,
 			       GIT_MAX_RAWSZ - algop->rawsz);

[RFC PATCH 5/6] object-name: use hexval

[brian m. carlson]

We've open-coded a different implementation of parsing hex values here
when we already have a perfectly good one in hexval.  This
implementation will almost certainly be slower because it isn't
table-driven, unlike the other one, and since it's not constant time it
has no other advantages either.  To tidy things up and prepare for
future work, switch to hexval in this case.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 object-name.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/object-name.c b/object-name.c
index 83efba0ba6..d2d81b3511 100644
--- a/object-name.c
+++ b/object-name.c
@@ -236,17 +236,10 @@ static int parse_oid_prefix(const char *name, int len,
 {
 	for (int i = 0; i < len; i++) {
 		unsigned char c = name[i];
-		unsigned char val;
-		if (c >= '0' && c <= '9') {
-			val = c - '0';
-		} else if (c >= 'a' && c <= 'f') {
-			val = c - 'a' + 10;
-		} else if (c >= 'A' && c <='F') {
-			val = c - 'A' + 10;
-			c -= 'A' - 'a';
-		} else {
+		int val = hexval(c, HEX_KIND_OID);
+
+		if (val < 0)
 			return -1;
-		}
 
 		if (hex_out)
 			hex_out[i] = c;

[RFC PATCH 6/6] hex: allow only lowercase object IDs in breaking changes mode

[brian m. carlson]

Git has historically allowed either lowercase or uppercase hex for
object IDs, but it has always emitted only lowercase.  This has caused
people to expect only lowercase and not handle uppercase.

As an example, Git's own example hooks look for "[0-9a-f]" in several
places, but there are many other Git-adjacent pieces of software,
including Gitolite, which make the assumption that object IDs are always
lowercase.  This is not to criticize the authors of these projects, but
rather to point out how common this assumption is.  In fact, it's so
common that we have only one test in our codebase that fails when we
reject uppercase object IDs.

More critically, it leads people to make security-based assumptions that
an object ID either does not contain uppercase characters or that an
object ID can be expressed uniquely in hex form, neither of which are
currently true.  Git itself normally uses binary object IDs, which
avoids many of these problems, but most other projects deal primarily in
hex object IDs, so they are more affected.

In preparation for Git 3.0, only allow lowercase hex object IDs in
breaking changes mode and document this as well.  Update the single
failing test and add a new one to verify we reject new uppercase object
IDs.  Note that in t5324, we change the hex character from "A" to "b"
because in SHA-256 mode, "a" is the correct value, so our test_must_fail
assertion will unexpectedly succeed in that case.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/BreakingChanges.adoc | 5 +++++
 hex-ll.h                           | 4 ++++
 t/t1503-rev-parse-verify.sh        | 5 +++++
 t/t5324-split-commit-graph.sh      | 4 ++--
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index 73bb939359..dbc46d14e3 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -171,6 +171,11 @@ JGit, libgit2 and Gitoxide need to support it.
   matches the default branch name used in new repositories by many of the
   big Git forges.
 
+* Git will accept hex object IDs only in lowercase. The fact that Git has
+	historically allowed uppercase characters in hex object IDs has been the
+	source of a variety of bugs and security problems in software using Git. We
+	don't expect most users to notice any change.
+
 * Git will require Rust as a mandatory part of the build process. While Git
   already started to adopt Rust in Git 2.49, all parts written in Rust are
   optional for the time being. This includes:
diff --git a/hex-ll.h b/hex-ll.h
index 9da76f17e8..2f9c8d7c25 100644
--- a/hex-ll.h
+++ b/hex-ll.h
@@ -6,7 +6,11 @@ enum hexkind {
 	HEX_KIND_LOWER = 1,
 };
 
+#ifdef WITH_BREAKING_CHANGES
+#define HEX_KIND_OID HEX_KIND_LOWER
+#else
 #define HEX_KIND_OID HEX_KIND_MIXED
+#endif
 
 extern const signed char hexval_table[256];
 extern const signed char hexval_lc_table[256];
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 87638a4a2c..f07b45de5a 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -60,6 +60,11 @@ test_expect_success 'works with one good rev' '
 	test "$rev_head" = "$HASH4"
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'rejects uppercase revs' '
+	UC_HASH=$(echo "$HASH1" | tr a-f A-F) &&
+	test_must_fail git rev-parse --verify "$UC_HASH"
+'
+
 test_expect_success 'fails with any bad rev or many good revs' '
 	test_must_fail git rev-parse --verify 2>error &&
 	test_grep "single revision" error &&
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index bf7ba0e558..29db815c77 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -349,7 +349,7 @@ test_expect_success 'verify after commit-graph-chain corruption (base)' '
 		test_must_fail git commit-graph verify 2>test_err &&
 		grep -v "^+" test_err >err &&
 		test_grep "invalid commit-graph chain" err &&
-		corrupt_file "$graphdir/commit-graph-chain" 30 "A" &&
+		corrupt_file "$graphdir/commit-graph-chain" 30 "a" &&
 		test_must_fail git commit-graph verify 2>test_err &&
 		grep -v "^+" test_err >err &&
 		test_grep "unable to find all commit-graph files" err
@@ -364,7 +364,7 @@ test_expect_success 'verify after commit-graph-chain corruption (tip)' '
 		test_must_fail git commit-graph verify 2>test_err &&
 		grep -v "^+" test_err >err &&
 		test_grep "invalid commit-graph chain" err &&
-		corrupt_file "$graphdir/commit-graph-chain" 70 "A" &&
+		corrupt_file "$graphdir/commit-graph-chain" 70 "b" &&
 		test_must_fail git commit-graph verify 2>test_err &&
 		grep -v "^+" test_err >err &&
 		test_grep "unable to find all commit-graph files" err

Re: [RFC PATCH 0/6] Git 3.0: restrict hex object IDs to lowercase only

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> As far as I can tell, Git has always emitted hex object IDs in
> lowercase, but our object ID parser accepts both uppercase and
> lowercase.  This leads to much software relying on hex object IDs being
> broken because it doesn't handle uppercase object IDs and this can even
> lead to security problems when people assume that an object ID has a
> unique hex form.
>
> This series proposes to remove the ability to use uppercase hex in
> object IDs in Git 3.0.  It is RFC simply because it's not clear if
> there's the desire to do this, although the series should be fully
> functional.
>
> As further evidence of why we should do this, I'll note that there is
> exactly one testcase in our testsuite that fails due to this change
> (fixed in the last patch) and it's not clear that it fails
> intentionally.  If we decide not to adopt this series, it would probably
> be prudent to add some additional tests for the uppercase variant of hex
> object IDs.

Before going there, we should hear a solid argument why doing this
might be beneficial longer term.  "Just because we might be able to
without harming too many users" is probably not good enough, when it
is not accompanied by "... the (low) risk may be worth taking because
we will gain such and such benefit".