Closed Bug 1432067 Opened 4 years ago Closed 4 years ago

heap-buffer-overflow in qcms with mAB tag type

Categories

(Core :: GFX: Color Management, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1464039
Tracking Status
firefox-esr52 --- fixed
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: u473386, Unassigned)

Details

(Keywords: sec-low)

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.98 Safari/537.36

Steps to reproduce:

==15782==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000284 at pc 0x00000051ebc0 bp 0x7ffee73c2f20 sp 0x7ffee73c2f18
READ of size 4 at 0x602000000284 thread T0
    #0 0x51ebbf in qcms_transform_module_clut_only QCMS/qcms/chain.c:153:21
    #1 0x51c796 in qcms_modular_transform_data QCMS/qcms/chain.c:975:17
    #2 0x51c1a6 in qcms_chain_transform QCMS/qcms/chain.c:988:16
    #3 0x52ad98 in qcms_transform_precacheLUT_float QCMS/qcms/transform.c:1182:9
    #4 0x52f493 in qcms_transform_create QCMS/qcms/transform.c:1247:28

0x602000000284 is located 12 bytes to the left of 1-byte region [0x602000000290,0x602000000291)
allocated by thread T0 here:
    #0 0x4bf0d3 in malloc /local/mnt/workspace/tmp/llvm/utils/release/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:67:3
    #1 0x51aa75 in qcms_modular_transform_create_mAB QCMS/qcms/chain.c:584:10
    #2 0x5198ae in qcms_modular_transform_create_input QCMS/qcms/chain.c:723:19
    #3 0x51c308 in qcms_modular_transform_create QCMS/qcms/chain.c:902:16
    #4 0x51c171 in qcms_chain_transform QCMS/qcms/chain.c:986:50
    #5 0x52ad98 in qcms_transform_precacheLUT_float QCMS/qcms/transform.c:1182:9
    #6 0x52f493 in qcms_transform_create QCMS/qcms/transform.c:1247:28

SUMMARY: AddressSanitizer: heap-buffer-overflow QCMS/qcms/chain.c:153:21 in qcms_transform_module_clut_only
Shadow bytes around the buggy address:
  0x0c047fff8000: fa fa fd fa fa fa fd fa fa fa fd fa fa fa 04 fa
  0x0c047fff8010: fa fa 04 fa fa fa 04 fa fa fa 04 fa fa fa 04 fa
  0x0c047fff8020: fa fa 04 fa fa fa 04 fa fa fa 04 fa fa fa 04 fa
  0x0c047fff8030: fa fa 04 fa fa fa 04 fa fa fa 00 00 fa fa 00 fa
  0x0c047fff8040: fa fa 00 fa fa fa 00 fa fa fa fd fa fa fa fd fd
=>0x0c047fff8050:[fa]fa 01 fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff80a0: 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


Actual results:

This occurs for A2B0 (or B2A0) tags with mAB (or mBA) type. The latter must have an A curve and B curve, with the CLUT offset set to 0, to trigger the following order of events.

https://dxr.mozilla.org/mozilla-central/rev/8b1f0ca44c7be9a15bfbedf0cde540831f2e2aec/gfx/qcms/iccread.c#575

1.) clut_offset = 0
2.) clut_size = 0
3.) lut is allocated with the size of struct lutmABType (320) + clut_size * 4.
4.) lut->clut_table points to &lut->clut_table_data[0] (lut+320)!

And it then cascades from there.

The following check should catch it, but doesn't, as lut->clut_table always points to an address.

https://dxr.mozilla.org/mozilla-central/rev/8b1f0ca44c7be9a15bfbedf0cde540831f2e2aec/gfx/qcms/chain.c#560

A quick patch that works.

--- a/iccread.c
+++ b/iccread.c
@@ -614,7 +614,7 @@
 		return NULL;
 	// we'll fill in the rest below
 	memset(lut, 0, sizeof(struct lutmABType));
-	lut->clut_table   = &lut->clut_table_data[0];
+	lut->clut_table = clut_size ? &lut->clut_table_data[0] : NULL;
 
         if (clut_offset) {
 		for (i = 0; i < num_in_channels; i++) {



Expected results:

Note that A2B0 (or B2A0) tags are only used when gfx.color_management.enablev4 is set, which isn't by default. Apparently it's popular with professional users (photographers, artists), so a non-insignificant amount of users are probably affected. It also may or may not become default at some point.

I can make a minimal test case if necessary.
Group: firefox-core-security → gfx-core-security
Component: Untriaged → GFX: Color Management
Product: Firefox → Core
Flags: sec-bounty?
Please add a testcase if possible.
Flags: needinfo?(pdknsk)
Attached file firefox-1432067
Flags: needinfo?(pdknsk)
firefox-1432067 is a minimal profile that triggers it. I'm not sure if you also want me to add code to that. Also, a trivially better patch.

--- a/iccread.c
+++ b/iccread.c
@@ -614,7 +614,8 @@
 		return NULL;
 	// we'll fill in the rest below
 	memset(lut, 0, sizeof(struct lutmABType));
-	lut->clut_table   = &lut->clut_table_data[0];
+	if (clut_size)
+		lut->clut_table = &lut->clut_table_data[0];
 
         if (clut_offset) {
 		for (i = 0; i < num_in_channels; i++) {
Attached image firefox-1432067.png
Or open this 198-byte PNG (with the profile included) in a Firefox ASAN release build. (Debug build hits an assert.) Remember to set gfx.color_management.enablev4 first. (Might need restart.)

==12833==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000f60d0 at pc 0x7f835d32f1b1 bp 0x7f834ca8d990 sp 0x7f834ca8d988
READ of size 4 at 0x6020000f60d0 thread T18 (ImgDecoder #1)
    #0 0x7f835d32f1b0  (firefox/firefox/libxul.so+0x44d91b0)
I forgot the symbolizer. And I think my previous comment might've been ambiguous in that the profile still needs to be attached to (included in) the PNG: it doesn't.

==32175==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200006b0d0 at pc 0x7f7e7706a281 bp 0x7f7e69ab0990 sp 0x7f7e69ab0988
READ of size 4 at 0x60200006b0d0 thread T12 (ImgDecoder #1)
    #0 0x7f7e7706a280 in qcms_transform_module_clut_only /builds/worker/workspace/build/src/gfx/qcms/chain.c:153:21
    #1 0x7f7e7706803e in qcms_modular_transform_data /builds/worker/workspace/build/src/gfx/qcms/chain.c:975:17
    #2 0x7f7e7706803e in qcms_chain_transform /builds/worker/workspace/build/src/gfx/qcms/chain.c:988
    #3 0x7f7e7707cb88 in qcms_transform_precacheLUT_float /builds/worker/workspace/build/src/gfx/qcms/transform.c:1182:9
    #4 0x7f7e7707fb1f in qcms_transform_create /builds/worker/workspace/build/src/gfx/qcms/transform.c:1248:28
    #5 0x7f7e779f237c in mozilla::image::nsPNGDecoder::info_callback(png_struct_def*, png_info_def*) /builds/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:644:27
    #6 0x7f7e7dfdad71 in MOZ_PNG_push_have_info /builds/worker/workspace/build/src/media/libpng/pngpread.c:1195:7
    #7 0x7f7e7dfdad71 in MOZ_PNG_push_read_chunk /builds/worker/workspace/build/src/media/libpng/pngpread.c:352
    #8 0x7f7e7dfd830b in MOZ_PNG_proc_some_data /builds/worker/workspace/build/src/media/libpng/pngpread.c:109:10
    #9 0x7f7e7dfd830b in MOZ_PNG_process_data /builds/worker/workspace/build/src/media/libpng/pngpread.c:46
    #10 0x7f7e779fa23c in mozilla::image::nsPNGDecoder::ReadPNGData(char const*, unsigned long) /builds/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:405:3

0x60200006b0d1 is located 0 bytes to the right of 1-byte region [0x60200006b0d0,0x60200006b0d1)
allocated by thread T12 (ImgDecoder #1) here:
    #0 0x4c1c93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f7e770661aa in qcms_modular_transform_create_mAB /builds/worker/workspace/build/src/gfx/qcms/chain.c:584:10
    #2 0x7f7e77064f67 in qcms_modular_transform_create_input /builds/worker/workspace/build/src/gfx/qcms/chain.c:723:19
    #3 0x7f7e77067c0b in qcms_modular_transform_create /builds/worker/workspace/build/src/gfx/qcms/chain.c:902:16
    #4 0x7f7e77067c0b in qcms_chain_transform /builds/worker/workspace/build/src/gfx/qcms/chain.c:986
    #5 0x7f7e7707cb88 in qcms_transform_precacheLUT_float /builds/worker/workspace/build/src/gfx/qcms/transform.c:1182:9
    #6 0x7f7e7707fb1f in qcms_transform_create /builds/worker/workspace/build/src/gfx/qcms/transform.c:1248:28
    #7 0x7f7e779f237c in mozilla::image::nsPNGDecoder::info_callback(png_struct_def*, png_info_def*) /builds/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:644:27
    #8 0x7f7e7dfdad71 in MOZ_PNG_push_have_info /builds/worker/workspace/build/src/media/libpng/pngpread.c:1195:7
    #9 0x7f7e7dfdad71 in MOZ_PNG_push_read_chunk /builds/worker/workspace/build/src/media/libpng/pngpread.c:352
    #10 0x7f7e7dfd830b in MOZ_PNG_proc_some_data /builds/worker/workspace/build/src/media/libpng/pngpread.c:109:10
    #11 0x7f7e7dfd830b in MOZ_PNG_process_data /builds/worker/workspace/build/src/media/libpng/pngpread.c:46
    #12 0x7f7e779fa23c in mozilla::image::nsPNGDecoder::ReadPNGData(char const*, unsigned long) /builds/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:405:3
I'm not 100% sure there is no work for zero sized tables, but it feels like the code that checks if the allocation failed may have been expecting malloc to return a null pointer for zero sized allocations.
Assignee: nobody → milan
Attachment #8964616 - Flags: review?(jmuizelaar)
Attachment #8964616 - Flags: review?(jmuizelaar) → review+
There is a use-of-uninitialized-value bug in the same file, albeit not security-relevant.

https://bugzilla.mozilla.org/show_bug.cgi?id=1444734

Also, I'd like to submit qcms to oss-fuzz. If you agree, I can open a separate bug report for further details.
This needs a sec rating and, depending on the rating, possibly sec-approval before it can land.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(milaninbugzilla)
Keywords: checkin-needed
This looks like a sec-moderate; it will access the zero sized memory, but it will not read arbitrary length, just that pointer, and there is no writing involved.
Flags: needinfo?(milaninbugzilla)
Keywords: sec-moderate
Assignee: milaninbugzilla → nobody
Status: ASSIGNED → NEW
I'm sorry to write that the bug was fixed incorrectly, in that valid profiles are now rejected. clut_size == 0 can only be caused by clut_offset == 0 here, which is perfectly legitimate and just indicates that there is no CLUT in the mAB (or mBA) table. You can verify this in 10.10.1 (page 55) of the ICC specification.

http://www.color.org/specification/ICC1v43_2010-12.pdf

(I suggest the patch I had proposed.)
(As requested.)
Flags: needinfo?(milaninbugzilla)
Darn.  Probably should back out while we're sorting the actual fix.
Flags: needinfo?(milaninbugzilla) → needinfo?(dbolter)
Conveniently, this bug never got resolved when it was merged to m-c. And it landed after the final merge of 61 to Beta, so this was only ever on trunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/408383be12633fc4ac61002b8e826b8ac994ae94
Flags: needinfo?(dbolter)
This bug was fixed by these commits.

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

Either this bug, or the other bug (which was reported by oss-fuzz), should be marked as duplicate.
Flags: needinfo?(milaninbugzilla)
(In reply to pdknsk from comment #18)

> Either this bug, or the other bug (which was reported by oss-fuzz), should
> be marked as duplicate.

What's the bug number? I see no links here.
1464039
If you enter bug 1464039 (bug followed by the number) it auto-links in bugzilla.

Normally, since this bug was opened well before, I'd make the other bug a dupe of this. As it is, that bug was fixed and disclosed in the advisories I wrote for the recent release so we should dupe this bug there.

I'm unclear about bounty status on OSS-Fuzz bugs. This bug is nominate for a bounty (I believe you emailed us to ask for this) but the other, fixed bug, is not.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: CVE-2018-12366
If it makes a difference, I did the oss-fuzz integration of qcms. I don't think there is a bounty for oss-fuzz bugs, and I don't want to claim it (separately, or for other qcms bugs oss-fuzz may find).
Flags: needinfo?(milaninbugzilla)
Flags: sec-bounty?
Ah, I think I need to clarify: if a bounty is granted for this bug, I'd still like to claim it, as the report predates the oss-fuzz integration by a few months.
(In reply to pdknsk from comment #23)
> Ah, I think I need to clarify: if a bounty is granted for this bug, I'd
> still like to claim it, as the report predates the oss-fuzz integration by a
> few months.

Since it pre-dates it, that seems reasonable. We're not going to offer bounties on bugs that come to us via oss-fuzz.
Flags: sec-bounty?
Lowering severity based on the description of the what's going on in bug 1464039: can't affect control flow and if it doesn't crash only shows up as a tweaked display.
Flags: sec-bounty? → sec-bounty-
Keywords: sec-moderatesec-low
Group: gfx-core-security
You need to log in before you can comment on or make changes to this bug.