From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 06 Jun 2024 19:42:59 GMT
Subject: Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded
Message-ID: <xmqqfrtpluws.fsf@gitster.g>
In-Reply-To: <0a16399d14afd527f4db63f2a4a3b0a3cbf112f1.1717699237.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

> After reading the pseudo-merge extension's metadata table, we allocate
> an array to store information about each pseudo-merge, including its
> byte offset within the .bitmap file itself.
>
> This is done like so:
>
>     pseudo_merge_ofs = index_end - 24 -
>             (index->pseudo_merges.nr * sizeof(uint64_t));
>     for (i = 0; i < index->pseudo_merges.nr; i++) {
>             index->pseudo_merges.v[i].at = get_be64(pseudo_merge_ofs);
>             pseudo_merge_ofs += sizeof(uint64_t);
>     }
>
> But if the pseudo-merge table is corrupt, we'll keep calling get_be64()
> past the end of the pseudo-merge extension, potentially reading off the
> end of the mmap'd region.
>
> Prevent this by ensuring that we have at least `table_size - 24` many
> bytes available to read (subtracting 24 as the length of the metadata
> component).
>
> This is sufficient to prevent us from reading off the end of the
> pseudo-merge extension, and ensures that all of the get_be64() calls
> below are in bounds.

Can table_size at this point be smaller than 24, which will allow
(table_size - 24) to be a huge number that st_mult() will
comfortably fit?

> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pack-bitmap.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 70230e2647..ad2635c025 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -238,6 +238,9 @@ static int load_bitmap_header(struct bitmap_index *index)
>  				index->pseudo_merges.commits_nr = get_be32(index_end - 20);
>  				index->pseudo_merges.nr = get_be32(index_end - 24);
>  
> +				if (st_mult(index->pseudo_merges.nr, sizeof(uint64_t)) > table_size - 24)
> +					return error(_("corrupted bitmap index file, pseudo-merge table too short"));
> +
>  				CALLOC_ARRAY(index->pseudo_merges.v,
>  					     index->pseudo_merges.nr);