Closed
Bug 1132467
(CVE-2015-4504)
Opened 10 years ago
Closed 10 years ago
[qcms] stack buffer overread in lut_inverse_interp16
Categories
(Core :: Graphics: Color Management, defect)
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)
1.95 KB,
application/octet-stream
|
Details | |
6.03 KB,
text/x-csrc
|
Details | |
7.60 KB,
text/plain
|
Details | |
10.88 KB,
patch
|
BenWa
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Component: Untriaged → GFX: Color Management
Product: Firefox → Core
Comment 1•10 years ago
|
||
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)?
Updated•10 years ago
|
Assignee: nobody → bgirard
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 5•10 years ago
|
||
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]
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8568799 -
Flags: review?(jmuizelaar)
Comment 8•10 years ago
|
||
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"?
Updated•10 years ago
|
Keywords: sec-moderate
Comment 9•10 years ago
|
||
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•10 years ago
|
||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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
Assignee | ||
Comment 14•10 years ago
|
||
I was away. Tried handing off this bug but it didn't happen. I'll pick this up again.
Updated•10 years ago
|
Attachment #8568799 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8568799 -
Attachment is obsolete: true
Attachment #8614911 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Assignee | ||
Comment 19•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Attachment #8614941 -
Flags: sec-approval?
Comment 20•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8614941 -
Flags: sec-approval?
Updated•10 years ago
|
status-firefox38:
--- → wontfix
status-firefox38.0.5:
--- → wontfix
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox41:
--- → +
Assignee | ||
Comment 21•10 years ago
|
||
Looks like I was missing a qref
Attachment #8614941 -
Attachment is obsolete: true
Attachment #8615698 -
Flags: review?(jmuizelaar)
Comment 22•10 years ago
|
||
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?
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Comment 24•10 years ago
|
||
Added a new test
Attachment #8628971 -
Attachment is obsolete: true
Attachment #8628981 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 25•10 years ago
|
||
(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?
Updated•10 years ago
|
Attachment #8628981 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 26•10 years ago
|
||
Flags: needinfo?(bgirard)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8628981 -
Attachment is obsolete: true
Attachment #8629405 -
Flags: review+
Assignee | ||
Comment 28•10 years ago
|
||
Comment 29•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 30•10 years ago
|
||
(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).
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bgirard)
Comment 31•10 years ago
|
||
(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
Assignee | ||
Comment 32•10 years ago
|
||
If the inversion is ambiguous can you support why we should prefer to invert towards a particular value?
Flags: needinfo?(noel.gordon)
Comment 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
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)
Assignee | ||
Comment 35•10 years ago
|
||
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 36•10 years ago
|
||
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+
Comment 37•10 years ago
|
||
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)
Assignee | ||
Comment 39•10 years ago
|
||
(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)
Comment 40•10 years ago
|
||
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)
Updated•10 years ago
|
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → wontfix
status-b2g-v2.1S:
--- → wontfix
status-b2g-v2.2:
--- → wontfix
status-b2g-v2.2r:
--- → wontfix
status-b2g-master:
--- → fixed
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1105045
Updated•9 years ago
|
Whiteboard: https://bugzilla.mozilla.org/show_bug.cgi?id=1105045 → [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Updated•9 years ago
|
Alias: CVE-2015-4504
Updated•9 years ago
|
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•