Bug 761014 (CVE-2012-1960)

Out of bounds read in qcms_transform_data_rgb_out_lut_sse2

RESOLVED FIXED

Status

()

Core
GFX: Color Management
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

({regression, sec-moderate})

unspecified
x86
Mac OS X
regression, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox13 wontfix, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr1015+ fixed)

Details

(Whiteboard: [advisory-tracking+], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 629636 [details]
Example profile

I received the following report from Tony Payne at Google

"The attached profiles respectively trigger SIGSEGV when looking up the output value for R, G or B when used as both the input and the output profile for test-transform.c"
(Assignee)

Comment 1

5 years ago
Created attachment 629637 [details] [diff] [review]
Avoid the OOB read by sanitizing our gamma_tables
Attachment #629637 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #629637 - Flags: review? → review?(bgirard)
If you don't crash on an AV maybe page content could try to read back gamma values to peek into random memory?
Keywords: sec-moderate

Comment 3

5 years ago
This patch does indeed avoid the crash, but causes serious problems in applying transform. Try it out against http://www.color.org/version4html.xalter, for example.

Comment 4

5 years ago
(In reply to Tony Payne from comment #3)
> This patch does indeed avoid the crash, but causes serious problems in
> applying transform. Try it out against
> http://www.color.org/version4html.xalter, for example.

Specifically, it seems that gamma == 1.f is expected and should not be forced to 0.f
From the specs:
NOTE The parameters selected for a parametric curve can result in complex or undefined values for the input range used. This can occur, for example, if d < −b/a. In such cases the behaviour of the curve is undefined.

I don't think I ever enforced this either.
Comment on attachment 629637 [details] [diff] [review]
Avoid the OOB read by sanitizing our gamma_tables

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

::: transform_util.c
@@ +222,5 @@
> +	int i;
> +	for (i = 0; i < 256; i++) {
> +		// Note: we check that the gamma is not in range
> +		// instead of out of range so that we catch NaNs
> +		if (!(gamma_table[i] > 0.f && gamma_table[i] < 1.f)) {

Tony is right, this should at the very least be <=. Before you post a new patch let me see if the values fall outside the range as specified in the specs.
Attachment #629637 - Flags: review?(bgirard) → review-
Parametric curve in question:
type=3
Gamma=-32,765.0000
a=0.8621
b=0.1379
c=0.1107
d=0.0800

((ax+b)^g when x >= 0.8621
Correction:
Parametric curve in question:
type=3
Gamma=-32,765.0000
a=0.8621
b=0.1379
c=0.1107
d=0.0800

((ax+b)^g when x >= 0.08
((0.8621*x+0.1379)^-32,765

Graph: http://tinyurl.com/cjrohpz
Alright so we should take the patch with the fix above and a check for d < −b/a:

Section 10.16:
The domain and range of each function shall be [0,0 1,0]. Any function value outside the range shall be clipped to the range of the function.
(Assignee)

Comment 10

5 years ago
Created attachment 631098 [details] [diff] [review]
Avoid the OOB by ensuring the output is always between 0..1

This is a more involved approach. Instead of validating the output we just ensure that is valid at creation time.
Attachment #629637 - Attachment is obsolete: true
Attachment #631098 - Flags: review?(bgirard)

Updated

5 years ago
Attachment #631098 - Flags: review?(bgirard) → review+

Comment 11

5 years ago
Thanks, Jeff and Benoit. Are there any plans to upstream the mozilla qcms patches to the github repo?
(Assignee)

Comment 12

5 years ago
(In reply to Tony Payne from comment #11)
> Thanks, Jeff and Benoit. Are there any plans to upstream the mozilla qcms
> patches to the github repo?

Yes, I'll do that today.
(Assignee)

Comment 13

5 years ago
I've landed this under bug 764181 to make the security hole less obvious.
(Assignee)

Comment 14

5 years ago
Comment on attachment 631098 [details] [diff] [review]
Avoid the OOB by ensuring the output is always between 0..1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 674230
User impact if declined: Content controlled OOB that could leak data
Testing completed (on m-c, etc.): been on m-c for a while
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None
Attachment #631098 - Flags: approval-mozilla-beta?
Attachment #631098 - Flags: approval-mozilla-aurora?
status-firefox14: --- → affected
status-firefox15: --- → affected
Comment on attachment 631098 [details] [diff] [review]
Avoid the OOB by ensuring the output is always between 0..1

Approving for both Aurora/Beta and setting status flags, please update once landed.
Attachment #631098 - Flags: approval-mozilla-beta?
Attachment #631098 - Flags: approval-mozilla-beta+
Attachment #631098 - Flags: approval-mozilla-aurora?
Attachment #631098 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9196ea4cae2
https://hg.mozilla.org/releases/mozilla-beta/rev/d53fd484df98
(Assignee)

Updated

5 years ago
status-firefox14: affected → fixed
status-firefox15: affected → fixed
If this is caused by bug 674230, this should affect ESR.
status-firefox-esr10: --- → ?
tracking-firefox-esr10: --- → ?
Whiteboard: [advisory-tracking+]
Jeff - I don't think this sg:moderate bug would be a requirement to fix in the ESR released alongside FF14, but it would be great to get into that release as well if at all possible. How hard would it be to backport?
Alias: CVE-2012-1960
Blocks: 674230
status-firefox-esr10: ? → affected
status-firefox13: --- → wontfix
Keywords: regression
What's the plan here for FF16?
tracking-firefox16: --- → ?

Comment 20

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #18)
> Jeff - I don't think this sg:moderate bug would be a requirement to fix in
> the ESR released alongside FF14, but it would be great to get into that
> release as well if at all possible. How hard would it be to backport?

The patch just applies to ESR-10 without problem.

(In reply to Alex Keybl [:akeybl] from comment #19)
> What's the plan here for FF16?

According to comment #13, it is fixed in Fx 16.
status-firefox16: --- → fixed
tracking-firefox16: ? → ---
Let's get this into the next ESR release, that ships with Firefox 15 - please nominate for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
tracking-firefox-esr10: ? → 15+
Group: core-security
Jeff - assigning to you to get ESR nomination & landing.
Assignee: nobody → jmuizelaar

Updated

5 years ago
Attachment #631098 - Flags: approval-mozilla-esr10+
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-esr10/rev/c11d39f2c8aa
status-firefox-esr10: affected → fixed
Keywords: checkin-needed
I'm confused. This bug's status is set to NEW while the status flags are set to FIXED.
Crash Signature: [@ qcms_transform_data_rgb_out_lut_sse2 ]
Jeff, is this fixed and we just had the status confusion?
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 26

4 years ago
I believe so yes.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(jmuizelaar)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.