@matheusbsbs / matheus-bagliano / commits / c0fe9b2da5

midx: check size of revindex chunk

When we load a revindex from disk, we check the size of the file compared to the number of objects we expect it to have. But when we use a RIDX chunk stored directly in the midx, we just access the memory directly. This can lead to out-of-bounds memory access for a corrupted or malicious multi-pack-index file. We can catch this by recording the RIDX chunk size, and then checking it against the expected size when we "load" the revindex. Note that this check is much simpler than the one that load_revindex_from_disk() does, because we just have the data array with no header (so we do not need to account for the header size, and nor do we need to bother validating the header values). The test confirms both that we catch this case, and that we continue the process (the revindex is required to use the midx bitmaps, but we fallback to a non-bitmap traversal). 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 c0fe9b2da5610bd4ff62d7871dfbbfa0247c7949

4 files changed +32 -2

midx.c

+2 -1

@@ -184,7 +184,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local

184 &m->chunk_large_offsets_len);

185

186 if (git_env_bool("GIT_TEST_MIDX_READ_RIDX", 1))

187 - pair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);

187 + pair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex,

188 + &m->chunk_revindex_len);

189

190 CALLOC_ARRAY(m->pack_names, m->num_packs);

191 CALLOC_ARRAY(m->packs, m->num_packs);

midx.h

@@ -39,6 +39,7 @@ struct multi_pack_index {

39 const unsigned char *chunk_large_offsets;

40 size_t chunk_large_offsets_len;

41 const unsigned char *chunk_revindex;

42 + size_t chunk_revindex_len;

44 const char **pack_names;

45 struct packed_git **packs;

pack-revindex.c

+12 -1

@@ -343,6 +343,17 @@ int verify_pack_revindex(struct packed_git *p)

343 return res;

344 }

345

346 +static int can_use_midx_ridx_chunk(struct multi_pack_index *m)

347 +{

348 + if (!m->chunk_revindex)

349 + return 0;

350 + if (m->chunk_revindex_len != st_mult(sizeof(uint32_t), m->num_objects)) {

351 + error(_("multi-pack-index reverse-index chunk is the wrong size"));

352 + return 0;

353 + }

354 + return 1;

355 +}

356 +

357 int load_midx_revindex(struct multi_pack_index *m)

358 {

359 struct strbuf revindex_name = STRBUF_INIT;

@@ -351,7 +362,7 @@ int load_midx_revindex(struct multi_pack_index *m)

362 if (m->revindex_data)

363 return 0;

364

354 - if (m->chunk_revindex) {

365 + if (can_use_midx_ridx_chunk(m)) {

366 /*

367 * If the MIDX `m` has a `RIDX` chunk, then use its contents for

368 * the reverse index instead of trying to load a separate `.rev`

t/t5319-multi-pack-index.sh

+17

@@ -1138,4 +1138,21 @@ test_expect_success 'reader bounds-checks large offset table' '

1138 )

1139 '

1140

1141 +test_expect_success 'reader notices too-small revindex chunk' '

1142 + # We only get a revindex with bitmaps (and likewise only

1143 + # load it when they are asked for).

1144 + test_config repack.writeBitmaps true &&

1145 + corrupt_chunk RIDX clear 00000000 &&

1146 + git -c core.multipackIndex=false rev-list \

1147 + --all --use-bitmap-index >expect.out &&

1148 + git -c core.multipackIndex=true rev-list \

1149 + --all --use-bitmap-index >out 2>err &&

1150 + test_cmp expect.out out &&

1151 + cat >expect.err <<-\EOF &&

1152 + error: multi-pack-index reverse-index chunk is the wrong size

1153 + warning: multi-pack bitmap is missing required reverse index

1154 + EOF

1155 + test_cmp expect.err err

1156 +'

1157 +

1158 test_done