1465075 - Two more divide-by-zero crashes in qcms

Status: NEW

(Core :: Graphics: Color Management, defect, P3)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: clusterfuzz-testcase-minimized-fuzz-4634864576888832

+++ This bug was initially created as a clone of Bug #1463424 +++

These were found by Google's OSS-Fuzz run on QCMS.

each of the attachments reproduces when run as an input to: https://github.com/google/oss-fuzz/blob/master/projects/qcms/fuzz.cc#L46

The two stacks are:

transform_util.c:501:41: runtime error: division by zero
	#0 0x43c81a in build_output_lut /src/firefox/gfx/qcms/transform_util.c:501:41
	#1 0x4325e8 in qcms_modular_transform_create_output /src/firefox/gfx/qcms/chain.c:824:3
	#2 0x4309a5 in qcms_modular_transform_create /src/firefox/gfx/qcms/chain.c:942:16
	#3 0x4307c0 in qcms_chain_transform /src/firefox/gfx/qcms/chain.c:986:50
	#4 0x437f6c in qcms_transform_precacheLUT_float /src/firefox/gfx/qcms/transform.c:1192:9
	#5 0x439e17 in qcms_transform_create /src/firefox/gfx/qcms/transform.c:1258:28
	#6 0x466360 in transform(_qcms_profile*, _qcms_profile*, unsigned long) /src/fuzz.cc:27:31
	#7 0x4661ff in LLVMFuzzerTestOneInput /src/fuzz.cc:68:3

and

transform_util.c:501:41: runtime error: division by zero
	#0 0x43c81a in build_output_lut /src/firefox/gfx/qcms/transform_util.c:501:41
	#1 0x439c3c in qcms_transform_create /src/firefox/gfx/qcms/transform.c:1276:3
	#2 0x466360 in transform(_qcms_profile*, _qcms_profile*, unsigned long) /src/fuzz.cc:27:31
	#3 0x4661ff in LLVMFuzzerTestOneInput /src/fuzz.cc:68:3

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: clusterfuzz-testcase-minimized-fuzz-4734210500722688

Comment 2

[Nicolas Silva [:nical]]

Crash due to the curve data being equal to zero and we divide by it. The curve data is set in three different places in gfx/qcms/iccread.c.
It's tempting to assume that it should not be equal to zero, add checks when creating the curve and return NULL in that case, but I'm not 100 % sure that it's the correct solution (discovering this whole qcms thing).

Comment 3

[u473386]

I think gamma == 0 is valid (if useless of course). Please take a look at 10.5 (page 48) of the specification.

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

So when u8Fixed8Number_to_float(trc->data[0]) == 0, gamma should just be set to 0. I haven't really traced the code flow, but I think that's right. It will eventually cause all bytes to be 1, but that's the (broken) profile's problem.

Comment 4

[u473386]

Or wait, it'll cause all entries in output_gamma_lut to be 1 I think, so the bytes will remain unchanged. It's a bit confusing.

Comment 5

[u473386]

Another comment: the fuzzer skips qcms_profile_is_bogus, but it doesn't check for this.

Comment 6

[u473386]

My previous comments were written in a bit of a haste and should mostly be disregarded.

Comment 7

[Nicolas Silva [:nical]]

I gave this another look, sorry for the long delay. I'm fine with special casing gamma == 0 but I can't figure out why we do this division in the first place.
A quick look at chromium's code: https://cs.chromium.org/chromium/src/third_party/skia/third_party/skcms/skcms.cc?l=367&rcl=373224c9ab3ae3f13c45f61521e01c282016fde0 The way they do things is quite different and it doesn't seem to contain a division either, the g term in that function turns into an exponent directly like in the spec, see https://cs.chromium.org/chromium/src/third_party/skia/third_party/skcms/skcms.h?l=42&rcl=373224c9ab3ae3f13c45f61521e01c282016fde0. When we build the table in our implementation we end up computing something using the inverse of this gamma value we fetch as exponent.

So maybe the real fix is to not do the division in the first place.

Comment 8

[Nicolas Silva [:nical]]

Attachment: Don't use the gamma value as an inverse.

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8990983 [details] [diff] [review]
Don't use the gamma value as an inverse.

Review of attachment 8990983 [details] [diff] [review]:
-----------------------------------------------------------------

pdknsk, could you have a look at this and tell me what you think? I have a limited understanding of these things but this seems to better match what I read from the spec and chrome's code (see comment 7).

Comment 10

[u473386]

It turns out the current implementation is correct. The inverse is only used for output profiles, which makes this a bit confusing.

skcms does the same thing. It reads in curve->parametric.g as 0, and then takes the inverse later in skcms_TransferFunction_invert.

        tf_inv.g = 1.0f / src->g;

Except that it never does because it fails before it gets to that.

    // Is the nonlinear section not invertible?
    if (has_nonlinear && (src->a == 0 || src->g == 0)) {
        return false;
    }

So I think qcms should do likewise and report those profiles as bogus.

Note that skcms takes curveType (curv) curves and converts them into parametricCurveTypes (para) curves internally, so the code has more parameters. curv with just a single value is identical to para function type 0. Described in 10.16 (page 69) in the spec.

Comment 11

[u473386]

I noticed that check is also missing in the para code in qcms, although it might be handled elsewhere later.

https://dxr.mozilla.org/mozilla-central/source/gfx/qcms/iccread.c#481

Note that qcms confusingly reads in the function type as count in the above function. Also g is named y.

https://dxr.mozilla.org/mozilla-central/source/gfx/qcms/transform_util.c#121