Last Comment Bug 761014 - (CVE-2012-1960) Out of bounds read in qcms_transform_data_rgb_out_lut_sse2
(CVE-2012-1960)
: Out of bounds read in qcms_transform_data_rgb_out_lut_sse2
Status: RESOLVED FIXED
[advisory-tracking+]
: regression, sec-moderate
Product: Core
Classification: Components
Component: GFX: Color Management (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks: 674230
  Show dependency treegraph
 
Reported: 2012-06-03 12:46 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-05-23 01:20 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
fixed
15+
fixed


Attachments
Example profile (700 bytes, text/plain)
2012-06-03 12:46 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
Avoid the OOB read by sanitizing our gamma_tables (991 bytes, patch)
2012-06-03 12:48 PDT, Jeff Muizelaar [:jrmuizel]
bgirard: review-
Details | Diff | Splinter Review
Avoid the OOB by ensuring the output is always between 0..1 (3.39 KB, patch)
2012-06-07 13:17 PDT, Jeff Muizelaar [:jrmuizel]
bgirard: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-06-03 12:46:21 PDT
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"
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-06-03 12:48:20 PDT
Created attachment 629637 [details] [diff] [review]
Avoid the OOB read by sanitizing our gamma_tables
Comment 2 Daniel Veditz [:dveditz] 2012-06-06 10:22:30 PDT
If you don't crash on an AV maybe page content could try to read back gamma values to peek into random memory?
Comment 3 Tony Payne 2012-06-06 16:01:43 PDT
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 Tony Payne 2012-06-06 16:09:04 PDT
(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
Comment 5 Benoit Girard (:BenWa) 2012-06-07 11:30:09 PDT
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 6 Benoit Girard (:BenWa) 2012-06-07 11:33:24 PDT
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.
Comment 7 Benoit Girard (:BenWa) 2012-06-07 11:39:55 PDT
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
Comment 8 Benoit Girard (:BenWa) 2012-06-07 11:47:34 PDT
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
Comment 9 Benoit Girard (:BenWa) 2012-06-07 11:51:46 PDT
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.
Comment 10 Jeff Muizelaar [:jrmuizel] 2012-06-07 13:17:57 PDT
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.
Comment 11 Tony Payne 2012-06-07 14:29:28 PDT
Thanks, Jeff and Benoit. Are there any plans to upstream the mozilla qcms patches to the github repo?
Comment 12 Jeff Muizelaar [:jrmuizel] 2012-06-12 15:11:53 PDT
(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.
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-06-12 15:20:05 PDT
I've landed this under bug 764181 to make the security hole less obvious.
Comment 14 Jeff Muizelaar [:jrmuizel] 2012-06-27 09:56:39 PDT
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
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-27 15:37:47 PDT
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.
Comment 17 Al Billings [:abillings] 2012-07-10 11:16:30 PDT
If this is caused by bug 674230, this should affect ESR.
Comment 18 Alex Keybl [:akeybl] 2012-07-10 11:23:47 PDT
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?
Comment 19 Alex Keybl [:akeybl] 2012-07-16 07:37:44 PDT
What's the plan here for FF16?
Comment 20 Ginn Chen 2012-07-17 21:43:33 PDT
(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.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-19 16:16:02 PDT
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
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 18:14:17 PDT
Jeff - assigning to you to get ESR nomination & landing.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-13 15:23:39 PDT
I'm confused. This bug's status is set to NEW while the status flags are set to FIXED.
Comment 25 Milan Sreckovic [:milan] 2013-05-06 14:14:56 PDT
Jeff, is this fixed and we just had the status confusion?
Comment 26 Jeff Muizelaar [:jrmuizel] 2013-05-23 01:20:16 PDT
I believe so yes.

Note You need to log in before you can comment on or make changes to this bug.