Re: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2016, 19:37 UTC
Message-ID: <xmqqr3ht41w8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160105152436.GA1205@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> [1] I briefly ran your case under valgrind and got:
>
>     ==5409== Invalid read of size 4
>     ==5409==    at 0x55F92A: nth_packed_object_offset (sha1_file.c:2464)
>     ==5409==    by 0x55FBD4: find_pack_entry_one (sha1_file.c:2523)
>     ==5409==    by 0x55FCF7: fill_pack_entry (sha1_file.c:2566)
>     ==5409==    by 0x55FDE2: find_pack_entry (sha1_file.c:2604)
>     ==5409==    by 0x5615BA: has_sha1_file_with_flags (sha1_file.c:3212)
>     ==5409==    by 0x50FC38: has_sha1_file (cache.h:1049)
>     ==5409==    by 0x51043B: parse_object (object.c:259)
>     ==5409==    by 0x546BF9: get_reference (revision.c:254)
>     ==5409==    by 0x54CA15: setup_revisions (revision.c:2342)
>     ==5409==    by 0x4531DA: cmd_log_init_finish (log.c:156)
>     ==5409==    by 0x453465: cmd_log_init (log.c:211)
>     ==5409==    by 0x4547EB: cmd_log (log.c:672)
>     ==5409==  Address 0x840244bc is not stack'd, malloc'd or (recently) free'd
>
>     So I'd guess it's not the pack itself, but rather the .idx which is
>     full of nonsense values. And that's always generated from scratch
>     locally.

After I seeing your "it's not the pack itself but rather the .idx" without looking at (rather, conciously avoiding to look at) the valgrind trace, I checked the codepath that starts from read_packed_sha1().

When we map in the .idx file, we do minimum sanity checks to make sure that .idx file itself has sorted fan-out. We do not check if the object names are sorted, so a bogus .idx could tell us that an object does not exist when it exists in the matching .pack, but that is harmless. Also an unsorted object names will not make our binary search run in circles while looking things up. We do not check if the offset of individual objects are within the corresponding .pack file, either.

And I arrived at nth_packed_object_offset(), so I think I am hitting the right nail. There are a few values we read from the .idx file here that we would want to validate for sanity.

    off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
    {
            const unsigned char *index = p->index_data;
            index += 4 * 256;
            if (p->index_version == 1) {
                    return ntohl(*((uint32_t *)(index + 24 * n)));

This can return an offset that goes beyond the end of the packfile, possibly leading to a read access of unmapped region.

            } else {
                    uint32_t off;
                    index += 8 + p->num_objects * (20 + 4);
                    off = ntohl(*((uint32_t *)(index + 4 * n)));
                    if (!(off & 0x80000000))
                            return off;

This also can return an offset that goes beyond the end of the packfile.

Otherwise 'off' at this point gives an offset into the .idx file itself, that is the location of the 8-byte offset into the packfile.

                    index += p->num_objects * 4 + (off & 0x7fffffff) * 8;

And this computation can take us beyond the end of the .idx

                    return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |
                                       ntohl(*((uint32_t *)(index + 4)));

Or the value we read with this can take us beyond the end of the packfile.

            }
    }

As long as the returned value is within the valid region of packfile, packed_object_info() and unpack_entry() should be responsible for ensuring that we do not read past the end of the data. They should both begin by calling use_pack(), which already has the "offset beyond end of packfile" check, so I think that they would successfully catch a malicious .idx file that has handcrafted bad offsets if we fixed nth_packed_object_offset().