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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html