From 9622610e55c7d4f81a924387947884b2ac268934 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 9 Oct 2023 17:05:38 -0400 Subject: [PATCH] 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(-) 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