Closed Bug 1132467 (CVE-2015-4504) Opened 5 years ago Closed 4 years ago

[qcms] stack buffer overread in lut_inverse_interp16

Categories

(Core :: GFX: Color Management, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 + fixed
firefox42 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-v2.2r --- wontfix
b2g-master --- fixed

People

(Reporter: groebert, Assigned: BenWa)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(4 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36

Steps to reproduce:

Load ICC file 012d065588837d069fc28baa44744c5d in a transformation.


Actual results:

stack-buffer-overflow READ of size 2
lut_inverse_interp16 qcms/src/transform_util.c:378:14

The calculation of cell0 on line 373 is wrong which leads to a negative offset into LutTable. Potentially, this can be used to disclose data from the stack because the read value is later used in the precached transformation.

(gdb) p length
$2529 = 256

371             val2 = (length-1) * ((double) (x - 1) / 65535.0);
(gdb) info locals
l = -258
r = -258
x = -257
res = 65535
NumZeroes = 0
NumPoles = 255
cell0 = -156712448
cell1 = 32767
val2 = 1.2598673968951787e-321
y0 = 6.9533490648866048e-310
y1 = 6.9533557036156112e-310
x0 = 6.9533558073464722e-310
x1 = 7.2035640719190427e-317
a = 0
b = 6.9533491707515823e-310
f = 6.9533484734308871e-310
(gdb) n
373             cell0 = (int) floor(val2);
(gdb) info locals
l = -258
r = -258
x = -257
res = 65535
NumZeroes = 0
NumPoles = 255
cell0 = -156712448
cell1 = 32767
val2 = -1.0038910505836576
y0 = 6.9533490648866048e-310
y1 = 6.9533557036156112e-310
x0 = 6.9533558073464722e-310
x1 = 7.2035640719190427e-317
a = 0
b = 6.9533491707515823e-310
f = 6.9533484734308871e-310
(gdb) n
374             cell1 = (int) ceil(val2);
(gdb) info locals
l = -258
r = -258
x = -257
res = 65535
NumZeroes = 0
NumPoles = 255
cell0 = -2
cell1 = 32767
val2 = -1.0038910505836576
y0 = 6.9533490648866048e-310
y1 = 6.9533557036156112e-310
x0 = 6.9533558073464722e-310
x1 = 7.2035640719190427e-317
a = 0
b = 6.9533491707515823e-310
f = 6.9533484734308871e-310
Component: Untriaged → GFX: Color Management
Product: Firefox → Core
CCing Benoit, who seems to work on color management. Can you help triaging this (and the other two issues I've been CCing you on)?
Assignee: nobody → bgirard
I'm getting:

parse.c:5:55: error: hex escape sequence out of range
"AAAAAAAAAAAAAAAAAAAAAAAAAA_+-=123\x20\x01\xff\xff\xff\xfeBCD1234"
                         

from gcc/clang. Should this be 0xfe followed by the characters BCD1234? Trying this but I can't reproduce the issue.
Taking a look using just the gdb session posted above:

The problem is we compute:
a = ((NumZeroes-1) * 0xFFFF) / (length-1);
b = ((length-1 - NumPoles) * 0xFFFF) / (length-1);
where NumZeroes=0, NumPoles = 255, which gives:
a = -65535 / 256 = -255
b = (256-1 - 255) / 255 = 0

Then we're trying to run a binary search from -255 to 0 which is a disaster.

I don't understand why this binary search should do something different for zeroes/poles and why it's so complicated.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Ok it looks like the bad code comes from the July27 edit to handle these 'degenerate' curves. Otherwise we could just fix r = length and run a normal binary search.

I'm not sure why this treats multiple 0 or 0xFFFF in a special way but not any other value. This edit feels like a hack. I'm not sure if the caller has any right to except one value over another in this case.
(Re: comment #3)

In GCC 4.8 it's just a warning.

> cc --version
cc (Ubuntu 4.8.2-19ubuntu1) 4.8.2
> cc -Wall -ggdb  -ldl  parse.c iccread.o transform.o matrix.o chain.o transform_util.o transform-sse2.o transform-sse1.o -lm  -o parse
parse.c:70:1: warning: hex escape sequence out of range [enabled by default]
Attached patch Add gtest (obsolete) — Splinter Review
Attachment #8568799 - Flags: review?(jmuizelaar)
Comment on attachment 8568799 [details] [diff] [review]
Add gtest

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

::: gfx/qcms/transform_util.c
@@ +309,5 @@
>          cell0 = (int) floor(val2);
>          cell1 = (int) ceil(val2);
> +
> +        assert(cell0 >= 0);
> +        assert(cell0 >= 0);

I assume one of these is supposed to be "cell1"?
With tht color profile 012d065588837d069fc28baa44744c5d, I can only reproduce when the line

  // qcms_profile_precache_output_transform(input_profile);

is not commented out.

I updated the test program to precache the input profile by default, added an optional arg to turn that off if needed, and made the profile the target of the color transform always. (attached).
012d065588837d069fc28baa44744c5d is an ICC V4 profile with a parametric TRC curve, y = 1 for all input x.

Chromium doesn't support V4 profiles at this time so never calls qcms_enable_iccv4(), yet QCMS allows this V4 profile to be read successfully.  That seems wrong... If parametric TRC curves were introduced in the V4 ICC spec, and not defined for V2 profiles, perhaps we should deny reading them unless qcms_enable_iccv4() has been called. That seems like a separate qcms bug.

Point is, this V4 profile comes straight through the door regardless of qcms_enable_iccv4().
And provided you call qcms_profile_precache_output_transform() on that profile:

(In reply to Benoit Girard (On Leave Until June 1st!) from comment #4)
 
> The problem is we compute:
> a = ((NumZeroes-1) * 0xFFFF) / (length-1);
> b = ((length-1 - NumPoles) * 0xFFFF) / (length-1);
> where NumZeroes=0, NumPoles = 255, which gives:
> a = -65535 / 256 = -255
> b = (256-1 - 255) / 255 = 0
> 
> Then we're trying to run a binary search from -255 to 0 which is a disaster.

Concur. The parametric TRC curve is all ones. qcms_profile_precache_output_transform() makes us land here when qcms tries to invert the TRC curve.

Mitigation might be to ensure the binary search bounds are sane on leaving this block of code that deals with the degenerate case.

  if (l < 1)
      l = 1;
  if (r > 0x10000)
      r = 0x10000;

> I don't understand why this binary search should do something different for
> zeroes/poles and why it's so complicated.

The degenerate case seems to be runs of 0's / 1's from the left / right edge of the TRC curve, a curve that this old lcms code wants to invert. The all-poles case is not handled securely (this bug).

And I'm guessing degenerate means the TRC curve is not monotonic increasing / decreasing in those edge areas - that'd mean there's no well-defined inverse in those edge areas maths-wise - and then the code past the binary search looks to invert using an inverse straight line approximation.

This code is gone from the current lcms repository, best I can tell.
(In reply to Benoit Girard (On Leave Until June 1st!) from comment #5)
> Ok it looks like the bad code comes from the July27 edit to handle these
> 'degenerate' curves. Otherwise we could just fix r = length and run a normal
> binary search.

Yeah that July27 code edit, and no details I could find about why it was added.

Mitigation per #12 makes the binary search limits sane, but the routine then inverts the all 1's curve to all 0's. Not sure that's right.

> I'm not sure why this treats multiple 0 or 0xFFFF in a special way but not
> any other value. This edit feels like a hack.

Yeah, might be a run of 0x8000 for example. That could be in the middle of a "curve" making it degenerate. Real-world devices have monotonic curves. Not sure what purpose degenerate curves serve.

> I'm not sure if the caller has
> any right to except one value over another in this case.

Older versions of the that July27 code output 0 in the 0's region, and 0xffff in the 1's region (see for example lcms-1.08/src/cmsintrp.c). I've used something like that for now and #12, and submitted a fix to mitigate this issue in Chrome [1].

[1] https://crrev.com/a34e151bce4bdf56031c13941408e292815241ca
I was away. Tried handing off this bug but it didn't happen. I'll pick this up again.
Attachment #8568799 - Flags: review?(jmuizelaar) → review+
Attached patch add gtest v2. r=jrmuizel (obsolete) — Splinter Review
Attachment #8568799 - Attachment is obsolete: true
Attachment #8614911 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
He's a fix. I understand the code somewhat but I'm still not certain that it's fully correct. This does fix the bug and fixes the cases that left me wondering what-if.

That being said I'm not ruling out that perhaps this code just be re-written. It's overly complicated for what it does.
Attachment #8614941 - Flags: review?(jmuizelaar)
Comment on attachment 8614941 [details] [diff] [review]
patch

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

Please don't change the white space at the same time or use mozreview
Attachment #8614941 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8614941 [details] [diff] [review]
patch

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

Please don't change the white space at the same time or use mozreview
Attachment #8614941 - Flags: review- → review+
Comment on attachment 8614911 [details] [diff] [review]
add gtest v2. r=jrmuizel

[Security approval request comment]
How easily could an exploit be constructed based on the patch? This testcase will point out how to crash but it isn't an exploit on its own.

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

Which older supported branches are affected by this flaw? All the way to 3.x-ish.

If not all supported branches, which bug introduced the flaw? 

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This code hasn't really changed much lately

How likely is this patch to cause regressions; how much testing does it need? Fairly low, worse cause it could cause side effects to non invertible color management profiles.
Attachment #8614911 - Flags: sec-approval?
Attachment #8614941 - Flags: sec-approval?
Comment on attachment 8614911 [details] [diff] [review]
add gtest v2. r=jrmuizel

As a sec-moderate, this doesn't need sec-approval+ to go in (only high and critical rated issues affecting multiple branches do).
Attachment #8614911 - Flags: sec-approval?
Attachment #8614941 - Flags: sec-approval?
Attached patch patch (obsolete) — Splinter Review
Looks like I was missing a qref
Attachment #8614941 - Attachment is obsolete: true
Attachment #8615698 - Flags: review?(jmuizelaar)
Does this change affect the inversion of a valid output profile such as sRGB (IEC61966-2.1)?  Does the inversion of its TRC makes sense?
Attached patch patch v4 (obsolete) — Splinter Review
I folded the test in there and I added a test for inverting non trivial TRCs
Attachment #8614911 - Attachment is obsolete: true
Attachment #8615698 - Attachment is obsolete: true
Attachment #8615698 - Flags: review?(jmuizelaar)
Attached patch patch v5 (obsolete) — Splinter Review
Added a new test
Attachment #8628971 - Attachment is obsolete: true
Attachment #8628981 - Flags: review?(jmuizelaar)
(In reply to noel gordon from comment #22)
> Does this change affect the inversion of a valid output profile such as sRGB
> (IEC61966-2.1)?  Does the inversion of its TRC makes sense?

The LUT inverse test should address your concern.

Basically I just make sure the values maps within the range. There will be information lost in those ranges.

Let me know if you have any specific concerns?
Attachment #8628981 - Flags: review?(jmuizelaar) → review+
Attached patch patch v6Splinter Review
Fixed warning:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc9325b3708
Attachment #8628981 - Attachment is obsolete: true
Attachment #8629405 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/83d5d84f1092
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Benoit Girard (:BenWa) from comment #25)
> (In reply to noel gordon from comment #22)
> > Does this change affect the inversion of a valid output profile such as sRGB
> > (IEC61966-2.1)?  Does the inversion of its TRC makes sense?
> 
> The LUT inverse test should address your concern.

Thank you for the tests, they look good to me.

> Basically I just make sure the values maps within the range. There will be
> information lost in those ranges.

Nod.

> Let me know if you have any specific concerns?

Minor one: sRGB inverts to 7 at the origin for me (should be 0, I think).
Flags: needinfo?(bgirard)
(In reply to noel gordon from comment #30)

> Minor one: sRGB inverts to 7 at the origin for me (should be 0, I think).

http://ghostscript.com/~tor/gs-browse/gs/lcms/src/cmsintrp.c#554

https://codereview.chromium.org/1215583002
If the inversion is ambiguous can you support why we should prefer to invert towards a particular value?
Flags: needinfo?(noel.gordon)
Ben, is this patch already in Beta41? I believe this was checked in after the merge on 6-29 so I do not think this made it into Aurora41 or Beta41 but I could be wrong. 

If this needs to be uplifted, can you please make a request? Thanks.
Flags: needinfo?(bgirard)
It's not in beta and the patch applies cleanly. There shouldn't be any harm in uplifting.
Flags: needinfo?(noel.gordon)
Flags: needinfo?(bgirard)
Comment on attachment 8629405 [details] [diff] [review]
patch v6

Approval Request Comment
[Feature/regressing bug #]: qcms v4 (which is not enabled so not overly important to uplift this to beta)
[User impact if declined]: Slight security risk for users that manually flip this preference (tiny audience)
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: low, has been on aurora/nightly for a month now.
[String/UUID change made/needed]: none
Attachment #8629405 - Flags: approval-mozilla-beta?
Comment on attachment 8629405 [details] [diff] [review]
patch v6

Approved for uplift to Beta as this patch fixes a sec vulnerability and the fix has stabilized for long in Nightly and Aurora.
Attachment #8629405 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Benoit, thanks for the beta uplift request. Given that esr38 is also marked affected, should we uplift this to moz-esr38 branch as well? Thanks.
Flags: needinfo?(bgirard)
(In reply to Ritu Kothari (:ritu) from comment #37)
> Benoit, thanks for the beta uplift request. Given that esr38 is also marked
> affected, should we uplift this to moz-esr38 branch as well? Thanks.

We could, or we could wait. This isn't really a critical issue IMO and it's behind a preference. Since the patch changes the linkage of qcms to be in XUL it's probably best to not uplift to esr-38.
Flags: needinfo?(bgirard) → needinfo?(rkothari)
Thanks Benoit for your prompt assessment. We can definitely wait, given that there are ~4 weeks until ESR 38.3.0 release.
Flags: needinfo?(rkothari)
Group: core-security → core-security-release
Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1105045
Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1105045 → [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: CVE-2015-4504
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.