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:

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.

Jeff King wrote:

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

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:

quoted

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

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.

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:

Jeff King wrote:

quoted

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

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

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 [off-list ref] wrote:

If you can find a fuzzed packfile that crashes "index-pack", then _that_
would be a big deal.

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

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

On Tue, Jan 5, 2016 at 10:24 PM, Jeff King [off-list ref] wrote:

quoted

If you can find a fuzzed packfile that crashes "index-pack", then _that_
would be a big deal.

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.

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.