5 ms·
The original commit in question: https://github.com/webmproject/libwebp/commit/f75dfbf23d1df1be52350b1a6fc5cfa6c2194499 https://github.com/webmproject/libwebp/c
by lifthrasiir 3y ago
The original commit in question: https://github.com/webmproject/libwebp/commit/f75dfbf23d1df1be52350b1a6fc5cfa6c2194499 https://github.com/webmproject/libwebp/commit/f75dfbf23d1df1...
The commit that fixes this bug: https://github.com/webmproject/libwebp/commit/902bc9190331343b2017211debcec8d2ab87e17a https://github.com/webmproject/libwebp/commit/902bc919033134...
The original commit optimizes a Huffman decoder. The decoder uses a well-known optimization: it reads N bits in advance and determines how many bits have to be actually consumed and which symbol should be decoded, or, if it's an N-bit prefix of multiple symbols, which table should be consulted for remaining bits.
The old version did use lookup tables for short symbols, but longer symbols needed a graph traversal. The new version improved this by using an array of lookup tables. Each entry contains (nbits, value) where `nbits` is # bits to be consumed and `value` is normally a symbol, but if `nbits` exceeds N `value` is interpreted as a table index and `nbits` is reinterpreted as the longest code length in that subtree. So each subsequent table should have `2^(nbits - N)` entries (the root table is always fixed to 2^N entries).
The new version calculated the maximum number of entries based on the number of symbols (kTableSize). Of course, the Huffman tree comes from an untrusted source and you can easily imagine the case where `nbits` is very big. VP8 Lossless specifically allows up to 15 bits, so the largest possible table has 2^N + 2^15 entries when every single LUT is mapped to its own secondary table, and doing this doesn't need that many symbols (you only need 16-N symbols for each table). Amusingly enough the code itself had a mode where only the table size is calculated (VP8LBuildHuffmanTable called with `root_table == NULL`), but it wasn't somehow unused and the fixed maximum size was assumed. So if the Huffman tree was crafted in the way that maximizes the number of entries, it will overflow the allocation.
To be fair, I can see why this happened; the Huffman decoding step is one of the most computationally intensive part of many compression format and any small improvement matters. The Huffman decoder optimization described above is well known, but the longer code case is commonly considered less important to optimize because longer code should rarely appear in general. The original commit message refuted this, and was able to be merged. I'm not even sure that a memory safe language could have prevented this; this is a rare case where you actively want to avoid the overflow check [1].
[1] I should note that the memory corruption occurs during the table construction, which is not a tight loop, so a partial overflow check would be very helpful. The actual fix never altered the `ReadSymbol` function at all! But the safety of the tight loop should be still justified, so the wrong justification can ruin everything.
- avg_dev 3y agoThanks for your explanation. I’m not a C programmer over the past decade+ and was never very good at it anyway. But I was thinking, based on your description, I agree that a bounds check would have caught the issue, but I’m also curious if an automated test could have been constructed to catch this sort of thing. I personally work with code where you could factor out some calculations to their own function and test them in isolation. Perhaps that would be tough to do here because of performance; I am really not sure.
- danielheath 3y agoI’m quite surprised coverage-guided fuzzing didn’t find it almost immediately, to the point that I wonder if it had even been set up.
- lifthrasiir 3y agoI believe it is possible to construct a dedicated automated test, but a normal fuzzing wouldn't be very effective in this case because you need some non-trivial logic to construct a Huffman tree with very large LUT. It's like a condition like phi(input) equals to the specific number [1]; you can construct an input by hand, but fuzzers generally have no idea to approach that. I do think this was found by fuzzing, but it is entirely reasonable that fuzzers took much time to trigger the actual memory corruption. That said, this particular bug could have been avoided by a careful coding; VP8LBuildHuffmanTable could have received the maximum size of `root_table`, just like snprintf. It still would have needed much time to find but at least it could never be a security bug. [1] https://math.stackexchange.com/questions/265397/inversion-of-the-euler-totient-function https://math.stackexchange.com/questions/265397/inversion-of...
- avg_dev 3y agoI guess my thinking was based off of this: > Each entry contains (nbits, value) where `nbits` is # bits to be consumed and `value` is normally a symbol, but if `nbits` exceeds N `value` is interpreted as a table index and `nbits` is reinterpreted as the longest code length in that subtree. So each subsequent table should have `2^(nbits - N)` entries (the root table is always fixed to 2^N entries). Given that, wouldn't it be only natural to construct a test where nbits exceeds N, so that you exercise the code where `value` was being interpreted as an index, and `nbits` as the longest code length in that subtree? Hmm, I guess that this could have been done and perhaps was done (I took a quick glance at the code and didn't really grok it in a couple mins), and the issue would be that even if you did do what I suggest, you would likely not exceed the maximum fixed size with your crafted test, and thus not catch the bug.