1432067 - heap-buffer-overflow in qcms with mAB tag type

Status: RESOLVED DUPLICATE of bug 1464039

(Core :: Graphics: Color Management, defect)

Description

[u473386]

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.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

Please add a testcase if possible.

Comment 2

[u473386]

Attachment: firefox-1432067

Comment 3

[u473386]

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++) {

Comment 4

[u473386]

Attachment: firefox-1432067.png

Comment 5

[u473386]

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)

Comment 6

[u473386]

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

Comment 7

[u473386]

Ping?

Comment 8

[Al Billings [:abillings - ex-MoCo]]

(In reply to pdknsk from comment #7)
> Ping?

Please don't do this. It doesn't help bugs get fixed. The developers will work on it as they can.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: If the table is zero sized, skip it. r?jrmuizel

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.

Comment 10

[u473386]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This needs a sec rating and, depending on the rating, possibly sec-approval before it can land.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Well, in that case... :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b228e67f4c4fd14dadc89b7e583d12d17bea2a

Comment 14

[u473386]

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

Comment 15

[u473386]

(As requested.)

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Darn.  Probably should back out while we're sorting the actual fix.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[u473386]

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.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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

Comment 20

[u473386]

1464039

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 22

[u473386]

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

Comment 23

[u473386]

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.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

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

Comment 25

[Daniel Veditz [:dveditz]]

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.