1132467 - (CVE-2015-4504) [qcms] stack buffer overread in lut_inverse_interp16

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[groebert]

Attachment: 012d065588837d069fc28baa44744c5d

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

Comment 1

[Frederik Braun [:freddy]]

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

Comment 2

[groebert]

Attachment: You can use the following sample program to invoke the QCMS API and reproduce this and the other crashes.

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[Benoit Girard (:BenWa)]

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.

Comment 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[groebert]

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

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Add gtest

Comment 8

[Daniel Veditz [:dveditz]]

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"?

Comment 9

[noel gordon]

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

Comment 10

[noel gordon]

Attachment: Test profile code: enable profile trasnform precache by default

Comment 11

[noel gordon]

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

Comment 12

[noel gordon]

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.

Comment 13

[noel gordon]

(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

Comment 14

[Benoit Girard (:BenWa)]

I was away. Tried handing off this bug but it didn't happen. I'll pick this up again.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: add gtest v2. r=jrmuizel

Comment 16

[Benoit Girard (:BenWa)]

Attachment: patch

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.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

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

Comment 18

[Jeff Muizelaar [:jrmuizel]]

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

Comment 19

[Benoit Girard (:BenWa)]

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.

Comment 20

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

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

Comment 21

[Benoit Girard (:BenWa)]

Attachment: patch

Looks like I was missing a qref

Comment 22

[noel gordon]

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?

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch v4

I folded the test in there and I added a test for inverting non trivial TRCs

Comment 24

[Benoit Girard (:BenWa)]

Attachment: patch v5

Added a new test

Comment 25

[Benoit Girard (:BenWa)]

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

Comment 26

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=856960f8309a

Comment 27

[Benoit Girard (:BenWa)]

Attachment: patch v6

Fixed warning:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc9325b3708

Comment 28

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83d5d84f1092

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/83d5d84f1092

Comment 30

[noel gordon]

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

Comment 31

[noel gordon]

(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

Comment 32

[Benoit Girard (:BenWa)]

If the inversion is ambiguous can you support why we should prefer to invert towards a particular value?

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 34

[Benoit Girard (:BenWa)]

It's not in beta and the patch applies cleanly. There shouldn't be any harm in uplifting.

Comment 35

[Benoit Girard (:BenWa)]

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

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f38383a965d7

Comment 39

[Benoit Girard (:BenWa)]

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

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Benoit for your prompt assessment. We can definitely wait, given that there are ~4 weeks until ESR 38.3.0 release.