git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 1/3] shorten_unambiguous_ref(): avoid integer truncation

From
Jeff King <peff@peff.net>
Date
Feb 14, 2023, 18:39 UTC
Message-ID
<Y+vVbdS79fZHRs6H@coredump.intra.peff.net>
In-Reply-To
<Y+vVFFCRem6t4IGM@coredump.intra.peff.net>

We parse the shortened name "foo" out of the full refname "refs/heads/foo", and then assign the result of strlen(short_name) to an int, which may truncate or wrap to negative.

In practice, this should never happen, as it requires a 2GB refname. And even somebody trying to do something malicious should at worst end up with a confused answer (we use the size only to feed back as a placeholder length to strbuf_addf() to see if there are any collisions in the lookup rules).

And it may even be impossible to trigger this, as we parse the string with sscanf(), and stdio formatting functions are not known for handling large strings well. I didn't test, but I wouldn't be surprised if sscanf() on many platforms simply reports no match here.

But even if it is not a problem in practice so far, it is worth fixing for two reasons:

  1. We'll shortly be replacing the sscanf() call with a real parser
     which will handle arbitrary-sized strings.
  2. Assigning strlen() to an int is an anti-pattern that requires
     people to look twice when auditing for real overflow problems.

So we'll make this a size_t. Unfortunately we still have to cast to int eventually for the strbuf_addf() call, but at least we can localize the cast there, and check that it will be valid. I used our new cast helper here, which will just bail completely. That should be OK, as anybody with a 2GB refname is up to no good, but if we really wanted to, we could detect it manually and just refuse to shorten the refname.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index e31dbcda59..94d938390d 100644
--- a/refs.c
+++ b/refs.c
@@ -1356,7 +1356,7 @@ char *refs_shorten_unambiguous_ref(struct ref_store *refs,
 	for (i = nr_rules - 1; i > 0 ; --i) {
 		int j;
 		int rules_to_fail = i;
-		int short_name_len;
+		size_t short_name_len;
 
 		if (1 != sscanf(refname, scanf_fmts[i], short_name))
 			continue;
@@ -1388,7 +1388,8 @@ char *refs_shorten_unambiguous_ref(struct ref_store *refs,
 			 */
 			strbuf_reset(&resolved_buf);
 			strbuf_addf(&resolved_buf, rule,
-				    short_name_len, short_name);
+				    cast_size_t_to_int(short_name_len),
+				    short_name);
 			if (refs_ref_exists(refs, resolved_buf.buf))
 				break;
 		}
-- 
2.39.1.849.g86e176252e
Previous: Jeff KingNext: Jeff King
Message 15 of 46 in “bug report: symbolic-ref --short command echos the wrong text while use Chinese language”
  1. 孟子易Feb 13, 2023
  2. Jeff KingFeb 13, 2023
  3. Eric SunshineFeb 13, 2023
  4. Jeff KingFeb 14, 2023
  5. Eric SunshineFeb 14, 2023
  6. Eric SunshineFeb 14, 2023
  7. Junio C HamanoFeb 14, 2023
  8. Eric SunshineFeb 14, 2023
  9. Junio C HamanoFeb 14, 2023
  10. Eric SunshineFeb 14, 2023
  11. Jeff KingFeb 14, 2023
  12. Eric SunshineFeb 14, 2023
  13. Jeff KingFeb 14, 2023
  14. 0/3 get rid of sscanf() when shortening refsJeff King, Feb 14, 2023
  15. 1/3 shorten_unambiguous_ref(): avoid integer truncationJeff King, Feb 14, 2023
  16. 2/3 shorten_unambiguous_ref(): use NUM_REV_PARSE_RULES constantJeff King, Feb 14, 2023
  17. Junio C HamanoFeb 14, 2023
  18. Jeff KingFeb 14, 2023
  19. 3/3 shorten_unambiguous_ref(): avoid sscanf()Jeff King, Feb 14, 2023
  20. Junio C HamanoFeb 14, 2023
  21. Junio C HamanoFeb 14, 2023
  22. Jeff KingFeb 14, 2023
  23. Junio C HamanoFeb 14, 2023
  24. Jeff KingFeb 14, 2023
  25. Junio C HamanoFeb 15, 2023
  26. Jeff KingFeb 15, 2023
  27. Junio C HamanoFeb 15, 2023
  28. Eric SunshineFeb 14, 2023
  29. 0/3 get rid of sscanf() when shortening refsJeff King, Feb 15, 2023
  30. 1/3 shorten_unambiguous_ref(): avoid integer truncationJeff King, Feb 15, 2023
  31. 2/3 shorten_unambiguous_ref(): use NUM_REV_PARSE_RULES constantJeff King, Feb 15, 2023
  32. 3/3 shorten_unambiguous_ref(): avoid sscanf()Jeff King, Feb 15, 2023
  33. Torsten BögershausenFeb 16, 2023
  34. Eric SunshineFeb 16, 2023
  35. Junio C HamanoFeb 16, 2023
  36. Jeff KingFeb 16, 2023
  37. Junio C HamanoFeb 16, 2023
  38. Jeff KingFeb 16, 2023
  39. Torsten BögershausenFeb 17, 2023
  40. Junio C HamanoFeb 15, 2023
  41. Junio C HamanoFeb 14, 2023
  42. Jeff KingFeb 14, 2023
  43. Torsten BögershausenFeb 15, 2023
  44. Eric SunshineFeb 15, 2023
  45. Torsten BögershausenFeb 15, 2023
  46. Eric SunshineFeb 16, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.