Re: [PATCH 03/23] pack-bitmap: bounds-check size of cache extension

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 13, 2020, 05:26 UTC
Message-ID: <CAN0heSpQURCY4xnLHzq8ok7as-YUq4VingPrZ-NJnetsv-RG1w@mail.gmail.com>
In-Reply-To: <20201113045700.GA743619@coredump.intra.peff.net>

On Fri, 13 Nov 2020 at 05:57, Jeff King <peff@peff.net> wrote:

Show 20 quoted lines

>
> On Thu, Nov 12, 2020 at 06:47:09PM +0100, Martin Ågren wrote:
>
> > > +               uint32_t cache_size = st_mult(index->pack->num_objects, sizeof(uint32_t));
> >
> > Hmm. If `sizeof(size_t)` is 8, then this multiplication can't possibly
> > overflow. A huge value of `num_objects` (say, 0xffffffff) would give a
> > huge return value (0xffffffff<<2) which would be truncated (0xfffffffc).
> > I think?
>
> Yeah, `cache_size` should absolutely be a `size_t`. If you have more
> than a billion objects, obviously your cache is going to be bigger than
> that. But most importantly, somebody can _claim_ to have a huge number
> of objects and foil the size checks by wrapping around.
>
> > Do we want a `u32_mult()`?
>
> Nah, we should be doing this as a size_t in the first place. There are
> similar problems with the .idx format, btw. I have a series to deal with
> that which I've been meaning to post.

Yes, that makes sense!

> >   if (cache_size > index_end - index->map - header_size)
>
> Yes, I agree this should be done as a subtraction as you showed to avoid
> integer overflow.

> >   -       if (index->map_size < sizeof(*header) + the_hash_algo->rawsz)
> >   +       if (index->map_size < header_size + the_hash_algo->rawsz)

> Right. I think that's right, and the previous patch is just buggy.

Martin