selmf / unarr

A decompression library for rar, tar, zip and 7z archives
GNU Lesser General Public License v3.0
73 stars 13 forks source link

fix memmove out-of-bounds access #5

Closed comicfans closed 5 years ago

comicfans commented 5 years ago

found by fuzzer + address sanitizer .

selmf commented 5 years ago

Great catch! I'd like some modifications before merging though:

~~There is some existing code in the rar parser you can reuse for this (I will check this once I'm home). I'd also like to cross-check the bug with the zip specification to make sure the solution is the best one, so you don't need to rewrite your patch right away ;)~~

Edit: After spending some more time on this I think there is an easier solution to this problem. The code only ever uses the first 4 bytes copied by memmove, everything else is overwritten when the next chunk of data is read. So to fix the problem it should be enough to change the memmove call to:

memmove(data, data + count - 4, 4);

selmf commented 5 years ago

@comicfans could you test if the changed memmove call fixes the problem for you? If yes I need to know if you want to update you PR or if I just should push the fix.

comicfans commented 5 years ago

Hi @selmf , I've tested this by previously crashed input, confirm it worked. please just push your fix