Closed Bug 1166252 Opened 6 years ago Closed 6 years ago
QCMS crash OOB read at src/chain
6.03 KB, text/x-csrc
546.26 KB, application/octet-stream
546.30 KB, application/octet-stream
5.98 KB, patch
|Details | Diff | Splinter Review|
2.19 KB, patch
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.152 Safari/537.36 Steps to reproduce: Run repro with the attached samples. Chromium issue: https://code.google.com/p/chromium/issues/detail?id=487284 Actual results: ================================================================= ==24625==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000006fb0 at pc 0x000000ef4655 bp 0x7fff6dcf7f60 sp 0x7fff6dcf7f58 READ of size 4 at 0x602000006fb0 thread T0 #0 0xef4654 in qcms_transform_module_clut third_party/qcms/src/chain.c:211:21 #1 0xef32cd in qcms_modular_transform_data third_party/qcms/src/chain.c:971:17 #2 0xef2dd7 in qcms_chain_transform third_party/qcms/src/chain.c:984:16 #3 0xeea2d6 in qcms_transform_precacheLUT_float third_party/qcms/src/transform.c:1140:9 #4 0xeed50e in qcms_transform_create third_party/qcms/src/transform.c:1267:28 #5 0x4e21b7 in main third_party/qcms/qcms_parse.cc:89:31 0x602000006fb1 is located 0 bytes to the right of 1-byte region [0x602000006fb0,0x602000006fb1) allocated by thread T0 here: #0 0x4cec4b in __interceptor_malloc third_party/llvm/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:40:3 #1 0xef12aa in qcms_modular_transform_create_lut third_party/qcms/src/chain.c:679:9 #2 0xef095e in qcms_modular_transform_create_input third_party/qcms/src/chain.c:713:19 #3 0xef2f05 in qcms_modular_transform_create third_party/qcms/src/chain.c:898:16 #4 0xef2da8 in qcms_chain_transform third_party/qcms/src/chain.c:982:50 #5 0xeea2d6 in qcms_transform_precacheLUT_float third_party/qcms/src/transform.c:1140:9 #6 0xeed50e in qcms_transform_create third_party/qcms/src/transform.c:1267:28 #7 0x4e21b7 in main third_party/qcms/qcms_parse.cc:89:31 SUMMARY: AddressSanitizer: heap-buffer-overflow third_party/qcms/src/chain.c:211:21 in qcms_transform_module_clut Shadow bytes around the buggy address: 0x0c047fff8da0: fa fa fd fd fa fa fd fd fa fa 00 00 fa fa fd fd 0x0c047fff8db0: fa fa fd fd fa fa 00 00 fa fa fd fd fa fa fd fd 0x0c047fff8dc0: fa fa 00 00 fa fa fd fd fa fa fd fd fa fa 00 00 0x0c047fff8dd0: fa fa fd fd fa fa fd fd fa fa 00 00 fa fa 00 00 0x0c047fff8de0: fa fa fd fa fa fa fd fd fa fa fd fd fa fa fd fd =>0x0c047fff8df0: fa fa 00 00 fa fafa fa fa 00 00 fa fa 00 00 0x0c047fff8e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==24625==ABORTING
Status: UNCONFIRMED → NEW
Ever confirmed: true
I believe this code is pref'd off in the version that we ship.
Kamil, can you try running the attached cases and see if it repros?
I think this is a dupe of bug 1132468. The profile causes us to allocate a zero length clut because we were missing a branch for that. The bug description shows that allocate stack with size 0.
Now that bug 1132468 is fixed, can we check if this is gone?
I'm almost positive it's fixed but I haven't had time to build the test case. I'll try and do it soon!
This requires a 500 KB profile which is too large for the value it would give us, just makes it easy to test this in the future if we need.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8650169 - Flags: checkin-
Attachment #8650170 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8650170 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Fairly easily for someone willing to read the documented binary format of ICC color profiles. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The asserts do. Which older supported branches are affected by this flaw? All the way back to Firefox 4 however it's behind a flag: gfx.color_management.enablev4 If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? There should be no conflict or just trivial ones. How likely is this patch to cause regressions; how much testing does it need? Unlikely, just adding a check to reject the profiles.
Attachment #8650170 - Flags: sec-approval?
Because this requires a non-default pref setting I'm lowering the severity to moderate. Do we have plans to enable this any time soon? I found blog posts about trying it out going back years. Couldn't find any telemetry about how many people might have this enabled. Seems a popular on-line recommendation for photo buffs but probably not all that mainstream. But that's just guessing on my part.
Whiteboard: requires gfx.color_management.enablev4 set to true
Comment on attachment 8650170 [details] [diff] [review] patch sec-approval = dveditz
Attachment #8650170 - Flags: sec-approval? → sec-approval+
(In reply to Daniel Veditz [:dveditz] from comment #11) > Do we have plans to enable this any time soon? Not at the moment. The code would need a SIMD paths to have acceptable performance and it could probably use some additional security review and fuzzing.
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/419ade49d346f41632f8ad4478edc2b1ecac5825 changeset: 419ade49d346f41632f8ad4478edc2b1ecac5825 user: Benoit Girard <firstname.lastname@example.org> date: Tue Aug 25 15:48:55 2015 -0400 description: Bug 1166252 - Reject lut8/16Type with empty CLUT grid. r=jrmuizel
Benoit, do you need any help testing this?
No, this should be fine. I already checked it against the iccv4 test page.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Benoit, are you planning to request the uplift to aurora/beta? Thanks
Comment on attachment 8650170 [details] [diff] [review] patch Since it's behind a preference I don't think we *need* to uplift this, but it's just a simple check to reject a bad profile (and debug-only assert) so I think it's worth landing at least on aurora, perhaps on beta too. Approval Request Comment [Feature/regressing bug #]: Since Firefox 4, we requires a feature preference to be turned on. [User impact if declined]: None if the preference isn't flipped. OOB read using a carefully crafted profile. [Describe test coverage new/current, TreeHerder]: Test case isn't landed because it requires a 500 KB profile. [Risks and why]: Unlikely worse case with this patch we could reject valid profiles and regress color management. [String/UUID change made/needed]: None
Comment on attachment 8650170 [details] [diff] [review] patch OK, let's take it only on aurora then. Merci!
Whiteboard: requires gfx.color_management.enablev4 set to true → [post-critsmash-triage] requires gfx.color_management.enablev4 set to true
Whiteboard: [post-critsmash-triage] requires gfx.color_management.enablev4 set to true → [post-critsmash-triage] requires gfx.color_management.enablev4 set to true[b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.