On Thu, 30 Aug 2018, Jeff King wrote: > 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. Looks good to me (feels like traveling back in time). Reviewed-by: Nicolas Pitre <nico@xxxxxxxxxxx> > > -Peff >
