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.
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
