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

From: Jeff King <peff@peff.net>
Date: Jan 11, 2016, 21:33 UTC
Message-ID: <20160111213351.GA21131@sigill.intra.peff.net>
In-Reply-To: <xmqqtwmp2e6d.fsf@gitster.mtv.corp.google.com>

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

Show 13 quoted lines

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

Show 18 quoted lines

> 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