On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:
> 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:
> 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:
> } 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.
> @@ -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
