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

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

[Jacek Wielemborek]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff King]

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.

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

[Jonathan Nieder]

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

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

[Jeff King]

On Tue, Jan 05, 2016 at 04:23:33PM -0800, Jonathan Nieder 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.

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

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

[Jacek Wielemborek]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

W dniu 06.01.2016 o 01:23, Jonathan Nieder pisze:
> 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

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Duy Nguyen]

On Tue, Jan 5, 2016 at 10:24 PM, Jeff King <peff@peff.net> 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

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

[Jacek Wielemborek]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

W dniu 06.01.2016 o 10:46, Duy Nguyen pisze:
> On Tue, Jan 5, 2016 at 10:24 PM, Jeff King <peff@peff.net> 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.
> 

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

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

diff --git a/sha1_file.c b/sha1_file.c
index 73ccd49..8aca1f6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2458,6 +2458,13 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
 		off = ntohl(*((uint32_t *)(index + 4 * n)));
 		if (!(off & 0x80000000))
 			return off;
+
+		/* 8-byte offset table */
+		if ((p->index_size - (8 + 256 * 4 + 28 * p->num_objects + 40))
+		    <
+		    (off & 0x7fffffff) * 8)
+			die("offset beyond end of .idx file");
+
 		index += p->num_objects * 4 + (off & 0x7fffffff) * 8;
 		return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |
 				   ntohl(*((uint32_t *)(index + 4)));

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

[Jeff King]

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

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

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.

> diff --git a/sha1_file.c b/sha1_file.c
> index 73ccd49..8aca1f6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2458,6 +2458,13 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
>  		off = ntohl(*((uint32_t *)(index + 4 * n)));
>  		if (!(off & 0x80000000))
>  			return off;
> +
> +		/* 8-byte offset table */
> +		if ((p->index_size - (8 + 256 * 4 + 28 * p->num_objects + 40))
> +		    <
> +		    (off & 0x7fffffff) * 8)
> +			die("offset beyond end of .idx file");
> +
>  		index += p->num_objects * 4 + (off & 0x7fffffff) * 8;
>  		return (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |
>  				   ntohl(*((uint32_t *)(index + 4)));

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

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jan 07, 2016 at 02:54:50PM -0800, Junio C Hamano wrote:
>
>> 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.
>
> 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 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.

> 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

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

[Jeff King]

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

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

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