761014 - (CVE-2012-1960) Out of bounds read in qcms_transform_data_rgb_out_lut_sse2

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: 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]]

Attachment: Avoid the OOB read by sanitizing our gamma_tables

Comment 2

[Daniel Veditz [:dveditz]]

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]

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]

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

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

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

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

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

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

Attachment: 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]

Thanks, Jeff and Benoit. Are there any plans to upstream the mozilla qcms patches to the github repo?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

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

I've landed this under bug 764181 to make the security hole less obvious.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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]

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 16

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9196ea4cae2
https://hg.mozilla.org/releases/mozilla-beta/rev/d53fd484df98

Comment 17

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

If this is caused by bug 674230, this should affect ESR.

Comment 18

[Alex Keybl [:akeybl]]

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

What's the plan here for FF16?

Comment 20

[Ginn Chen]

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

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]

Jeff - assigning to you to get ESR nomination & landing.

Comment 23

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-esr10/rev/c11d39f2c8aa

Comment 24

[u279076]

I'm confused. This bug's status is set to NEW while the status flags are set to FIXED.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff, is this fixed and we just had the status confusion?

Comment 26

[Jeff Muizelaar [:jrmuizel]]

I believe so yes.