Re: [PATCH v2 2/3] commit-graph: fix buffer read-overflow

From: Jeff King <peff@peff.net>
Date: Dec 7, 2018, 09:07 UTC
Message-ID: <20181207090755.GB5167@sigill.intra.peff.net>
In-Reply-To: <af45c2337fbe2a59ac95aff3ce90a69d8c30416f.1544127439.git.steadmon@google.com>

On Thu, Dec 06, 2018 at 12:20:54PM -0800, Josh Steadmon wrote:

Show 20 quoted lines

> diff --git a/commit-graph.c b/commit-graph.c
> index 07dd410f3c..224a5f161e 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -165,10 +165,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
>  	last_chunk_offset = 8;
>  	chunk_lookup = data + 8;
>  	for (i = 0; i < graph->num_chunks; i++) {
> -		uint32_t chunk_id = get_be32(chunk_lookup + 0);
> -		uint64_t chunk_offset = get_be64(chunk_lookup + 4);
> +		uint32_t chunk_id;
> +		uint64_t chunk_offset;
>  		int chunk_repeated = 0;
>  
> +		if (chunk_lookup + GRAPH_CHUNKLOOKUP_WIDTH >
> +		    data + graph_size) {
> +			error(_("chunk lookup table entry missing; graph file may be incomplete"));
> +			free(graph);
> +			return NULL;
> +		}

Is it possible to overflow the addition here? E.g., if I'm on a 32-bit system and the truncated chunk appears right at the 4GB limit, in which case we wrap back around? I guess that's pretty implausible, since it would mean that the mmap is bumping up against the end of the address space. I didn't check, but I wouldn't be surprised if sane operating systems avoid allocating those addresses.

But I think you could write this as:

  if (data + graph_size - chunk_lookup < GRAPH_CHUNKLOOKUP_WIDTH)

to avoid overflow (we know that "data + graph_size" is sane because
that's our mmap, and chunk_lookup is somewhere between "data" and "data
+ graph_size", so the result is between 0 and graph_size).

I dunno. I think I've convinced myself it's a non-issue here, but it may be good to get in the habit of writing these sorts of offset checks in an overflow-proof order.

-Peff