Closed Bug 1464039 (CVE-2018-12366) Opened 2 years ago Closed 2 years ago

Heap-buffer-overflow READ 4 · qcms_transform_module_clut_only

Categories

(Core :: GFX: Color Management, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 61+ fixed
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: Alex_Gaynor, Assigned: nical)

References

Details

(Keywords: csectype-bounds, oss-fuzz, sec-low, Whiteboard: [disclosure deadline August 22][adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

Note: This was found by Google's OSS-Fuzz and as a result there is a 90 day disclosure deadline.

The attachment can be used as an input to the fuzzer: https://github.com/google/oss-fuzz/blob/master/projects/qcms/fuzz.cc

[Environment] ASAN_OPTIONS = redzone=128:strict_string_check=1:strict_memcmp=1:allow_user_segv_handler=0:allocator_may_return_null=1:handle_sigfpe=1:handle_sigbus=1:detect_stack_use_after_return=1:alloc_dealloc_mismatch=0:print_scariness=1:strip_path_prefix=/workspace/:max_uar_stack_size_log=16:quarantine_size_mb=256:detect_odr_violation=0:handle_sigill=1:allocator_release_to_os_interval_ms=500:coverage=0:use_sigaltstack=1:fast_unwind_on_fatal=0:detect_leaks=1:print_summary=1:handle_abort=1:check_malloc_usable_size=0:detect_container_overflow=1:symbolize=0:handle_segv=1

==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x609000000074 at pc 0x000000531f06 bp 0x7ffc9190bbc0 sp 0x7ffc9190bbb8
	READ of size 4 at 0x609000000074 thread T0
	SCARINESS: 27 (4-byte-read-heap-buffer-overflow-far-from-bounds)
	#0 0x531f05 in qcms_transform_module_clut_only /src/firefox/gfx/qcms/chain.c:153:21
	#1 0x53005d in qcms_modular_transform_data /src/firefox/gfx/qcms/chain.c:975:17
	#2 0x52faaa in qcms_chain_transform /src/firefox/gfx/qcms/chain.c:988:16
	#3 0x540849 in qcms_transform_precacheLUT_float /src/firefox/gfx/qcms/transform.c:1182:9
	#4 0x543ea8 in qcms_transform_create /src/firefox/gfx/qcms/transform.c:1248:28
	#5 0x54b859 in transform(_qcms_profile*, _qcms_profile*, int) /src/fuzz.cc:24:31
	#6 0x54b772 in LLVMFuzzerTestOneInput /src/fuzz.cc:64:3
	#7 0x54998f in ExecuteFilesOnyByOne(int, char**) /src/libfuzzer/afl/afl_driver.cpp:301:5
	#8 0x549f3e in main /src/libfuzzer/afl/afl_driver.cpp:339:12
	#9 0x7f3d3e62a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
	#10 0x41c978 in _start
	
	0x609000000074 is located 12 bytes to the left of 1-byte region [0x609000000080,0x609000000081)
	allocated by thread T0 here:
	#0 0x4ea807 in malloc _asan_rtl_
	#1 0x52e3b7 in qcms_modular_transform_create_mAB /src/firefox/gfx/qcms/chain.c:584:10
	#2 0x52d203 in qcms_modular_transform_create_input /src/firefox/gfx/qcms/chain.c:723:19
	#3 0x52fbf8 in qcms_modular_transform_create /src/firefox/gfx/qcms/chain.c:902:16
	#4 0x52fa71 in qcms_chain_transform /src/firefox/gfx/qcms/chain.c:986:50
	#5 0x540849 in qcms_transform_precacheLUT_float /src/firefox/gfx/qcms/transform.c:1182:9
	#6 0x543ea8 in qcms_transform_create /src/firefox/gfx/qcms/transform.c:1248:28
	#7 0x54b859 in transform(_qcms_profile*, _qcms_profile*, int) /src/fuzz.cc:24:31
	#8 0x54b772 in LLVMFuzzerTestOneInput /src/fuzz.cc:64:3
	#9 0x54998f in ExecuteFilesOnyByOne(int, char**) /src/libfuzzer/afl/afl_driver.cpp:301:5
	#10 0x549f3e in main /src/libfuzzer/afl/afl_driver.cpp:339:12
	#11 0x7f3d3e62a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
	
	SUMMARY: AddressSanitizer: heap-buffer-overflow (/mnt/scratch0/clusterfuzz/slave-bot/builds/clusterfuzz-builds-afl_qcms_6de1215797ff093b4b972acc94def85d38f18127/revisions/fuzz+0x531f05)
	Shadow bytes around the buggy address:
	0x0c127fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	0x0c127fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	0x0c127fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	0x0c127fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	0x0c127fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	=>0x0c127fff8000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa
	0x0c127fff8010: 01 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c127fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c127fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c127fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c127fff8050: 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
	Freed heap region:       fd
	Stack left redzone:      f1
	Stack mid redzone:       f2
	Stack right redzone:     f3
	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
	Shadow gap:              cc
	==1==ABORTING
Jeff: can you take a look here? We're assuming this is reading (only) bad color values (float in this case) and not anything pointery, thus sec-moderate. If it's worse let us know.
QA Whiteboard: [disclosure deadline August]
Flags: needinfo?(jmuizelaar)
IIUC, Jeff is away for a few weeks. David, can you please help find an assignee for this?
Flags: needinfo?(dbolter)
Bas I think Nical had his hands in qcms recently? Could he take a look (since Jeff is away)?
Flags: needinfo?(dbolter) → needinfo?(bas)
Flags: needinfo?(bas) → needinfo?(nical.bugzilla)
Flags: needinfo?(jmuizelaar)
I'm happy to look at the stream of qcms fuzzing bugs as a background task if it's OK that I take a while to get through them.
Flags: needinfo?(nical.bugzilla)
A 90 day disclosure deadline means that this will be disclosed before we ship Fx62 in September, FWIW.
Whiteboard: [disclosure deadline August 22]
(In reply to Nicolas Silva [:nical] from comment #4)
> I'm happy to look at the stream of qcms fuzzing bugs as a background task if
> it's OK that I take a while to get through them.

Thanks Nical. If someone else can steal it from you we'll update the bug.
Assignee: nobody → nical.bugzilla
I had a look and have some ideas but it's hard to know for sure without being able to inspect the values of various things, is there a documentation somewhere about how to build and run the generated test case?
Flags: needinfo?(agaynor)
https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md describes how to reproduce the bug in the OSS-Fuzz infrastructure. Let me know if that doesn't work or you have any follow up questions.
Flags: needinfo?(agaynor)
Thanks Alex, I managed to run the test case in the docker environment and also managed to run the fuzzer with gdb using the command presented here: https://github.com/google/oss-fuzz/blob/master/docs/debugging.md#debugging-fuzzers-with-gdb but I have no clue how to actually run the test case in gdb (or any other tool really).
I also failed at building from a local checkout https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md#reproduce-using-local-source-checkout It seems to just build the docker image's source no matter where I point it to.

Sorry, I never had to deal with docker and the likes before.
FWIW, the workflow I use is |python infra/helper.py shell qcms| and then once in the shell |compile|. And then you can run teh fuzzer against a target with |/out/<fuzzer-name> <path to input to test>|.

If you can actually run the fuzzer under GDB, all you need to do is pass it the path to the input to test against.
Nicolas, did you have any luck reproducing this? We're building Fx61 RC1 on Monday, so we're getting low on time for a fix (and given the disclosure deadline, we're going to be needing an extension from Project Zero if this misses 61).
Flags: needinfo?(nical.bugzilla)
I've been able to confirm my suspicion of the problem, I should have a patch tomorrow.
Flags: needinfo?(nical.bugzilla)
The expectation that grid_size is never <= 0 was already in the code here: https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/gfx/qcms/chain.c#137 but it's only a debug assertion. I think that it makes most sense to handle this in the caller so that the other transform types don't get the same type of problematic input.

I verified that this fixes the test case in the oss-fuzz infra.
Attachment #8985672 - Flags: review?(matt.woodrow)
Attachment #8985672 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8985672 [details] [diff] [review]
Reject transforms if grid_size <= 0

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Hard. Invalid read into a float which doesn't affect control flow as far as I can tell.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply cleanly to old branches.

How likely is this patch to cause regressions; how much testing does it need?

Not very likely. I hope reftests should be enough but I don't actually know how well covered qcms is.
Attachment #8985672 - Flags: sec-approval?
sec-approval is not required for sec-moderate bugs (https://wiki.mozilla.org/Security/Bug_Approval_Process)
Comment on attachment 8985672 [details] [diff] [review]
Reject transforms if grid_size <= 0

This is sec-moderate, you can go ahead and land ASAP.
Flags: needinfo?(nical.bugzilla)
Attachment #8985672 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/dfcc5301e872
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please nominate this for Beta/ESR60/ESR52 uplift when you get a chance.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8985672 [details] [diff] [review]
Reject transforms if grid_size <= 0

Approval Request Comment
[Feature/Bug causing the regression]: It's been there for a long while.
[User impact if declined]: low risk potential security issue (invalid rid into a float which doesn't seem to affect control flow).
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Verified locally, hasn't been in nightly for very long, though.
[Needs manual test from QE? If yes, steps to reproduce]: Not sure how to reproduce this test case with actual web content so no.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: early out in a case where we would hit an debug assertion.
[String changes made/needed]: None.

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It's a simple enough patch that it should be in ESR if there is anything else that warrants a new release, but I would not do an ESR release specifically for this patch given that the bug is considered low risk.
Flags: needinfo?(nical.bugzilla)
Attachment #8985672 - Flags: approval-mozilla-esr60?
Attachment #8985672 - Flags: approval-mozilla-esr52?
Attachment #8985672 - Flags: approval-mozilla-beta?
Comment on attachment 8985672 [details] [diff] [review]
Reject transforms if grid_size <= 0

Fixes a qcms sec bug with an August disclosure deadline. Approved for 61.0rc1, ESR 60.1, and ESR 52.9.
Attachment #8985672 - Flags: approval-mozilla-esr60?
Attachment #8985672 - Flags: approval-mozilla-esr60+
Attachment #8985672 - Flags: approval-mozilla-esr52?
Attachment #8985672 - Flags: approval-mozilla-esr52+
Attachment #8985672 - Flags: approval-mozilla-beta?
Attachment #8985672 - Flags: approval-mozilla-beta+
Depends on: 1469264
Whiteboard: [disclosure deadline August 22] → [disclosure deadline August 22][adv-main61+][adv-esr52.9+][adv-esr60.1+]
I see this has a disclosure deadline. We've fixed for Firefox 61 so I assume there is no issue with disclosing in an advisory when that ships on June 26?
Flags: needinfo?(agaynor)
Flags: qe-verify-
Whiteboard: [disclosure deadline August 22][adv-main61+][adv-esr52.9+][adv-esr60.1+] → [disclosure deadline August 22][adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage]
Unfortunately The previous patch which probably already got uplifted to all of the trees has an issue: some transform functions don't use the table and grid size, and therefore are expected to work without valid values in these members.
As a result my patch conservatively rejected some valid transforms.

The bigger problem is that if firefox fails to load a color profile it will keep trying to load it over and over again thousands of times per second which makes the browser very slow (problematic if we are rejecting valid profiles).

My understanding is that this is only affecting a non-default set of prefs, but we still might want to uplift this fix.

This patch moves the code to reject invalid grid sizes to only the transforms that depend on it, and stores whether we failed to load the profile in a static boolean to avoid freezing the browser if we run into an actually invalid color profile.
Attachment #8986149 - Flags: review?(bas)
Attachment #8986149 - Attachment is obsolete: true
Attachment #8986149 - Flags: review?(bas)
Attachment #8986151 - Flags: review?(bas)
Al: We can always disclose whenever we want, OSS-Fuzz is never a limitation on how early we can fix/disclose, just a bound on when they'll disclose.
Flags: needinfo?(agaynor)
Attachment #8986151 - Flags: review?(bas) → review+
Attachment #8986151 - Flags: approval-mozilla-release?
Attachment #8986151 - Flags: approval-mozilla-esr60?
Attachment #8986151 - Flags: approval-mozilla-esr52?
Attachment #8986151 - Flags: approval-mozilla-beta?
Comment on attachment 8986151 [details] [diff] [review]
Only reject invalid grid sizes if teh transform uses the grid size.

approved for 61, 60.1esr, 52.9esr
Attachment #8986151 - Flags: approval-mozilla-release?
Attachment #8986151 - Flags: approval-mozilla-release+
Attachment #8986151 - Flags: approval-mozilla-esr60?
Attachment #8986151 - Flags: approval-mozilla-esr60+
Attachment #8986151 - Flags: approval-mozilla-esr52?
Attachment #8986151 - Flags: approval-mozilla-esr52+
Attachment #8986151 - Flags: approval-mozilla-beta?
Alias: CVE-2018-12366
Duplicate of this bug: 1432067
Lowering severity based on the description of the actual bug, which can't affect control flow and only shows up as a tweaked display.
Keywords: sec-moderatesec-low
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.