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

Hello,

Steps to reproduce:

1. base64 -d and unpack the .tar.gz file from here:
https://gist.github.com/d33tah/4e976f2e043718594a85

2. cd into it, run "git log"

I'll be happy to guide you through the fuzzing process - I stopped it at the first crash.

Cheers, d33tah

On Tue, Jan 05, 2016 at 02:44:49PM +0100, Jacek Wielemborek wrote:

I'm not terribly surprised, and we should look into fixing the segfault[1]. But I want to also note that fuzzing packfiles for "git log" is not a terribly realistic attack vector.

Git packfiles come from two places:

  1. Local maintenance repacks loose and already-packed objects into a
     new packfile. We trust the local repack process to generate a valid
     packfile (though the contents of individual objects may be
     untrusted, of course).

  2. A fetch or push may introduce a new packfile to the repository, but
     it does not enter directly. It is passed through "index-pack
     --stdin", which checks it byte by byte, creating its own index, and
     making sure that both the pack structure is good, and optionally
     the format of the individual object data.

So "index-pack" is the enforcement point, and the rest of the git commands generally assume that we can trust what is on disk (as it is has either been generated by us, or checked by index-pack). The rest of the commands do not spend time checking that the on-disk contents are sane (though you can run git-fsck if you want to do that).

If you can find a fuzzed packfile that crashes "index-pack", then _that_ would be a big deal. I tried AFL against it a few months ago, but didn't turn up any hits. I didn't run it for all that long, but I'd be surprised if it is all that fruitful. There are quite a few embedded checksums in a packfile (zlib checksums, as well as a sha1 over the whole file), and index-pack punts when one doesn't match. I think you'd need a fuzzer which is aware of the zlib and packfile formats.

-Peff

[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.

    Which isn't to say there aren't _some_ attacks you could do with
    this. But git's security model has never been that you can untar
    a random .git directory and use it securely.

Jeff King wrote:

I think we should reconsider such trust. If one user creates a malicious pack, if another user uses read-only git commands to access the repository (after inspecting .git/config to make sure it doesn't contain anything scary) the result should not be arbitrary code execution.

Producing bogus output or aborting is okay; arbitrary code execution less so.

Thanks, Jonathan

On Tue, Jan 05, 2016 at 04:23:33PM -0800, Jonathan Nieder wrote:

I agree it is better if we can meet this standard, and I didn't mean to discourage fixes in the area. But I do think it is worth classifying them differently than bugs that can be triggered via the network. The attack surface for on-disk attacks is much larger, and the number of people affected is much smaller.

Regarding your example, I'm not sure it's the best motivating example, as I imagine hardly anyone examines .git/config. :) A simplified one might be that:

  git fetch me@shared-server:/home/you/foo.git

is running git-upload-pack as me on packfiles created by you (on the server). We normally expect that to be a "safe" operation (and it is if done over git:// or similar).

-Peff

W dniu 06.01.2016 o 01:23, Jonathan Nieder pisze:

I'd be happy to help you go through the fuzzing process - I don't have enough horsepower and codebase knowledge to do it on my own though. If you have an afl-fuzz question though, let me know.

On Tue, Jan 5, 2016 at 10:24 PM, Jeff King <peff@peff.net> wrote:

I'm sure you know this, but if Jacek moves to break index-pack, then he/she should also try to break unpack-objects because sometimes we use that command instead of index-pack.

W dniu 06.01.2016 o 10:46, Duy Nguyen pisze:

It sounds that you could use a little explanation on how I found this crashing case and what would it take to fuzz index-pack, according to the conversation I had on #git-devel on irc.freenode.net. Should I assume that you know the basic afl-fuzz in my next post?

BTW @Duy, thanks for CC to me, I'm not subscribed to the ML.

Jeff King <peff@peff.net> writes:

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().

When we map in the .idx file, we do only minimum sanity checks to make sure that the .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 nth_packed_object_offset() does return the data read from .idx file that is not checked for sanity. use_pack(), which is the helper used by the callers of nth_packed_object_offset() that finds the offset in the packfile for each object, avoids allowing a read access to mapped pack data beyond the end of it, so it is OK to return bogus value that was read from the .idx file from this function, but there is one computation the function itself does using a possibly bogus value read from the disk: to find out where in the secondary offset table in the .idx file the offset in the packfile is stored.

---

 This is not even compile tested; I just wanted to prevent people
 from adding two unnecessary checks to this function following my
 analysis in the previous message.  I think returning bogus value
 stored in a crafted .idx file from this function is OK, as the
 offset will be first used by use_pack() and the sanity of the
 offset, relative to the packfile size, is checked there, and an
 offset that points to a random point in the packfile will be caught
 by the pack reading code, either by unpack_compressed_entry() or by
 patch_delta(), so that is also safe.

 We do need to check the unprotected access here.  Nobody else in
 the current codepath protects us from this access attempting to
 read an unmapped memory and segfault.

 sha1_file.c | 7 +++++++
 1 file changed, 7 insertions(+)

On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:

I think this is the right track, and it does indeed catch the bogus .idx that started this thread. I agree that we should be OK to hand back a bogus offset to the pack code, which already handles bounds-checking due to the sliding window code.

It's hard to verify that this is right due to all the magic numbers. :)

This function advances the "index" pointer to access the data, and it has already handled the initial header, v2 object names, etc. I think it might be simpler to compute:

  const unsigned char *end = p->index_data + p->index_size;

and compare the computed "index" against that. I suspect the earlier accesses of "index" can also be fooled using integer overflows (e.g., claim that we have a huge number of objects, and (20 * p->num_objects) may overflow to arbitrary memory, especially on 32-bit systems where it's easy to wrap).

-Peff

On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:

Looks like this topic got dropped. I was reminded of it when somebody pointed me to a similar case[1] today which segfaults in a similar way (but this time was caused by actual filesystem corruption).

Did you ever push the patch below further along?

I confirmed that your patch detects and complains on this newer case. Though I think there is another similar problem in read_v2_anomalous_offsets (I haven't dug, but it triggers for me in "index-pack --verify").

-Peff

[1] https://github.com/libgit2/libgit2/issues/3556

Jeff King <peff@peff.net> writes:

I do not think so, as I didn't "dig"; I recall trying to be explicit that it was an illustration to prevent two extra and unnecessary changes I alluded to in the earlier parts of the thread, not a real patch.

On Wed, Feb 24, 2016 at 10:48:27AM -0800, Junio C Hamano wrote:

Thanks. I was planning to dig further, but I didn't want to duplicate any work. I've got a series which I'll post momentarily.

-Peff