From 52e2e8d43dbae8c05b68efd60cde2aacf3a23890 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 16:59:51 -0400 Subject: commit-graph: check size of oid fanout chunk We load the oid fanout chunk with pair_chunk(), which means we never see the size of the chunk. We just assume the on-disk file uses the appropriate size, and if it's too small we'll access random memory. It's easy to check this up-front; the fanout always consists of 256 uint32's, since it is a fanout of the first byte of the hash pointing into the oid index. These parameters can't be changed without introducing a new chunk type. This matches the similar check in the midx OIDF chunk (but note that rather than checking for the error immediately, the graph code just leaves parts of the struct NULL and checks for required fields later). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5318-commit-graph.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) (limited to 't/t5318-commit-graph.sh') diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index ba65f17dd9..d25bea3ec5 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -2,6 +2,7 @@ test_description='commit graph' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-chunk.sh GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0 @@ -821,4 +822,29 @@ test_expect_success 'overflow during generation version upgrade' ' ) ' +corrupt_chunk () { + graph=full/.git/objects/info/commit-graph && + test_when_finished "rm -rf $graph" && + git -C full commit-graph write --reachable && + corrupt_chunk_file $graph "$@" +} + +check_corrupt_chunk () { + corrupt_chunk "$@" && + git -C full -c core.commitGraph=false log >expect.out && + git -C full -c core.commitGraph=true log >out 2>err && + test_cmp expect.out out +} + +test_expect_success 'reader notices too-small oid fanout chunk' ' + # make it big enough that the graph file is plausible, + # otherwise we hit an earlier check + check_corrupt_chunk OIDF clear $(printf "000000%02x" $(test_seq 250)) && + cat >expect.err <<-\EOF && + error: commit-graph oid fanout chunk is wrong size + error: commit-graph is missing the OID Fanout chunk + EOF + test_cmp expect.err err +' + test_done -- cgit v1.3 From 4169d8964523198ca89f507824c07b70ba833732 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 17:04:58 -0400 Subject: commit-graph: check consistency of fanout table We use bsearch_hash() to look up items in the oid index of a commit-graph. It also has a fanout table to reduce the initial range in which we'll search. But since the fanout comes from the on-disk file, a corrupted or malicious file can cause us to look outside of the allocated index memory. One solution here would be to pass the total table size to bsearch_hash(), which could then bounds check the values it reads from the fanout. But there's an inexpensive up-front check we can do, and it's the same one used by the midx and pack idx code (both of which likewise have fanout tables and use bsearch_hash(), but are not affected by this bug): 1. We can check the value of the final fanout entry against the size of the table we got from the index chunk. These must always match, since the fanout is just slicing up the index. As a side note, the midx and pack idx code compute it the other way around: they use the final fanout value as the object count, and check the index size against it. Either is valid; if they disagree we cannot know which is wrong (a corrupted fanout value, or a too-small table of oids). 2. We can quickly scan the fanout table to make sure it is monotonically increasing. If it is, then we know that every value is less than or equal to the final value, and therefore less than or equal to the table size. It would also be sufficient to just check that each fanout value is smaller than the final one, but the midx and pack idx code both do a full monotonicity check. It's the same cost, and it catches some other corruptions (though not all; the checks done by "commit-graph verify" are more complete but more expensive, and our goal here is to be fast and memory-safe). There are two new tests. One just checks the final fanout value (this is the mirror image of the "too small oid lookup" case added for the midx in the previous commit; it's flipped here because commit-graph considers the oid lookup chunk to be the source of truth). The other actually creates a fanout with many out-of-bounds entries, and prior to this patch, it does cause the segfault you'd expect. But note that the error is not "your fanout entry is out-of-bounds", but rather "fanout value out of order". That's because we leave the final fanout value in place (to get past the table size check), making the index non-monotonic (the second-to-last entry is big, but the last one must remain small to match the actual table). We need adjustments to a few existing tests, as well: - an earlier test in t5318 corrupts the fanout and runs "commit-graph verify". Its message is now changed, since we catch the problem earlier (during the load step, rather than the careful validation step). - in t5324, we test that "commit-graph verify --shallow" does not do expensive verification on the base file of the chain. But the corruption it uses (munging a byte at offset 1000) happens to be in the middle of the fanout table. And now we detect that problem in the cheaper checks that are performed for every part of the graph. We'll push this back to offset 1500, which is only caught by the more expensive checksum validation. Likewise, there's a later test in t5324 which munges an offset 100 bytes into a file (also in the fanout table) that is referenced by an alternates file. So we now find that corruption during the load step, rather than the verification step. At the very least we need to change the error message (like the case above in t5318). But it is probably good to make sure we handle all parts of the verification even for alternate graph files. So let's likewise corrupt byte 1500 and make sure we found the invalid checksum. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 16 ++++++++++++++++ t/t5318-commit-graph.sh | 25 ++++++++++++++++++++++++- t/t5324-split-commit-graph.sh | 6 +++--- 3 files changed, 43 insertions(+), 4 deletions(-) (limited to 't/t5318-commit-graph.sh') diff --git a/commit-graph.c b/commit-graph.c index 9b3b01da61..b217e19194 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -277,6 +277,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r, static int verify_commit_graph_lite(struct commit_graph *g) { + int i; + /* * Basic validation shared between parse_commit_graph() * which'll be called every time the graph is used, and the @@ -302,6 +304,20 @@ static int verify_commit_graph_lite(struct commit_graph *g) return 1; } + for (i = 0; i < 255; i++) { + uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]); + uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]); + + if (oid_fanout1 > oid_fanout2) { + error("commit-graph fanout values out of order"); + return 1; + } + } + if (ntohl(g->chunk_oid_fanout[255]) != g->num_commits) { + error("commit-graph oid table and fanout disagree on size"); + return 1; + } + return 0; } diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d25bea3ec5..d10658de9e 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' ' test_expect_success 'detect incorrect fanout final value' ' corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \ - "fanout value" + "oid table and fanout disagree on size" ' test_expect_success 'detect incorrect OID order' ' @@ -847,4 +847,27 @@ test_expect_success 'reader notices too-small oid fanout chunk' ' test_cmp expect.err err ' +test_expect_success 'reader notices fanout/lookup table mismatch' ' + check_corrupt_chunk OIDF 1020 "FFFFFFFF" && + cat >expect.err <<-\EOF && + error: commit-graph oid table and fanout disagree on size + EOF + test_cmp expect.err err +' + +test_expect_success 'reader notices out-of-bounds fanout' ' + # Rather than try to corrupt a specific hash, we will just + # wreck them all. But we cannot just set them all to 0xFFFFFFFF or + # similar, as they are used for hi/lo starts in a binary search (so if + # they are identical, that indicates that the search should abort + # immediately). Instead, we will give them high values that differ by + # 2^24, ensuring that any that are used would cause an out-of-bounds + # read. + check_corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) && + cat >expect.err <<-\EOF && + error: commit-graph fanout values out of order + EOF + test_cmp expect.err err +' + test_done diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 06bb897f02..55b5765e2d 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -317,7 +317,7 @@ test_expect_success 'verify --shallow does not check base contents' ' cd verify-shallow && git commit-graph verify && base_file=$graphdir/graph-$(head -n 1 $graphdir/commit-graph-chain).graph && - corrupt_file "$base_file" 1000 "\01" && + corrupt_file "$base_file" 1500 "\01" && git commit-graph verify --shallow && test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err && @@ -391,10 +391,10 @@ test_expect_success 'verify across alternates' ' test_commit extra && git commit-graph write --reachable --split && tip_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph && - corrupt_file "$tip_file" 100 "\01" && + corrupt_file "$tip_file" 1500 "\01" && test_must_fail git commit-graph verify --shallow 2>test_err && grep -v "^+" test_err >err && - test_i18ngrep "commit-graph has incorrect fanout value" err + test_i18ngrep "incorrect checksum" err ) ' -- cgit v1.3 From b72df612afc12b46ea003732d739d7d746871773 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 17:05:36 -0400 Subject: commit-graph: check size of commit data chunk We expect a commit-graph file to have a fixed-size data record for each commit in the file (and we know the number of commits to expct from the size of the lookup table). If we encounter a file where this is too small, we'll look past the end of the chunk (and possibly even off the mapped memory). We can fix this by checking the size up front when we record the pointer. The included test doesn't segfault, since it ends up reading bytes from another chunk. But it produces nonsense results, since the values it reads are garbage. Our test notices this by comparing the output to a non-corrupted run of the same command (and of course we also check that the expected error is printed to stderr). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 12 +++++++++++- t/t5318-commit-graph.sh | 9 +++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) (limited to 't/t5318-commit-graph.sh') diff --git a/commit-graph.c b/commit-graph.c index 472332f603..9b80bbd75b 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -340,6 +340,16 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start, return 0; } +static int graph_read_commit_data(const unsigned char *chunk_start, + size_t chunk_size, void *data) +{ + struct commit_graph *g = data; + if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH) + return error("commit-graph commit data chunk is wrong size"); + g->chunk_commit_data = chunk_start; + return 0; +} + static int graph_read_bloom_data(const unsigned char *chunk_start, size_t chunk_size, void *data) { @@ -422,7 +432,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph); read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph); - pair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data); + read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph); pair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges); pair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs); diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index d10658de9e..492460157d 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -870,4 +870,13 @@ test_expect_success 'reader notices out-of-bounds fanout' ' test_cmp expect.err err ' +test_expect_success 'reader notices too-small commit data chunk' ' + check_corrupt_chunk CDAT clear 00000000 && + cat >expect.err <<-\EOF && + error: commit-graph commit data chunk is wrong size + error: commit-graph is missing the Commit Data chunk + EOF + test_cmp expect.err err +' + test_done -- cgit v1.3 From 9622610e55c7d4f81a924387947884b2ac268934 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 17:05:38 -0400 Subject: commit-graph: detect out-of-bounds extra-edges pointers If an entry in a commit-graph file has more than 2 parents, the fixed-size parent fields instead point to an offset within an "extra edges" chunk. We blindly follow these, assuming that the chunk is present and sufficiently large; this can lead to an out-of-bounds read for a corrupt or malicious file. We can fix this by recording the size of the chunk and adding a bounds-check in fill_commit_in_graph(). There are a few tricky bits: 1. We'll switch from working with a pointer to an offset. This makes some corner cases just fall out naturally: a. If we did not find an EDGE chunk at all, our size will correctly be zero (so everything is "out of bounds"). b. Comparing "size / 4" lets us make sure we have at least 4 bytes to read, and we never compute a pointer more than one element past the end of the array (computing a larger pointer is probably OK in practice, but is technically undefined behavior). c. The current code casts to "uint32_t *". Replacing it with an offset avoids any comparison between different types of pointer (since the chunk is stored as "unsigned char *"). 2. This is the first case in which fill_commit_in_graph() may return anything but success. We need to make sure to roll back the "parsed" flag (and any parents we might have added before running out of buffer) so that the caller can cleanly fall back to loading the commit object itself. It's a little non-trivial to do this, and we might benefit from factoring it out. But we can wait on that until we actually see a second case where we return an error. As a bonus, this lets us drop the st_mult() call. Since we've already done a bounds check, we know there won't be any integer overflow (it would imply our buffer is larger than a size_t can hold). The included test does not actually segfault before this patch (though you could construct a case where it does). Instead, it reads garbage from the next chunk which results in it complaining about a bogus parent id. This is sufficient for our needs, though (we care that the fallback succeeds, and that stderr mentions the out-of-bounds read). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 20 ++++++++++++++------ commit-graph.h | 1 + t/t5318-commit-graph.sh | 8 ++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) (limited to 't/t5318-commit-graph.sh') diff --git a/commit-graph.c b/commit-graph.c index 9b80bbd75b..e4860841fc 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -433,7 +433,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph); read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph); read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph); - pair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges); + pair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges, + &graph->chunk_extra_edges_size); pair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs); if (s->commit_graph_generation_version >= 2) { @@ -899,7 +900,7 @@ static int fill_commit_in_graph(struct repository *r, struct commit_graph *g, uint32_t pos) { uint32_t edge_value; - uint32_t *parent_data_ptr; + uint32_t parent_data_pos; struct commit_list **pptr; const unsigned char *commit_data; uint32_t lex_index; @@ -931,14 +932,21 @@ static int fill_commit_in_graph(struct repository *r, return 1; } - parent_data_ptr = (uint32_t*)(g->chunk_extra_edges + - st_mult(4, edge_value & GRAPH_EDGE_LAST_MASK)); + parent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK; do { - edge_value = get_be32(parent_data_ptr); + if (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) { + error("commit-graph extra-edges pointer out of bounds"); + free_commit_list(item->parents); + item->parents = NULL; + item->object.parsed = 0; + return 0; + } + edge_value = get_be32(g->chunk_extra_edges + + sizeof(uint32_t) * parent_data_pos); pptr = insert_parent_or_die(r, g, edge_value & GRAPH_EDGE_LAST_MASK, pptr); - parent_data_ptr++; + parent_data_pos++; } while (!(edge_value & GRAPH_LAST_EDGE)); return 1; diff --git a/commit-graph.h b/commit-graph.h index 20ada7e891..1f8a9de4fb 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -95,6 +95,7 @@ struct commit_graph { const unsigned char *chunk_generation_data; const unsigned char *chunk_generation_data_overflow; const unsigned char *chunk_extra_edges; + size_t chunk_extra_edges_size; const unsigned char *chunk_base_graphs; const unsigned char *chunk_bloom_indexes; const unsigned char *chunk_bloom_data; diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 492460157d..05bafcfe5f 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -879,4 +879,12 @@ test_expect_success 'reader notices too-small commit data chunk' ' test_cmp expect.err err ' +test_expect_success 'reader notices out-of-bounds extra edge' ' + check_corrupt_chunk EDGE clear && + cat >expect.err <<-\EOF && + error: commit-graph extra-edges pointer out of bounds + EOF + test_cmp expect.err err +' + test_done -- cgit v1.3 From 4a3c34662bc56a0e2369635536ac2ee1e79d8f56 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 17:05:44 -0400 Subject: commit-graph: check size of generations chunk We neither check nor record the size of the generations chunk we parse from a commit-graph file. This should have one uint32_t for each commit in the file; if it is smaller (due to corruption, etc), we may read outside the mapped memory. The included test segfaults without this patch, as it shrinks the size considerably (and the chunk is near the end of the file, so we read off the end of the array rather than accidentally reading another chunk). We can fix this by checking the size up front (like we do for other fixed-size chunks, like CDAT). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit-graph.c | 14 ++++++++++++-- t/t5318-commit-graph.sh | 8 ++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) (limited to 't/t5318-commit-graph.sh') diff --git a/commit-graph.c b/commit-graph.c index 4377b547c8..ca26870d1b 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -350,6 +350,16 @@ static int graph_read_commit_data(const unsigned char *chunk_start, return 0; } +static int graph_read_generation_data(const unsigned char *chunk_start, + size_t chunk_size, void *data) +{ + struct commit_graph *g = data; + if (chunk_size != g->num_commits * sizeof(uint32_t)) + return error("commit-graph generations chunk is wrong size"); + g->chunk_generation_data = chunk_start; + return 0; +} + static int graph_read_bloom_data(const unsigned char *chunk_start, size_t chunk_size, void *data) { @@ -439,8 +449,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s, &graph->chunk_base_graphs_size); if (s->commit_graph_generation_version >= 2) { - pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA, - &graph->chunk_generation_data); + read_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA, + graph_read_generation_data, graph); pair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW, &graph->chunk_generation_data_overflow); diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 05bafcfe5f..6505ff595a 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -887,4 +887,12 @@ test_expect_success 'reader notices out-of-bounds extra edge' ' test_cmp expect.err err ' +test_expect_success 'reader notices too-small generations chunk' ' + check_corrupt_chunk GDA2 clear 00000000 && + cat >expect.err <<-\EOF && + error: commit-graph generations chunk is wrong size + EOF + test_cmp expect.err err +' + test_done -- cgit v1.3