Skip to content

Commit 090de6b

Browse files
committed
Merge branch 'jk/pack-idx-corruption-safety'
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 bc0ffd4 + 7465feb commit 090de6b

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
@@ -1369,6 +1369,16 @@ extern void free_pack_by_name(const char *);
13691369
extern void clear_delta_base_cache(void);
13701370
extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);
13711371

1372+
/*
1373+
* Make sure that a pointer access into an mmap'd index file is within bounds,
1374+
* and can provide at least 8 bytes of data.
1375+
*
1376+
* Note that this is only necessary for variable-length segments of the file
1377+
* (like the 64-bit extended offset table), as we compare the size to the
1378+
* fixed-length parts when we open the file.
1379+
*/
1380+
extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
1381+
13721382
/*
13731383
* Return the SHA-1 of the nth object within the specified packfile.
13741384
* 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)