Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 13, 2023, 14:24 UTC
Message-ID: <ZLAJNbIBFUPHYhlt@nand.local>
In-Reply-To: <5d2cf09f-34c7-9a88-bab2-8bf348dd13bb@gmail.com>

On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:

Show 15 quoted lines

> > diff --git a/packfile.c b/packfile.c
> > index 89220f0e03..70acf1694b 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
> >   		     */
> >   		    (sizeof(off_t) <= 4))
> >   			return error("pack too large for current definition of off_t in %s", path);
> > -		p->crc_offset = 8 + 4 * 256 + nr * hashsz;
> > +		p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));
>
> p->crc_offset is a uint32_t so we're still prone to truncation here unless
> we change the crc_offset member of struct packed_git to be a size_t. I
> haven't checked if the other users of crc_offset would need adjusting if we
> change its type.

Thanks for spotting. Luckily, this should be a straightforward change:

    $ git grep crc_offset
    builtin/index-pack.c:	idx1 = (((const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset))
    object-store-ll.h:	uint32_t crc_offset;
    packfile.c:		p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));

The single usage in index-pack is OK, so we only need to change its type to a size_t.

I could see an argument that this should be an off_t, since it is an offset into a file. But since we memory map the whole thing anyway, I think we are equally OK to treat it as a pointer offset. A similar argument is made in f86f769550e (compute pack .idx byte offsets using size_t, 2020-11-13), so I am content to leave this as a size_t.

Thanks, Taylor