@matheusbsbs / matheus-bagliano / commits / 581e0f8b18

commit-graph: check bounds when accessing BIDX chunk

We load the bloom_filter_indexes chunk using pair_chunk(), so we have no idea how big it is. This can lead to out-of-bounds reads if it is smaller than expected, since we index it based on the number of commits found elsewhere in the graph file. We can check the chunk size up front, like we do for CDAT and other chunks with one fixed-size record per commit. The test case demonstrates the problem. It actually won't segfault, because we end up reading random data from the follow-on chunk (BDAT in this case), and the bounds checks added in the previous patch complain. But this is by no means assured, and you can craft a commit-graph file with BIDX at the end (or a smaller BDAT) that does segfault. 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 581e0f8b189b5b3d50aae16be09941ab6fccf335

2 files changed +23 -2

commit-graph.c

+14 -2

@@ -360,6 +360,18 @@ static int graph_read_generation_data(const unsigned char *chunk_start,

360 return 0;

361 }

362

363 +static int graph_read_bloom_index(const unsigned char *chunk_start,

364 + size_t chunk_size, void *data)

365 +{

366 + struct commit_graph *g = data;

367 + if (chunk_size != g->num_commits * 4) {

368 + warning("commit-graph changed-path index chunk is too small");

369 + return -1;

370 + }

371 + g->chunk_bloom_indexes = chunk_start;

372 + return 0;

373 +}

374 +

375 static int graph_read_bloom_data(const unsigned char *chunk_start,

376 size_t chunk_size, void *data)

377 {

@@ -470,8 +482,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,

482 }

483

484 if (s->commit_graph_read_changed_paths) {

473 - pair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,

474 - &graph->chunk_bloom_indexes);

485 + read_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,

486 + graph_read_bloom_index, graph);

487 read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,

488 graph_read_bloom_data, graph);

489 }

t/t4216-log-bloom.sh

@@ -432,4 +432,13 @@ test_expect_success 'Bloom reader notices out-of-bounds filter offsets' '

432 grep "warning: ignoring out-of-range offset (4294967295) for changed-path filter at pos 3 of .git/objects/info/commit-graph" err

433 '

434

435 +test_expect_success 'Bloom reader notices too-small index chunk' '

436 + # replace the index with a single entry, making most

437 + # lookups out-of-bounds

438 + check_corrupt_graph BIDX clear 00000000 &&

439 + echo "warning: commit-graph changed-path index chunk" \

440 + "is too small" >expect.err &&

441 + test_cmp expect.err err

442 +'

443 +

444 test_done