nth_packed_object_offset: bounds-check extended offset

If a pack .idx file has a corrupted offset for an object, we
may try to access an offset in the .idx or .pack file that
is larger than the file's size.  For the .pack case, we have
use_pack() to protect us, which realizes the access is out
of bounds. But if the corrupted value asks us to look in the
.idx file's secondary 64-bit offset table, we blindly add it
to the mmap'd index data and access arbitrary memory.

We can fix this with a simple bounds-check compared to the
size we found when we opened the .idx file.

Note that there's similar code in index-pack that is
triggered only during "index-pack --verify". To support
both, we pull the bounds-check into a separate function,
which dies when it sees a corrupted file.

It would be nice if we could return an error, so that the
pack code could try to find a good copy of the object
elsewhere. Currently nth_packed_object_offset doesn't have
any way to return an error, but it could probably use "0" as
a sentinel value (since no object can start there). This is
the minimal fix, and we can improve the resilience later on
top.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 723fe8e11d1d494a82f50caac1434dba7682962c..98bdbb5e6c0cc3ddc855d96427f05d880cd2dad6 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1445,6 +1445,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p,
                if (!(off & 0x80000000))
                        continue;
                off = off & 0x7fffffff;
+               check_pack_index_ptr(p, &idx2[off * 2]);
                if (idx2[off * 2])
                        continue;
                /*

diff --git a/cache.h b/cache.h
index 4427945bc08be2d182b123ce5f684d23304582d9..6c9aaa1ae63a2255a215c1287e38e75fcc5fc5d3 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1236,6 +1236,16 @@ extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
 extern struct packed_git *add_packed_git(const char *, int, int);
 
+/*
+ * Make sure that a pointer access into an mmap'd index file is within bounds,
+ * and can provide at least 8 bytes of data.
+ *
+ * Note that this is only necessary for variable-length segments of the file
+ * (like the 64-bit extended offset table), as we compare the size to the
+ * fixed-length parts when we open the file.
+ */
+extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
+
 /*
  * Return the SHA-1 of the nth object within the specified packfile.
  * Open the index if it is not already open.  The return value points

diff --git a/sha1_file.c b/sha1_file.c
index 99155c0d6b77aa2a772330d51fa604b1c4a9b0cf..bd0f8f7c8db5731ef4e808ceff536860e97f6746 100644 (file)
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2359,6 +2359,20 @@ const unsigned char *nth_packed_object_sha1(struct packed_git *p,
        }
 }
 
+void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
+{
+       const unsigned char *ptr = vptr;
+       const unsigned char *start = p->index_data;
+       const unsigned char *end = start + p->index_size;
+       if (ptr < start)
+               die("offset before start of pack index for %s (corrupt index?)",
+                   p->pack_name);
+       /* No need to check for underflow; .idx files must be at least 8 bytes */
+       if (ptr >= end - 8)
+               die("offset beyond end of pack index for %s (truncated index?)",
+                   p->pack_name);
+}
+
 off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
 {
        const unsigned char *index = p->index_data;
@@ -2372,6 +2386,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
                if (!(off & 0x80000000))
                        return off;
                index += p->num_objects * 4 + (off & 0x7fffffff) * 8;
+               check_pack_index_ptr(p, index);
                return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |
                                   ntohl(*((uint32_t *)(index + 4)));
        }

diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index efc519734524e3a7aeb795d6e730e520451ef481..0717746479a4ca82302d67faf91bf71c76bc7ceb 100755 (executable)
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh
@@ -128,7 +128,7 @@ test_expect_success 'bogus object offset (v2, no msb)' '
        test_must_fail git index-pack --verify $pack
 '
 
-test_expect_failure 'bogus offset into v2 extended table' '
+test_expect_success 'bogus offset into v2 extended table' '
        do_pack $object --index-version=2 &&
        munge $idx $(ofs_table 1) "\377\0\0\0" &&
        clear_base &&