commit-graph: bounds-check generation overflow chunk

If the generation entry in a commit-graph doesn't fit, we instead insert an offset into a generation overflow chunk. But since we don't record the size of the chunk, we may read outside the chunk if the offset we find on disk is malicious or corrupted. We can't check the size of the chunk up-front; it will vary based on how many entries need overflow. So instead, we'll do a bounds-check before accessing the chunk memory. Unfortunately there is no error-return from this function, so we'll just have to die(), which is what it does for other forms of corruption. As with other cases, we can drop the st_mult() call, since we know our bounds-checked value will fit within a size_t. Before this patch, the test here actually "works" because we read garbage data from the next chunk. And since that garbage data happens not to provide a generation number which changes the output, it appears to work. We could construct a case that actually segfaults or produces wrong output, but it would be a bit tricky. For our purposes its sufficient to check that we've detected the bounds error. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 9, 2023 at 17:05 UTC ee6a7924124b2a9030da55c94a27829e410b3b64
3 files changed +18 -3
commit-graph.c
+7 -3
@@ -451,8 +451,9 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
451 if (s->commit_graph_generation_version >= 2) {
452 read_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,
453 graph_read_generation_data, graph);
454 - pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
455 - &graph->chunk_generation_data_overflow);
454 + pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,
455 + &graph->chunk_generation_data_overflow,
456 + &graph->chunk_generation_data_overflow_size);
457
458 if (graph->chunk_generation_data)
459 graph->read_generation_data = 1;
@@ -896,7 +897,10 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
897 die(_("commit-graph requires overflow generation data but has none"));
898
899 offset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;
899 - graph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + st_mult(8, offset_pos));
900 + if (g->chunk_generation_data_overflow_size / sizeof(uint64_t) <= offset_pos)
901 + die(_("commit-graph overflow generation data is too small"));
902 + graph_data->generation = item->date +
903 + get_be64(g->chunk_generation_data_overflow + sizeof(uint64_t) * offset_pos);
904 } else
905 graph_data->generation = item->date + offset;
906 } else
commit-graph.h
+1
@@ -94,6 +94,7 @@ struct commit_graph {
94 const unsigned char *chunk_commit_data;
95 const unsigned char *chunk_generation_data;
96 const unsigned char *chunk_generation_data_overflow;
97 + size_t chunk_generation_data_overflow_size;
98 const unsigned char *chunk_extra_edges;
99 size_t chunk_extra_edges_size;
100 const unsigned char *chunk_base_graphs;
t/t5328-commit-graph-64bit-time.sh
+10
@@ -10,6 +10,7 @@ then
10 fi
11
12 . "$TEST_DIRECTORY"/lib-commit-graph.sh
13 +. "$TEST_DIRECTORY/lib-chunk.sh"
14
15 UNIX_EPOCH_ZERO="@0 +0000"
16 FUTURE_DATE="@4147483646 +0000"
@@ -72,4 +73,13 @@ test_expect_success 'single commit with generation data exceeding UINT32_MAX' '
73 git -C repo-uint32-max commit-graph verify
74 '
75
76 +test_expect_success 'reader notices out-of-bounds generation overflow' '
77 + graph=.git/objects/info/commit-graph &&
78 + test_when_finished "rm -rf $graph" &&
79 + git commit-graph write --reachable &&
80 + corrupt_chunk_file $graph GDO2 clear &&
81 + test_must_fail git log 2>err &&
82 + grep "commit-graph overflow generation data is too small" err
83 +'
84 +
85 test_done