pack-bitmap: bounds-check size of cache extension

A .bitmap file may have a "name hash cache" extension, which puts a sequence of uint32_t values (one per object) at the end of the file. When we see a flag indicating this extension, we blindly subtract the appropriate number of bytes from our available length. However, if the .bitmap file is too short, we'll underflow our length variable and wrap around, thinking we have a very large length. This can lead to reading out-of-bounds bytes while loading individual ewah bitmaps. We can fix this by checking the number of available bytes when we parse the header. The existing "truncated bitmap" test is now split into two tests: one where we don't have this extension at all (and hence actually do try to read a truncated ewah bitmap) and one where we realize up-front that we can't even fit in the cache structure. We'll check stderr in each case to make sure we hit the error we're expecting. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Dec 8, 2020 at 17:03 UTC ec6c7b43679fcd7db00019a850636abc5c94e44e
2 files changed +21 -4
pack-bitmap.c
+6 -2
@@ -153,14 +153,18 @@ static int load_bitmap_header(struct bitmap_index *index)
153 /* Parse known bitmap format options */
154 {
155 uint32_t flags = ntohs(header->options);
156 + size_t cache_size = st_mult(index->pack->num_objects, sizeof(uint32_t));
157 + unsigned char *index_end = index->map + index->map_size - the_hash_algo->rawsz;
158
159 if ((flags & BITMAP_OPT_FULL_DAG) == 0)
160 return error("Unsupported options for bitmap index file "
161 "(Git requires BITMAP_OPT_FULL_DAG)");
162
163 if (flags & BITMAP_OPT_HASH_CACHE) {
162 - unsigned char *end = index->map + index->map_size - the_hash_algo->rawsz;
163 - index->hashes = ((uint32_t *)end) - index->pack->num_objects;
164 + if (cache_size > index_end - index->map - header_size)
165 + return error("corrupted bitmap index file (too short to fit hash cache)");
166 + index->hashes = (void *)(index_end - cache_size);
167 + index_end -= cache_size;
168 }
169 }
170
t/t5310-pack-bitmaps.sh
+15 -2
@@ -343,7 +343,8 @@ test_expect_success 'pack reuse respects --incremental' '
343 test_must_be_empty actual
344 '
345
346 -test_expect_success 'truncated bitmap fails gracefully' '
346 +test_expect_success 'truncated bitmap fails gracefully (ewah)' '
347 + test_config pack.writebitmaphashcache false &&
348 git repack -ad &&
349 git rev-list --use-bitmap-index --count --all >expect &&
350 bitmap=$(ls .git/objects/pack/*.bitmap) &&
@@ -352,7 +353,19 @@ test_expect_success 'truncated bitmap fails gracefully' '
353 mv -f $bitmap.tmp $bitmap &&
354 git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
355 test_cmp expect actual &&
355 - test_i18ngrep corrupt stderr
356 + test_i18ngrep corrupt.ewah.bitmap stderr
357 +'
358 +
359 +test_expect_success 'truncated bitmap fails gracefully (cache)' '
360 + git repack -ad &&
361 + git rev-list --use-bitmap-index --count --all >expect &&
362 + bitmap=$(ls .git/objects/pack/*.bitmap) &&
363 + test_when_finished "rm -f $bitmap" &&
364 + test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
365 + mv -f $bitmap.tmp $bitmap &&
366 + git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
367 + test_cmp expect actual &&
368 + test_i18ngrep corrupted.bitmap.index stderr
369 '
370
371 # have_delta <obj> <expected_base>