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

[PATCH 01/20] chunk-format: note that pair_chunk() is unsafe

From
Jeff King <peff@peff.net>
Date
Oct 9, 2023, 20:58 UTC
Message-ID
<20231009205823.GA3282181@coredump.intra.peff.net>
In-Reply-To
<20231009205544.GA3281950@coredump.intra.peff.net>

The pair_chunk() function is provided as an easy helper for parsing chunks that just want a pointer to a set of bytes. But every caller has a hidden bug: because we return only the pointer without the matching chunk size, the callers have no clue how many bytes they are allowed to look at. And as a result, they may read off the end of the mmap'd data when the on-disk file does not match their expectations.

Since chunk files are typically used for local-repository data like commit-graph files and midx's, the security implications here are pretty mild. The worst that can happen is that you hand somebody a corrupted repository tarball, and running Git on it does an out-of-bounds read and crashes. So it's worth being more defensive, but we don't need to drop everything and fix every caller immediately.

I noticed the problem because the pair_chunk_fn() callback does not look at its chunk_size argument, and wanted to annotate it to silence -Wunused-parameter. We could do that now, but we'd lose the hint that this code should be audited and fixed.

So instead, let's set ourselves up for going down that path:
  1. Provide a pair_chunk() function that does return the size, which
     prepares us for fixing these cases.
  2. Rename the existing function to pair_chunk_unsafe(). That gives us
     an easy way to grep for cases which still need to be fixed, and the
     name should cause anybody adding new calls to think twice before
     using it.

There are no callers of the "safe" version yet, but we'll add some in subsequent patches.

Signed-off-by: Jeff King <peff@peff.net>
---
I originally wasn't sure if I'd convert every case, or if we'd have to
leave some longer-term. But as you'll see at the end, we can get rid of
pair_chunk_unsafe() in the final patch (but we need both forms to
co-exist during the series, since the intermediate state is that we have
some safe and some unsafe calls).
 chunk-format.c | 24 ++++++++++++++++++++----
 chunk-format.h | 19 +++++++++++++++++--
 commit-graph.c | 14 +++++++-------
 midx.c         | 10 +++++-----
 4 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/chunk-format.c b/chunk-format.c
index 140dfa0dcc..8d910e23f6 100644
--- a/chunk-format.c
+++ b/chunk-format.c
@@ -154,20 +154,36 @@ int read_table_of_contents(struct chunkfile *cf,
 	return 0;
 }
 
+struct pair_chunk_data {
+	const unsigned char **p;
+	size_t *size;
+};
+
 static int pair_chunk_fn(const unsigned char *chunk_start,
 			 size_t chunk_size,
 			 void *data)
 {
-	const unsigned char **p = data;
-	*p = chunk_start;
+	struct pair_chunk_data *pcd = data;
+	*pcd->p = chunk_start;
+	*pcd->size = chunk_size;
 	return 0;
 }
 
 int pair_chunk(struct chunkfile *cf,
 	       uint32_t chunk_id,
-	       const unsigned char **p)
+	       const unsigned char **p,
+	       size_t *size)
+{
+	struct pair_chunk_data pcd = { .p = p, .size = size };
+	return read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);
+}
+
+int pair_chunk_unsafe(struct chunkfile *cf,
+		      uint32_t chunk_id,
+		      const unsigned char **p)
 {
-	return read_chunk(cf, chunk_id, pair_chunk_fn, p);
+	size_t dummy;
+	return pair_chunk(cf, chunk_id, p, &dummy);
 }
 
 int read_chunk(struct chunkfile *cf,
diff --git a/chunk-format.h b/chunk-format.h
index c7794e84ad..8dce7967f4 100644
--- a/chunk-format.h
+++ b/chunk-format.h
@@ -43,13 +43,28 @@ int read_table_of_contents(struct chunkfile *cf,
 /*
  * Find 'chunk_id' in the given chunkfile and assign the
  * given pointer to the position in the mmap'd file where
- * that chunk begins.
+ * that chunk begins. Likewise the "size" parameter is filled
+ * with the size of the chunk.
  *
  * Returns CHUNK_NOT_FOUND if the chunk does not exist.
  */
 int pair_chunk(struct chunkfile *cf,
 	       uint32_t chunk_id,
-	       const unsigned char **p);
+	       const unsigned char **p,
+	       size_t *size);
+
+/*
+ * Unsafe version of pair_chunk; it does not return the size,
+ * meaning that the caller cannot possibly be careful about
+ * reading out of bounds from the mapped memory.
+ *
+ * No new callers should use this function, and old callers should
+ * be audited and migrated over to using the regular pair_chunk()
+ * function.
+ */
+int pair_chunk_unsafe(struct chunkfile *cf,
+		      uint32_t chunk_id,
+		      const unsigned char **p);
 
 typedef int (*chunk_read_fn)(const unsigned char *chunk_start,
 			     size_t chunk_size, void *data);
diff --git a/commit-graph.c b/commit-graph.c
index 1a56efcf69..a689a55b79 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -394,25 +394,25 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
 				   GRAPH_HEADER_SIZE, graph->num_chunks))
 		goto free_and_return;
 
-	pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,
+	pair_chunk_unsafe(cf, GRAPH_CHUNKID_OIDFANOUT,
 		   (const unsigned char **)&graph->chunk_oid_fanout);
 	read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);
-	pair_chunk(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);
-	pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);
-	pair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);
+	pair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);
+	pair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);
+	pair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);
 
 	if (s->commit_graph_generation_version >= 2) {
-		pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,
+		pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,
 			&graph->chunk_generation_data);
-		pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
+		pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
 			&graph->chunk_generation_data_overflow);
 
 		if (graph->chunk_generation_data)
 			graph->read_generation_data = 1;
 	}
 
 	if (s->commit_graph_read_changed_paths) {
-		pair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,
+		pair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,
 			   &graph->chunk_bloom_indexes);
 		read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
 			   graph_read_bloom_data, graph);
diff --git a/midx.c b/midx.c
index 931f557350..3165218ab5 100644
--- a/midx.c
+++ b/midx.c
@@ -143,19 +143,19 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 				   MIDX_HEADER_SIZE, m->num_chunks))
 		goto cleanup_fail;
 
-	if (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)
+	if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)
 		die(_("multi-pack-index missing required pack-name chunk"));
 	if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)
 		die(_("multi-pack-index missing required OID fanout chunk"));
-	if (pair_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)
+	if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)
 		die(_("multi-pack-index missing required OID lookup chunk"));
-	if (pair_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)
+	if (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)
 		die(_("multi-pack-index missing required object offsets chunk"));
 
-	pair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);
+	pair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);
 
 	if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1))
-		pair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);
+		pair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);
 
 	m->num_objects = ntohl(m->chunk_oid_fanout[255]);
 

-- 
2.42.0.884.g35e1fe1a6a
Previous: Jeff KingNext: Taylor Blau
Message 2 of 67 in “bounds-checks for chunk-based files”
  1. 0/20 bounds-checks for chunk-based filesJeff King, Oct 9, 2023
  2. 01/20 chunk-format: note that pair_chunk() is unsafeJeff King, Oct 9, 2023
  3. Taylor BlauOct 10, 2023
  4. Jeff KingOct 11, 2023
  5. 02/20 t: add library for munging chunk-format filesJeff King, Oct 9, 2023
  6. Taylor BlauOct 10, 2023
  7. 03/20 midx: stop ignoring malformed oid fanout chunkJeff King, Oct 9, 2023
  8. Taylor BlauOct 10, 2023
  9. Jeff KingOct 11, 2023
  10. 04/20 commit-graph: check size of oid fanout chunkJeff King, Oct 9, 2023
  11. Taylor BlauOct 11, 2023
  12. Taylor BlauOct 11, 2023
  13. Jeff KingOct 11, 2023
  14. 05/20 midx: check size of oid lookup chunkJeff King, Oct 9, 2023
  15. 06/20 commit-graph: check consistency of fanout tableJeff King, Oct 9, 2023
  16. Taylor BlauOct 11, 2023
  17. 07/20 midx: check size of pack names chunkJeff King, Oct 9, 2023
  18. Taylor BlauOct 11, 2023
  19. Jeff KingOct 11, 2023
  20. 08/20 midx: enforce chunk alignment on readingJeff King, Oct 9, 2023
  21. Taylor BlauOct 11, 2023
  22. Taylor BlauOct 11, 2023
  23. Jeff KingOct 11, 2023
  24. 09/20 midx: check size of object offset chunkJeff King, Oct 9, 2023
  25. Taylor BlauOct 11, 2023
  26. 10/20 midx: bounds-check large offset chunkJeff King, Oct 9, 2023
  27. Taylor BlauOct 11, 2023
  28. Jeff KingOct 11, 2023
  29. 11/20 midx: check size of revindex chunkJeff King, Oct 9, 2023
  30. Taylor BlauOct 11, 2023
  31. 12/20 commit-graph: check size of commit data chunkJeff King, Oct 9, 2023
  32. Taylor BlauOct 11, 2023
  33. Jeff KingOct 11, 2023
  34. 13/20 commit-graph: detect out-of-bounds extra-edges pointersJeff King, Oct 9, 2023
  35. Taylor BlauOct 11, 2023
  36. 14/20 commit-graph: bounds-check base graphs chunkJeff King, Oct 9, 2023
  37. Taylor BlauOct 11, 2023
  38. 15/20 commit-graph: check size of generations chunkJeff King, Oct 9, 2023
  39. 16/20 commit-graph: bounds-check generation overflow chunkJeff King, Oct 9, 2023
  40. 17/20 commit-graph: check bounds when accessing BDAT chunkJeff King, Oct 9, 2023
  41. Taylor BlauOct 11, 2023
  42. Jeff KingOct 11, 2023
  43. 18/20 commit-graph: check bounds when accessing BIDX chunkJeff King, Oct 9, 2023
  44. Taylor BlauOct 11, 2023
  45. 19/20 commit-graph: detect out-of-order BIDX offsetsJeff King, Oct 9, 2023
  46. Taylor BlauOct 11, 2023
  47. 20/20 chunk-format: drop pair_chunk_unsafe()Jeff King, Oct 9, 2023
  48. Taylor BlauOct 11, 2023
  49. Jeff KingOct 11, 2023
  50. 0/8 chunk-format: introduce `pair_chunk_expect()` convenience APITaylor Blau, Oct 13, 2023
  51. 1/8 chunk-format: introduce `pair_chunk_expect()` helperTaylor Blau, Oct 13, 2023
  52. 2/8 commit-graph: read `OIDF` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  53. 3/8 commit-graph: read `CDAT` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  54. 4/8 commit-graph: read `GDAT` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  55. 5/8 commit-graph: read `BIDX` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  56. Taylor BlauOct 13, 2023
  57. Junio C HamanoOct 14, 2023
  58. Jeff KingOct 20, 2023
  59. 6/8 midx: read `OIDF` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  60. Junio C HamanoOct 13, 2023
  61. 7/8 midx: read `OIDL` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  62. 8/8 midx: read `OOFF` chunk with `pair_chunk_expect()`Taylor Blau, Oct 13, 2023
  63. Jeff KingOct 20, 2023
  64. 21/20 t5319: make corrupted large-offset test more robustJeff King, Oct 14, 2023
  65. Junio C HamanoOct 14, 2023
  66. Jeff KingOct 15, 2023
  67. Junio C HamanoOct 15, 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.