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

[Jeff King <peff@peff.net>]

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

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