Monorail Project: webp Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner: ----
Closed: Nov 2015



Sign in to add a comment
out-of-bounds pointers
Reported by john.reg...@gmail.com, Nov 18 2015 Back to list
What steps will reproduce the problem?

in two code locations, libwebp will compute an out of bounds pointer (but not dereference it). it is clear that the code does this intentionally, but please keep in mind that this is undefined behavior in C and could lead to problems in practice on a platform where the allocated block is bumped up against the end of the address space, in which case the invalid pointer would wrap around, preventing a safety check from firing.

the first problem is the computation of (br->buf_ + sizeof(lbit_t)) at src/util/bit_reader_inl.h:61, where the summed pointer is up to 8 past the end of the array. you can confirm by adding a printf and then running this command:

  examples/dwebp id:001880,src:001631,op:havoc,rep:16.webp

the other UB is the computation of part_end at src/dec/vp8.c:205. repro by adding a printf and running:

  examples/dwebp id:002329,src:001680,op:havoc,rep:4,+cov.webp

here part_end is up to 8331300 beyond buf_end (and could be worse since psize is 24 bits) which seems like it might be a real problem.

I believe the issue is analogous to the issue discussed here:

  https://bugzilla.mozilla.org/show_bug.cgi?id=770534
  https://bugzilla.mozilla.org/show_bug.cgi?id=927687

and there is some additional good discussion about how not to fix this kind of problem here:

  http://lwn.net/Articles/278137/

The webp files in question can be found here:

  http://lcamtuf.coredump.cx/afl/demo/webp/full/

What is the expected output? What do you see instead?

expect pointers to remain in bounds, or to point at most one element past the end of a valid storage block

What version of the product are you using? On what operating system?

libwebp's master branch as of Nov 17 on Ubuntu 14.04

Please provide any additional information below.


 
Project Member Comment 1 by pascal.m...@gmail.com, Nov 18 2015
thanks for the very documented report John. Having a look.
Project Member Comment 2 by pascal.m...@gmail.com, Nov 18 2015
proposed patch: https://chromium-review.googlesource.com/312494

while the second case (vp8.c:205) wasn't speed-critical and rather straightforward to fix, the first one (bit_reader_inl.h:61) needed a check for potential speed regression.
-> Whereas x86 is ok, i see a 2% speed drop on ARM.

-> needs more investigation.


Since the overflow is not by much, is it possible to just make that buffer 8 bytes bigger than it needs to be? Then you don't need to touch the performance-sensitive code.
There's another file in this batch that makes the 2nd pointer OOB by just about the full 16 MB:

id:001596,src:001512,op:havoc,rep:4.webp

The "buf_end - part_start" fix requires (on 32-bit platforms) that the difference does not exceed 2^31. Is this guaranteed to be the case?
Project Member Comment 6 by pascal.m...@gmail.com, Nov 19 2015
re: comment #3: unfortunately, the buffer is user-supplied in most cases, so we can't pad it at will

re: comment #5: the 2^31 bound is pretty much ensured by the format and early checks in the code, but i will cross-check. Patch update to follow.
Project Member Comment 7 by pascal.m...@gmail.com, Nov 21 2015
Status: Fixed
The patch https://chromium-review.googlesource.com/#/c/312494 was submitted.
Please don't hesitate to re-open this bug if you spot other risky pointer calculations.

Thanks!
Sign in to add a comment