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"

If you don't crash on an AV maybe page content could try to read back gamma values to peek into random memory?

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.

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

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.

This is a more involved approach. Instead of validating the output we just ensure that is valid at creation time.

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

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

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

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

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

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

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?

What's the plan here for FF16?

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

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

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

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

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

I believe so yes.