@matheusbsbs / matheus-bagliano / commits / 72a9a08283

midx: check size of pack names chunk

We parse the pack-name chunk as a series of NUL-terminated strings. But since we don't look at the chunk size, there's nothing to guarantee that we don't parse off the end of the chunk (or even off the end of the mapped file). We can record the length, and then as we parse make sure that we never walk past it. The new test exercises the case, though note that it does not actually segfault before this patch. It hits a NUL byte somewhere in one of the other chunks, and comes up with a garbage pack name. You could construct one that reads out-of-bounds (e.g., a PNAM chunk at the end of file), but this case is simple and sufficient to check that we detect the problem. 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 72a9a08283f1b56598d4af5efb8cd178d4150323

3 files changed +21 -2

midx.c

+9 -2

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

157 MIDX_HEADER_SIZE, m->num_chunks))

158 goto cleanup_fail;

159

160 - if (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))

160 + if (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names, &m->chunk_pack_names_len))

161 die(_("multi-pack-index required pack-name chunk missing or corrupted"));

162 if (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))

163 die(_("multi-pack-index required OID fanout chunk missing or corrupted"));

@@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local

176

177 cur_pack_name = (const char *)m->chunk_pack_names;

178 for (i = 0; i < m->num_packs; i++) {

179 + const char *end;

180 + size_t avail = m->chunk_pack_names_len -

181 + (cur_pack_name - (const char *)m->chunk_pack_names);

182 +

183 m->pack_names[i] = cur_pack_name;

184

181 - cur_pack_name += strlen(cur_pack_name) + 1;

185 + end = memchr(cur_pack_name, '\0', avail);

186 + if (!end)

187 + die(_("multi-pack-index pack-name chunk is too short"));

188 + cur_pack_name = end + 1;

189

190 if (i && strcmp(m->pack_names[i], m->pack_names[i - 1]) <= 0)

191 die(_("multi-pack-index pack names out of order: '%s' before '%s'"),

midx.h

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

32 int local;

34 const unsigned char *chunk_pack_names;

35 + size_t chunk_pack_names_len;

36 const uint32_t *chunk_oid_fanout;

37 const unsigned char *chunk_oid_lookup;

38 const unsigned char *chunk_object_offsets;

t/t5319-multi-pack-index.sh

+11

@@ -1083,4 +1083,15 @@ test_expect_success 'reader notices too-small oid lookup chunk' '

1083 test_cmp expect err

1084 '

1085

1086 +test_expect_success 'reader notices too-small pack names chunk' '

1087 + # There is no NUL to terminate the name here, so the

1088 + # chunk is too short.

1089 + corrupt_chunk PNAM clear 70656666 &&

1090 + test_must_fail git log 2>err &&

1091 + cat >expect <<-\EOF &&

1092 + fatal: multi-pack-index pack-name chunk is too short

1093 + EOF

1094 + test_cmp expect err

1095 +'

1096 +

1097 test_done