-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Additional stb_image fixes for bugs from ossfuzz and issues 1289, 1291, 1292, and 1293 #1297
Additional stb_image fixes for bugs from ossfuzz and issues 1289, 1291, 1292, and 1293 #1297
Conversation
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! |
2eca8fb
to
2cdd738
Compare
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. |
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 |
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
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 Calling 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. |
it's perfectly fine to zero-initialize excessively |
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? |
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! |
Whatever is easiest or best for you; I tend to process PRs in batches anyway, so it doesn't matter too much to me. |
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. |
Mainstream pull requests: nothings/stb#1230 nothings/stb#1223 nothings/stb#1297 Related mainstream issue tickets: nothings/stb#1224 nothings/stb#1225 nothings/stb#1289 nothings/stb#1291 nothings/stb#1292 nothings/stb#1293
…unk sizes to fix an OOM issue, and check for a situation where a sequence of bad Huffman code reads could result in a left shift by a negative number.
…here stbi__grow_buffer_unsafe doesn't read all bits required.
51e438b
to
5cfc2a7
Compare
Code LGTM, changes merged into dev branch, will be in the next release. |
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 anunsigned 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 increasesidata_limit
to fit this amount of memory, and then reallocates space for0xcce48000
bytes, causing an ossfuzz out-of-memory error (since this is greater than ossfuzz's limit of 2560 MB). Thestbi__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 instbi__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 instbi__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));
, anddc = j->img_comp[b].dc_pred + diff;
). This MR adds two functions,stbi__addints_valid
andstbi__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 tostbi__grow_buffer_unsafe
and a byte is read that's not 0xff, thej->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 decreasingj->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!