Additional stb_image fixes for bugs from ossfuzz and issues 1289, 1291, 1292, and 1293

[NBickford-NV]

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 stbijpeg and decrease stbijpeg::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!

[NBickford-NV]

By the way, I've made another branch at https://github.com/NeilBickford-NV/stb/tree/neilbickford/all-fixes which contains the fixes from this pull request and the other pull requests to fix fuzzer-found issues (#1223 and #1230), in case that's easier to look at or to merge in when the time comes to process everything. Thanks again!

[NBickford-NV]

I've added some additional checks to fix issues reported in bugs 1292 and 1293; looks like there could still be at least one out-of-memory issue from fuzz-testing with libFuzzer.

[NBickford-NV]

Update: Looks like the file causing last comment's out-of-memory issue was in fact correctly reaching the 2^30 byte PNG IDAT section limit implemented in this merge request; the fuzzer set c.length to exactly 2^30 - 1, resulting in an allocation of 2^32 bytes. This went just over libFuzzer's default limit of 2048 Mb of memory, although it didn't go much higher than that. I guess checking against the file size when we're not reading data from streams might be a good idea, though!

[NBickford-NV]

One more change in this latest commit: Running libFuzzer on stbi_read_fuzzer locally, I was seeing occasional out-of-bounds errors from UndefinedBehaviorSanitizer when accessing stbi__bmask in the line

   STBI_ASSERT((((j->code_buffer) >> (32 - h->size[c])) & stbi__bmask[h->size[c]]) == h->code[c]);

Unlike the previous bugs, I wasn't able to reproduce this one when loading an individual file, since it looks like it's due to reading from an uninitialized value. Specifically, it looks like it's possible for control to reach stbi__jpeg_huff_decode without setting the values inside the stbi__huffman struct, which would normally be set inside stbi__build_huffman (called from stbi__process_marker). So, my suspicion is that this was due to how stbi__jpeg_load doesn't clear the stbi__jpeg struct it allocates; malloc probably returned a memory block that was written by an earlier file during fuzz testing, which just happens to have the right values where calling stbi__jpeg_huff_decode without building the Huffman tables first indexes out of bounds.

Calling memset(j, 0, sizeof(stbi__jpeg)); after allocating the stbi__jpeg struct appears to fix these intermittent errors. It's possible I've added it in more places than strictly necessary: it might not be necessary to zero-initialize it in stbi__jpeg_test and stbi__jpeg_info, and it might only be necessary to zero-initialize stbi__jpeg::huff_dc and stbi__jpeg::huff_ac. I decided to err on the side of caution here, but please let me know if you would like any modifications!

I've now been able to run libFuzzer through about 30 million test cases (on a branch with all 3 pull requests combined) without crashes, but will likely run libFuzzer for a few more days to see if it finds anything.

[nothings]

it's perfectly fine to zero-initialize excessively

[NBickford-NV]

Thanks, that's good to hear! I can have the fuzzer search for other cases where it winds up reading from uninitialized values and add fixes for those, if that would be good?

[NBickford-NV]

Also, looks like the ossfuzz continuous integration check ran into the PNM crash from #1225 again - should I see if I can replace these 3 pull requests with a single pull request that combines them all? I think ossfuzz may well run cleanly on that. Thanks again!

[nothings]

should I see if I can replace these 3 pull requests with a single pull request that combines them all?

Whatever is easiest or best for you; I tend to process PRs in batches anyway, so it doesn't matter too much to me.

[NBickford-NV]

OK! I don't have too much of a preference either way and the PRs don't conflict with each other, so I'll leave it as is.

[rygorous]

Code LGTM, changes merged into dev branch, will be in the next release.