CVE-2022-28041: Additional stb_image fixes for bugs from ossfuzz and issues 1289, 1291, 1292, and 1293 by NeilBickford-NV · Pull Request #1297 · nothings/stb

Hi stb maintainers! I went through the first ten ossfuzz issues (up to and including ossfuzz issue 38394) and put together fixes for them. About half were duplicates of previous issues, but three were new and are fixed in this PR.

I also went through the test files in issues #1289, #1291, #1292, and #1293 and fixed ASan+UBSan reports from loading those issues’ repro cases which weren’t already fixed by #1223 or #1230.

Here are the bugs, some quick analyses, and their fixes:

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22620&q=proj%3Dstb&can=2 :

This is a PNM file with specifies a width of 3333333333. Parsing this integer overflows a signed 32-bit integer.

The fix I chose isn’t the most elegant: I check to see if value * 10 + (*c - 0) would overflow a signed int, and then propagate an error up two levels. This also makes loading a 0-width or 0-height PNM file more explicitly an error (previously, this would produce an error for other reasons). However, I can think of a few other good ways of fixing this (e.g. simplifying the condition by limiting the maximum size a bit further, or changing value to an unsigned int and letting later error-handing handle it.) Please let me know if you would prefer any of these other ways!

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32803&q=proj%3Dstb&can=2 :

The reproducer file here is a PNG file with a second IDAT chunk with a c.length of 0x68088c86. The IDAT handler increases idata_limit to fit this amount of memory, and then reallocates space for 0xcce48000 bytes, causing an ossfuzz out-of-memory error (since this is greater than ossfuzz’s limit of 2560 MB). The stbi__getn call later fails.

I’m not totally sure about my fix for this one, which is to reject IDAT sections larger than 1 GiB (the ossfuzz OOM threshold is 2560 MB). Comparing against the compressed data size would be ideal - but we don’t have the data size in a callback context, right?

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36193&q=proj%3Dstb&can=2 :

The reproducer here is a JPEG file that manages to set j->code_bits to -8 in stbi__grow_buffer_unsafe! This results in a left shift of 32 bits on a 32-bit type, which is undefined behavior. This happens by getting into a situation where code_bits is 1, but the combined length s to read is 9.

This PR checks for this situation and returns an error if it happens.

Issue #1289:

Will post analysis in issue. This PR avoids this crash by bounds-checking c.

Issue #1291, file id_000154,sig_06,src_002783+000969,time_39921237,op_splice,rep_4,trial_1492432:

Will post analysis in issue. This pull request adds checks to both the h->size[k++] loop in stbi__build_huffman and the “DHT - define huffman table” code to verify that writes are in-bounds.

Since creating this pull request, I also put together fixes for several other files in issues 1292 and 1293:

Issue #1292, files id_000118,sig_06,src_003303,time_27343468,op_havoc,rep_16,trial_1496823 and id_000130,sig_06,src_002266+002478,time_16238914,op_splice,rep_16,trial_1492432:

These files all produce signed integer overflows (which are undefined behavior) when decoding the DC coefficient of a JPEG block or when accumulating delta codes (i.e. in the lines data[0] = (shirt) (dc * dequant[0]);, data[0] = (short) (dc * (1 << j->succ_low));, and dc = j->img_comp[b].dc_pred + diff;). This MR adds two functions, stbi__addints_valid and stbi__mul2shorts_valid, that check for overflow of addition of two 32-bit signed ints and multiplication of two signed shorts, respectively; if there’s overflow here, we return an error. These two functions are maybe a bit more complex than ideal; maybe wrapping is OK here (in which case, casting to a signed type would make the overflow defined)?

Issue #1293, files id_000115,sig_06,src_003085,time_21982593,op_havoc,rep_8,trial_1497271, id_000157,sig_06,src_003484,time_46825823,op_havoc,rep_16,trial_1492432, and id_000212,sig_06,src_005033,time_18133274,op_havoc,rep_4,trial_1499760:

These are all relatively similar to ossfuzz issue 36193 above: the decoder exhausts its code buffer, then calls stbi__grow_buffer_unsafe (which I think implements NEXTBIT in ITU-T81), which immediately reaches a fill followed by a marker, leaving code_bits at 0. Any of a number of functions try to read some number of code bits from stbi__jpeg and decrease stbi__jpeg::code_bits, making it negative (e.g. -8 or -9)! The next time control returns to stbi__grow_buffer_unsafe and a byte is read that’s not 0xff, the j->code_buffer |= b << (24 - j->code_bits); line left shifts by more than 32 bits.

I sort of manually patched each of the places where stbi__grow_buffer_unsafe(j); can be called followed by decreasing j->code_bits by making sure that the required number of bits were actually read. If not enough bits were read, I have the code act as if all 0s were read or return an error. This could be better - perhaps the best solution would be to implement NEXTBIT more closely and handle error propagation here? - but seems to work.

Thanks!