Issue 622 in webp: BuildHuffmanTable check bounds of offset[] and sorted[] array

[jongr… via monorail]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 622 by jongr...@gmail.com: BuildHuffmanTable check bounds of offset[] and sorted[] array
https://bugs.chromium.org/p/webp/issues/detail?id=622

This is a suggestion, the code doesn't cover with 100% certainty that two arrays are not overrun.

I added a check at compiler time to check gcc could satisfy itself the index used to access offset[] and sorted[] array were never overrun, but the compiler couldn't. It's easy enough to add the check, so that is what I am proposing. Most 0-day exploits aren't known, it's safer just to defend against them than wait for them to occur.

The code is roughly from line 150 in webp_test/src/utils.c

1. I will attach a file which can be compared against latest git source to show the changes.
2. The change I added checks at *compile* time that the array is not overrun, in this situation the compiler can't figure out if the index will ever overrun.
3. In this case, a check on the index should be added, I put the example in

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


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

Latest git. Ubuntu GCC.

The output below includes my local changes. These are just my suggestions, to make the code bullet proof, 100% secure, the compiler is then 100% certain the index never overruns.


$ make -f makefile.unix
gcc -I. -Isrc/ -Wall -O3 -DNDEBUG -DWEBP_HAVE_PNG -DWEBP_HAVE_JPEG -DWEBP_HAVE_TIFF -I/usr/local/include -DWEBP_USE_THREAD -fvisibility=hidden -Wextra -Wold-style-definition -Wmissing-prototypes -Wmissing-declarations -Wdeclaration-after-statement -Wshadow -Wformat-security -Wformat-nonliteral -c src/utils/huffman_utils.c -o src/utils/huffman_utils.o
src/utils/huffman_utils.c: In function ‘BuildHuffmanTable’:
src/utils/huffman_utils.c:80:66: error: call to ‘_stop_compile’ declared with attribute error: build stopped
80 | #define compile_assert(condition, description) {if(!condition) { _stop_compile();}}
| ^~~~~~~~~~~~~~~
src/utils/huffman_utils.c:151:9: note: in expansion of macro ‘compile_assert’
151 | compile_assert((symbol_code_length<MAX_ALLOWED_CODE_LENGTH + 1), "index must be within the capacity of the offset array");
| ^~~~~~~~~~~~~~
src/utils/huffman_utils.c:80:66: error: call to ‘_stop_compile’ declared with attribute error: build stopped
80 | #define compile_assert(condition, description) {if(!condition) { _stop_compile();}}
| ^~~~~~~~~~~~~~~
src/utils/huffman_utils.c:146:9: note: in expansion of macro ‘compile_assert’
146 | compile_assert(((offset[symbol_code_length]+1)<MAX_ALLOWED_CODE_LENGTH + 1), "offset index must be within the capacity of the sorted array");
| ^~~~~~~~~~~~~~
make: *** [makefile.unix:397: src/utils/huffman_utils.o] Error 1


Wanted to run the fuzzer, it gave an error, happy to do so if someone can tell me how to.


$ make -C fuzzer -f makefile.unix
make: Entering directory '/home/jonny/code/webp/webp_test/tests/fuzzer'
clang -fsanitize=fuzzer -I../../src -I../.. -Wall -Wextra -c -o advanced_api_fuzzer.o advanced_api_fuzzer.c
make: *** No rule to make target '../../src/mux/libwebpmux.a', needed by 'advanced_api_fuzzer'. Stop.

Attachments:
huffman_utils.c 11.6 KB

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jz… via monorail]

Comment #1 on issue 622 by jz...@google.com: BuildHuffmanTable check bounds of offset[] and sorted[] array
https://bugs.chromium.org/p/webp/issues/detail?id=622#c1

> The output below includes my local changes. These are just my suggestions, to make the code bullet proof, 100% secure, the compiler is then 100% certain the index never overruns.

If the size isn't guaranteed or not checked at a higher level then updates might make sense. These are performance critical areas, so often either known assumptions are used or validation happens at a higher level to allow the low level functions to be as fast as possible.

If you would like to send a change so it can be discussed, please have a look at our contributors guide [1].

> Wanted to run the fuzzer, it gave an error, happy to do so if someone can tell me how to.

Run `make -f makefile.unix all` before trying to compile the fuzzers.

[1] https://chromium.googlesource.com/webm/libwebp/+/05c469843ce3947a63f5ac7ac2c3baa5df2c167d/CONTRIBUTING.md

[j… via monorail]

Comment #2 on issue 622 by j...@jguk.org: BuildHuffmanTable check bounds of offset[] and sorted[] array
https://bugs.chromium.org/p/webp/issues/detail?id=622#c2

VP8LBuildHuffmanTable() calls BuildHuffmanTable() with either a 'sorted' buffer on the stack, or a heap allocated 'sorted'

The compiler can't see the sorted[] array accesses are always within bounds, the change I propose is to add a new parameter to pass the sorted_count :

static int BuildHuffmanTable(HuffmanCode* const root_table, int root_bits,
const int code_lengths[], int code_lengths_size,
uint16_t sorted[], const size_t sorted_count)

Then the offset[symbol_code_length] index into sorted[] via the array subscript should be checked before it is accessed.


I can check how to submit it formally.

[jz… via monorail]

Updates:
Owner: vra...@google.com

Comment #3 on issue 622 by jz...@google.com: BuildHuffmanTable check bounds of offset[] and sorted[] array
https://bugs.chromium.org/p/webp/issues/detail?id=622#c3

(No comment was entered for this change.)

[jgran… via monorail]

Comment #4 on issue 622 by jgran...@gmail.com: BuildHuffmanTable check bounds of offset[] and sorted[] array
https://bugs.chromium.org/p/webp/issues/detail?id=622#c4

Linking

https://chromium-review.googlesource.com/c/webm/libwebp/+/4979970