[PATCH 0/5] handle corruption in patch-delta

From: Jeff King <peff@peff.net>
Date: Aug 30, 2018, 07:05 UTC
Message-ID: <20180830070548.GA15081@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.

Based on my earlier observations, here's a replacement patch series I came up with. It has:

  [1/5]: test-delta: read input into a heap buffer

    A simpler replacement for your patch 2 which avoids portability
    issues.

  [2/5]: t5303: test some corrupt deltas

    A more complete set of boundary tests based on the 4 cases I laid
    out, plus the cp_size problem I found.

  [3/5]: patch-delta: fix oob read

    Your actual fix.

  [4/5]: patch-delta: consistently report corruption

    Your related trailing-garbage fix. I split this into two in order to
    better demonstrate the cases this part covers.

  [5/5]: patch-delta: handle truncated copy parameters

    My fix for the cp_size read.

I hope you don't mind me hacking up your patches a bit. Thanks again for your original report and patch.

-Peff