Skip to content

Commit 80047fa

Browse files
committed
Merge branch 'jk/pack-idx-corruption-safety' into maint
The code to read the pack data using the offsets stored in the pack idx file has been made more carefully check the validity of the data in the idx. * jk/pack-idx-corruption-safety: sha1_file.c: mark strings for translation use_pack: handle signed off_t overflow nth_packed_object_offset: bounds-check extended offset t5313: test bounds-checks of corrupted/malicious pack/idx files
2 parents 0e58b47 + 7465feb commit 80047fa

File tree

4 files changed

+207
-0
lines changed

4 files changed

+207
-0
lines changed

builtin/index-pack.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1514,6 +1514,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p,
15141514
if (!(off & 0x80000000))
15151515
continue;
15161516
off = off & 0x7fffffff;
1517+
check_pack_index_ptr(p, &idx2[off * 2]);
15171518
if (idx2[off * 2])
15181519
continue;
15191520
/*

cache.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,6 +1367,16 @@ extern void free_pack_by_name(const char *);
13671367
extern void clear_delta_base_cache(void);
13681368
extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
13691369

1370+
/*
1371+
* Make sure that a pointer access into an mmap'd index file is within bounds,
1372+
* and can provide at least 8 bytes of data.
1373+
*
1374+
* Note that this is only necessary for variable-length segments of the file
1375+
* (like the 64-bit extended offset table), as we compare the size to the
1376+
* fixed-length parts when we open the file.
1377+
*/
1378+
extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
1379+
13701380
/*
13711381
* Return the SHA-1 of the nth object within the specified packfile.
13721382
* Open the index if it is not already open. The return value points

sha1_file.c

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,6 +1076,8 @@ unsigned char *use_pack(struct packed_git *p,
10761076
die("packfile %s cannot be accessed", p->pack_name);
10771077
if (offset > (p->pack_size - 20))
10781078
die("offset beyond end of packfile (truncated pack?)");
1079+
if (offset < 0)
1080+
die(_("offset before end of packfile (broken .idx?)"));
10791081

10801082
if (!win || !in_window(win, offset)) {
10811083
if (win)
@@ -2448,6 +2450,20 @@ const unsigned char *nth_packed_object_sha1(struct packed_git *p,
24482450
}
24492451
}
24502452

2453+
void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
2454+
{
2455+
const unsigned char *ptr = vptr;
2456+
const unsigned char *start = p->index_data;
2457+
const unsigned char *end = start + p->index_size;
2458+
if (ptr < start)
2459+
die(_("offset before start of pack index for %s (corrupt index?)"),
2460+
p->pack_name);
2461+
/* No need to check for underflow; .idx files must be at least 8 bytes */
2462+
if (ptr >= end - 8)
2463+
die(_("offset beyond end of pack index for %s (truncated index?)"),
2464+
p->pack_name);
2465+
}
2466+
24512467
off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
24522468
{
24532469
const unsigned char *index = p->index_data;
@@ -2461,6 +2477,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
24612477
if (!(off & 0x80000000))
24622478
return off;
24632479
index += p->num_objects * 4 + (off & 0x7fffffff) * 8;
2480+
check_pack_index_ptr(p, index);
24642481
return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |
24652482
ntohl(*((uint32_t *)(index + 4)));
24662483
}

t/t5313-pack-bounds-checks.sh

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
#!/bin/sh
2+
3+
test_description='bounds-checking of access to mmapped on-disk file formats'
4+
. ./test-lib.sh
5+
6+
clear_base () {
7+
test_when_finished 'restore_base' &&
8+
rm -f $base
9+
}
10+
11+
restore_base () {
12+
cp base-backup/* .git/objects/pack/
13+
}
14+
15+
do_pack () {
16+
pack_objects=$1; shift
17+
sha1=$(
18+
for i in $pack_objects
19+
do
20+
echo $i
21+
done | git pack-objects "$@" .git/objects/pack/pack
22+
) &&
23+
pack=.git/objects/pack/pack-$sha1.pack &&
24+
idx=.git/objects/pack/pack-$sha1.idx &&
25+
chmod +w $pack $idx &&
26+
test_when_finished 'rm -f "$pack" "$idx"'
27+
}
28+
29+
munge () {
30+
printf "$3" | dd of="$1" bs=1 conv=notrunc seek=$2
31+
}
32+
33+
# Offset in a v2 .idx to its initial and extended offset tables. For an index
34+
# with "nr" objects, this is:
35+
#
36+
# magic(4) + version(4) + fan-out(4*256) + sha1s(20*nr) + crc(4*nr),
37+
#
38+
# for the initial, and another ofs(4*nr) past that for the extended.
39+
#
40+
ofs_table () {
41+
echo $((4 + 4 + 4*256 + 20*$1 + 4*$1))
42+
}
43+
extended_table () {
44+
echo $(($(ofs_table "$1") + 4*$1))
45+
}
46+
47+
test_expect_success 'set up base packfile and variables' '
48+
# the hash of this content starts with ff, which
49+
# makes some later computations much simpler
50+
echo 74 >file &&
51+
git add file &&
52+
git commit -m base &&
53+
git repack -ad &&
54+
base=$(echo .git/objects/pack/*) &&
55+
chmod +w $base &&
56+
mkdir base-backup &&
57+
cp $base base-backup/ &&
58+
object=$(git rev-parse HEAD:file)
59+
'
60+
61+
test_expect_success 'pack/index object count mismatch' '
62+
do_pack $object &&
63+
munge $pack 8 "\377\0\0\0" &&
64+
clear_base &&
65+
66+
# We enumerate the objects from the completely-fine
67+
# .idx, but notice later that the .pack is bogus
68+
# and fail to show any data.
69+
echo "$object missing" >expect &&
70+
git cat-file --batch-all-objects --batch-check >actual &&
71+
test_cmp expect actual &&
72+
73+
# ...and here fail to load the object (without segfaulting),
74+
# but fallback to a good copy if available.
75+
test_must_fail git cat-file blob $object &&
76+
restore_base &&
77+
git cat-file blob $object >actual &&
78+
test_cmp file actual &&
79+
80+
# ...and make sure that index-pack --verify, which has its
81+
# own reading routines, does not segfault.
82+
test_must_fail git index-pack --verify $pack
83+
'
84+
85+
test_expect_success 'matched bogus object count' '
86+
do_pack $object &&
87+
munge $pack 8 "\377\0\0\0" &&
88+
munge $idx $((255 * 4)) "\377\0\0\0" &&
89+
clear_base &&
90+
91+
# Unlike above, we should notice early that the .idx is totally
92+
# bogus, and not even enumerate its contents.
93+
>expect &&
94+
git cat-file --batch-all-objects --batch-check >actual &&
95+
test_cmp expect actual &&
96+
97+
# But as before, we can do the same object-access checks.
98+
test_must_fail git cat-file blob $object &&
99+
restore_base &&
100+
git cat-file blob $object >actual &&
101+
test_cmp file actual &&
102+
103+
test_must_fail git index-pack --verify $pack
104+
'
105+
106+
# Note that we cannot check the fallback case for these
107+
# further .idx tests, as we notice the problem in functions
108+
# whose interface doesn't allow an error return (like use_pack()),
109+
# and thus we just die().
110+
#
111+
# There's also no point in doing enumeration tests, as
112+
# we are munging offsets here, which are about looking up
113+
# specific objects.
114+
115+
test_expect_success 'bogus object offset (v1)' '
116+
do_pack $object --index-version=1 &&
117+
munge $idx $((4 * 256)) "\377\0\0\0" &&
118+
clear_base &&
119+
test_must_fail git cat-file blob $object &&
120+
test_must_fail git index-pack --verify $pack
121+
'
122+
123+
test_expect_success 'bogus object offset (v2, no msb)' '
124+
do_pack $object --index-version=2 &&
125+
munge $idx $(ofs_table 1) "\0\377\0\0" &&
126+
clear_base &&
127+
test_must_fail git cat-file blob $object &&
128+
test_must_fail git index-pack --verify $pack
129+
'
130+
131+
test_expect_success 'bogus offset into v2 extended table' '
132+
do_pack $object --index-version=2 &&
133+
munge $idx $(ofs_table 1) "\377\0\0\0" &&
134+
clear_base &&
135+
test_must_fail git cat-file blob $object &&
136+
test_must_fail git index-pack --verify $pack
137+
'
138+
139+
test_expect_success 'bogus offset inside v2 extended table' '
140+
# We need two objects here, so we can plausibly require
141+
# an extended table (if the first object were larger than 2^31).
142+
do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&
143+
144+
# We have to make extra room for the table, so we cannot
145+
# just munge in place as usual.
146+
{
147+
dd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&
148+
printf "\200\0\0\0" &&
149+
printf "\377\0\0\0\0\0\0\0" &&
150+
dd if=$idx bs=1 skip=$(extended_table 2)
151+
} >tmp &&
152+
mv tmp "$idx" &&
153+
clear_base &&
154+
test_must_fail git cat-file blob $object &&
155+
test_must_fail git index-pack --verify $pack
156+
'
157+
158+
test_expect_success 'bogus OFS_DELTA in packfile' '
159+
# Generate a pack with a delta in it.
160+
base=$(test-genrandom foo 3000 | git hash-object --stdin -w) &&
161+
delta=$(test-genrandom foo 2000 | git hash-object --stdin -w) &&
162+
do_pack "$base $delta" --delta-base-offset &&
163+
rm -f .git/objects/??/* &&
164+
165+
# Double check that we have the delta we expect.
166+
echo $base >expect &&
167+
echo $delta | git cat-file --batch-check="%(deltabase)" >actual &&
168+
test_cmp expect actual &&
169+
170+
# Now corrupt it. We assume the varint size for the delta is small
171+
# enough to fit in the first byte (which it should be, since it
172+
# is a pure deletion from the base), and that original ofs_delta
173+
# takes 2 bytes (which it should, as it should be ~3000).
174+
ofs=$(git show-index <$idx | grep $delta | cut -d" " -f1) &&
175+
munge $pack $(($ofs + 1)) "\177\377" &&
176+
test_must_fail git cat-file blob $delta >/dev/null
177+
'
178+
179+
test_done

0 commit comments

Comments
 (0)