Re: [PATCH 1/3] patch-delta: fix oob read

From: Jeff King <peff@peff.net>
Date: Aug 29, 2018, 21:20 UTC
Message-ID: <20180829212025.GB29880@sigill.intra.peff.net>
In-Reply-To: <20180829205857.77340-1-jannh@google.com>

On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:

Show 13 quoted lines

> If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
> `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
> into `dst_buf`.
> 
> This is not an exploitable bug because triggering the bug increments the
> `data` pointer beyond `top`, causing the `data != top` sanity check after
> the loop to trigger and discard the destination buffer - which means that
> the result of the out-of-bounds read is never used for anything.
> 
> Also, directly jump into the error handler instead of just breaking out of
> the loop - otherwise, data corruption would be silently ignored if the
> delta buffer ends with a command and the destination buffer is already
> full.

Nice catch. The patch looks good to me, but just to lay out my thought process looking for other related problems:

We have two types of instructions:

  1. Take N bytes from position P within the source.

  2. Take the next N bytes from the delta stream.

In both cases we need to check that:

  a. We have enough space in the destination buffer.

  b. We have enough source bytes.

So this:

Show 11 quoted lines

> diff --git a/patch-delta.c b/patch-delta.c
> index 56e0a5ede..283fb4b75 100644
> --- a/patch-delta.c
> +++ b/patch-delta.c
> @@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
>  			if (unsigned_add_overflows(cp_off, cp_size) ||
>  			    cp_off + cp_size > src_size ||
>  			    cp_size > size)
> -				break;
> +				goto bad_length;
>  			memcpy(out, (char *) src_buf + cp_off, cp_size);

Covers 1a (cp_size > size) and 1b (cp_off + cp_size > src_size), plus the obvious overflow possibility.

And then here:

Show 6 quoted lines

>  		} else if (cmd) {
> -			if (cmd > size)
> -				break;
> +			if (cmd > size || cmd > top - data)
> +				goto bad_length;
>  			memcpy(out, data, cmd);

We had previously dealt with 2a (cmd > size), but failed to handle 2b, which you've added. We don't need to deal with over/underflow here, because our subtraction is on pointers to the same buffer.

Show 8 quoted lines

> @@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
>  
>  	/* sanity check */
>  	if (data != top || size != 0) {
> +		bad_length:
>  		error("delta replay has gone wild");
>  		bad:
>  		free(dst_buf);

And I agree that jumping straight here is a good idea.

Overall, very nicely done.

-Peff