Alex Gaynor [:Alex_Gaynor] Reporter
	Description • 6 years ago

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

	Daniel Veditz [:dveditz]
	Comment 1 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 2 • 6 years ago

IIUC, Jeff is away for a few weeks. David, can you please help find an assignee for this?

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 3 • 6 years ago

Bas I think Nical had his hands in qcms recently? Could he take a look (since Jeff is away)?

	Nicolas Silva [:nical] Assignee
	Comment 4 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 6 years ago

A 90 day disclosure deadline means that this will be disclosed before we ship Fx62 in September, FWIW.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 6 • 6 years ago

(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.

	Nicolas Silva [:nical] Assignee
	Comment 7 • 6 years ago

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?

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 8 • 6 years ago

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.

	Nicolas Silva [:nical] Assignee
	Comment 9 • 6 years ago

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.

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 10 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

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).

	Nicolas Silva [:nical] Assignee
	Comment 12 • 6 years ago

I've been able to confirm my suspicion of the problem, I should have a patch tomorrow.

	Nicolas Silva [:nical] Assignee
	Comment 13 • 6 years ago

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.

	Nicolas Silva [:nical] Assignee
	Comment 14 • 6 years ago

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.

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 15 • 6 years ago

sec-approval is not required for sec-moderate bugs (https://wiki.mozilla.org/Security/Bug_Approval_Process)

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago

Comment on attachment 8985672 [details] [diff] [review]
Reject transforms if grid_size <= 0

This is sec-moderate, you can go ahead and land ASAP.

	Nicolas Silva [:nical] Assignee
	Comment 17 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfcc5301e87235818394a46f80dc1c164c2ca4b3

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/dfcc5301e872

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 6 years ago

Please nominate this for Beta/ESR60/ESR52 uplift when you get a chance.

	Nicolas Silva [:nical] Assignee
	Comment 20 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1b1ae042aad2
https://hg.mozilla.org/releases/mozilla-esr60/rev/7bd079502f7a

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/edf2c7dff493

	Al Billings [:abillings - ex-MoCo]
	Comment 24 • 6 years ago

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?

	Nicolas Silva [:nical] Assignee
	Comment 25 • 6 years ago

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.

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 27 • 6 years ago

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.

	Nicolas Silva [:nical] Assignee
	Comment 28 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dde5c1d895e4c063d5bda39029c9a01a178ebba

	Julien Cristau [:jcristau]
	Comment 29 • 6 years ago

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

	Julien Cristau [:jcristau]
	Comment 30 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/67b2d8924841a4c44e2907628d35ee8a022ecd2d
https://hg.mozilla.org/releases/mozilla-esr60/rev/a64fb8de655dc28efd0b7de94b6f0008b08ae0ae

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 31 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/8dde5c1d895e

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-release/rev/30e4f7b85f9e

	Daniel Veditz [:dveditz]
	Comment 34 • 6 years ago

Lowering severity based on the description of the actual bug, which can't affect control flow and only shows up as a tweaked display.