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

From: Jeff King <peff@peff.net>
Date: Jan 5, 2016, 15:24 UTC
Message-ID: <20160105152436.GA1205@sigill.intra.peff.net>
In-Reply-To: <568BC8D1.3080201@gmail.com>

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

Show 9 quoted lines

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

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.