Last Comment Bug 722831 - (CVE-2013-0792) some grayscale png images display improperly when "gfx.color_management.enablev4" enabled
(CVE-2013-0792)
: some grayscale png images display improperly when "gfx.color_management.enabl...
Status: RESOLVED FIXED
[adv-main20+]
: sec-moderate
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: 11 Branch
: x86 All
: -- normal (vote)
: mozilla21
Assigned To: Milan Sreckovic [:milan]
:
Mentors:
: 791456 804137 (view as bug list)
Depends on: 839621
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-31 13:04 PST by Tobias Schula
Modified: 2014-11-19 19:37 PST (History)
17 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
fixed
20+
wontfix
20+
wontfix
wontfix
wontfix


Attachments
left how it looks, right how it should be (148.17 KB, image/png)
2012-01-31 13:04 PST, Tobias Schula
no flags Details
My display profile, made with dispcalGUI (505.92 KB, application/octet-stream)
2012-01-31 13:12 PST, Tobias Schula
no flags Details
Dell u2711 OEM display profile (100.37 KB, text/plain)
2012-03-25 04:46 PDT, Ilia Pozhilov
no flags Details
Adobe RGB 1998 from photoshop CS5 distro (560 bytes, text/plain)
2012-03-25 04:50 PDT, Ilia Pozhilov
no flags Details
The caching only seems to be setup for RGB (and not grayscale) (1.03 KB, patch)
2013-02-08 09:32 PST, Milan Sreckovic [:milan]
bgirard: feedback+
Details | Diff | Splinter Review
The caching only works for RGB, so only call for RGB (1.36 KB, patch)
2013-02-08 12:53 PST, Milan Sreckovic [:milan]
bgirard: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17-
lukasblakk+bugs: approval‑mozilla‑b2g18-
dveditz: sec‑approval+
Details | Diff | Splinter Review

Description Tobias Schula 2012-01-31 13:04:38 PST
Created attachment 593201 [details]
left how it looks, right how it should be

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:11.0a2) Gecko/20120131 Firefox/11.0a2
Build ID: 20120131042011

Steps to reproduce:

I enabled the "gfx.color_management.enablev4" value in "about:config" to use my LUT display profile. 


Actual results:

Grayscale png-images from xkcd (eg: http://xkcd.com/386/) display improperly.


Expected results:

The images should display without errors.
Comment 1 Tobias Schula 2012-01-31 13:12:55 PST
Created attachment 593204 [details]
My display profile, made with dispcalGUI
Comment 2 Ilia Pozhilov 2012-03-25 04:22:21 PDT
Same here on win7 x64
Comment 3 Ilia Pozhilov 2012-03-25 04:46:23 PDT
It happens with my OEM dell u2711 profile, but is not reproduced if I set preinstalled Adobe RGB profile.
Comment 4 Ilia Pozhilov 2012-03-25 04:46:59 PDT
Created attachment 609107 [details]
Dell u2711 OEM display profile
Comment 5 Ilia Pozhilov 2012-03-25 04:50:02 PDT
Created attachment 609108 [details]
Adobe RGB 1998 from photoshop CS5 distro
Comment 6 Benoit Girard (:BenWa) 2012-04-01 18:18:34 PDT
Confirmed on 10.7, I see roughly the same screenshot as above.

The profile doesn't appear to be 100% valid:
/System/Library/ColorSync/Profiles/Screen 1 2011-12-06 max D5000 min nativ 2.2 HQ XYZLUT.icc
   Tag 'desc': Tag size is not correct. 
   Tag 'dmdd': Tag size is not correct. 
   Tag 'desc': Description tag has a bad Macintosh string. 
   Tag 'dmdd': Description tag has a bad Macintosh string. 

We should either handle this profile correctly or reject it completely.
Comment 7 Benoit Girard (:BenWa) 2012-04-01 21:31:07 PDT
Turns out that QCMS rejects in ICCv4 mode but the nsPNGDecoder doesn't handle having a null decoder->mTransform when decoding grayscale->rgb.

At the very least there's uninitialized reads happening here.
Comment 8 Daniel Veditz [:dveditz] 2012-06-13 10:46:26 PDT
Over to Joe to take a stab at rating this. Uninitialized reads might be an info leak security bug, but if it's only with color profiles that an attacker can't force you to enable maybe it's not so bad?
Comment 9 Ilia Pozhilov 2012-06-13 10:56:27 PDT
(In reply to Daniel Veditz [:dveditz] from comment #8)
> Over to Joe to take a stab at rating this. Uninitialized reads might be an
> info leak security bug, but if it's only with color profiles that an
> attacker can't force you to enable maybe it's not so bad?

it's that bad because you see garbage instead of your pictures
Comment 10 Joe Drew (not getting mail) 2012-06-13 11:02:53 PDT
I would be surprised if you couldn't embed such a profile inside an image. For now I'll say this is sec-high.
Comment 11 Daniel Veditz [:dveditz] 2012-11-07 11:26:49 PST
*** Bug 804137 has been marked as a duplicate of this bug. ***
Comment 12 Daniel Veditz [:dveditz] 2012-11-07 11:28:36 PST
*** Bug 791456 has been marked as a duplicate of this bug. ***
Comment 13 Al Billings [:abillings] 2013-01-03 13:19:57 PST
Any updates on getting this sec-high fixed? It has been over six months.
Comment 14 Joe Drew (not getting mail) 2013-01-21 12:16:22 PST
It appears that this was fixed by some refactoring or another. We definitely handle null mTransforms now.
Comment 15 Emanuel Hoogeveen [:ehoogeveen] 2013-01-26 06:25:16 PST
The example from my bug (bug 791456) still looks totally broken when I set gfx.color_management.enablev4 to true and gfx.color_management.mode to anything other than 0. Tested on current Nightly (2013-01-25).
Comment 16 Matthew Gregan [:kinetik] 2013-02-06 19:26:50 PST
This is still broken in current nightlies.
Comment 17 Milan Sreckovic [:milan] 2013-02-07 07:47:06 PST
FWIW, it looks fine to me on Linux nightly, with gfx.color_management.enablev4 set to true (or false) and gfx.color_management.mode set to 2 (default).
Comment 18 Tobias Schula 2013-02-07 08:55:09 PST
(In reply to Milan Sreckovic [:milan] from comment #17)
> FWIW, it looks fine to me on Linux nightly, with
> gfx.color_management.enablev4 set to true (or false) and
> gfx.color_management.mode set to 2 (default).

That's because the pictures don't have an embedded color profile so firefox doesn't transform anything if gfx.color_management.mode is set to 2. If set to 1 the output is still garbled.
Comment 19 Milan Sreckovic [:milan] 2013-02-07 10:10:06 PST
Got it - I was going off Comment 15 "gfx.color_management.mode set to anything other than 0".  Even with set to 1, it works for me - so, just to summarize in one place, we need:
1) gfx.color_management.enablev4 set to true
2) gfx.color_management.mode set to 1
3) particular display profile
and then it fails.  I will try changing my display profile.
Comment 20 Matthew Gregan [:kinetik] 2013-02-07 12:09:29 PST
I've got gfx.color_management.mode set to 2 (the only pref I've changed is to enable v4) and still see the problem in the original link (http://xkcd.com/386/) and many other XKCD images.  That particular image (http://imgs.xkcd.com/comics/duty_calls.png) does have iCCP, cHRM, and gAMA chunks.
Comment 21 Milan Sreckovic [:milan] 2013-02-07 13:32:19 PST
(In reply to Tobias Schula from comment #1)
> Created attachment 593204 [details]
> My display profile, made with dispcalGUI

Changing my display profile on Win 7 to this one, I see the problem.
Comment 22 Milan Sreckovic [:milan] 2013-02-07 14:01:42 PST
We don't like the profile, qcms_transform_create returns null.
Comment 23 Emanuel Hoogeveen [:ehoogeveen] 2013-02-07 14:24:56 PST
dispcalGUI profiles not being accepted by Firefox sounds like it should be a separate bug (although it's causing this one at the moment). Is there a bug open for that?
Comment 24 Emanuel Hoogeveen [:ehoogeveen] 2013-02-07 14:25:52 PST
Oh, sorry, it's the profile in the image that's not being accepted, right? I got a bit confused.
Comment 25 Milan Sreckovic [:milan] 2013-02-08 09:32:07 PST
Created attachment 711873 [details] [diff] [review]
The caching only seems to be setup for RGB (and not grayscale)

There are a few things we could look at, and I can open the bugs for those:
* PNG decoder doesn't handle qcms_transform_create returning nullptr
* minor optimization in qcms_transform_create - we allocate the transform before we've decided that we're going to use it
However, the issue here is that we only handle RGB_SIGNATURE in qcms_transform_precacheLUT_float.  No point in calling it when we have the grayscale data, as, according to libpng, RGB profile on grayscale data should be an error.
Comment 26 Benoit Girard (:BenWa) 2013-02-08 11:18:51 PST
Comment on attachment 711873 [details] [diff] [review]
The caching only seems to be setup for RGB (and not grayscale)

This change is helpful. But I think we should also fix any users of QCMS that don't handle null return values so that this problem doesn't occur in the future.
Comment 27 Milan Sreckovic [:milan] 2013-02-08 12:09:05 PST
(In reply to Benoit Girard (:BenWa) from comment #26)
> Comment on attachment 711873 [details] [diff] [review]
> The caching only seems to be setup for RGB (and not grayscale)
> 
> This change is helpful. But I think we should also fix any users of QCMS
> that don't handle null return values so that this problem doesn't occur in
> the future.

I actually have those changes locally, but wasn't sure if stuff like that belongs in this bug or if I should open another one?
Comment 28 Benoit Girard (:BenWa) 2013-02-08 12:18:32 PST
Best to open a new one since this patch wouldn't fix this issue to my full satisfaction (i.e. not handling other instances of a transform failing because of memory), but riding along is perfectly acceptable just a bit confusing.
Comment 29 Milan Sreckovic [:milan] 2013-02-08 12:53:48 PST
Created attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

Bug 839621 stops the security sensitive part of this, but this one actually fixes the visuals.
Comment 30 Milan Sreckovic [:milan] 2013-02-08 14:17:59 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Could embed nasty stuff in the image/profile.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Perhaps.  It does mention QCMS and grayscale, though it doesn't mention PNG.
Which older supported branches are affected by this flaw?
All, since QCMS landed.
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?
These are easy fixes, no problem backporting.
How likely is this patch to cause regressions; how much testing does it need?
The scenarios that would fail below now would not.  performance may be effected in the cases where we were displaying the wrong result and running into this problem.
Comment 31 Daniel Veditz [:dveditz] 2013-02-11 11:49:55 PST
lowering overall severity because this is not (yet?) a default pref setting.
Comment 32 Daniel Veditz [:dveditz] 2013-02-11 11:50:11 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

sec-approval+
Comment 33 Ryan VanderMeulen [:RyanVM] 2013-02-11 12:17:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a101d8932e91

Can we get a test for this?
Comment 34 Ed Morley [:emorley] 2013-02-12 10:51:45 PST
https://hg.mozilla.org/mozilla-central/rev/a101d8932e91
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 11:04:59 PST
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Comment 36 Milan Sreckovic [:milan] 2013-02-13 11:34:07 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a101d8932e91
> 
> Can we get a test for this?

Not sure we can.  It requires a different display profile, otherwise the error doesn't show up.  Do we have a way of setting a display profile through API for the test?
Comment 37 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-15 15:29:21 PST
Can we get an uplift nomination here to affected branches?
Comment 38 Milan Sreckovic [:milan] 2013-02-19 07:09:46 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Some black & white PNG, under ICCv4 colour profile will look wrong, and a potential security hole is opened.
Fix Landed on Version: 21
Risk to taking this patch (and alternatives if risky): 
Low.  The code will behave in some cases as if ICCv4 is turned off, which is the default.
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 39 Milan Sreckovic [:milan] 2013-02-19 07:12:31 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

I'm not sure we should uplift this, so asking for approval so that we can  discuss whether we should do it or not - and have an explicit decision in the bug.

This shows up with non-standard colour display profiles, and enabling ICCv4 (off by default).  Is the former possible on mobile?  Can we upload a different colour profile?  If not, we shouldn't hit this problem.
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-20 08:19:21 PST
(In reply to Milan Sreckovic [:milan] from comment #39)

> 
> This shows up with non-standard colour display profiles, and enabling ICCv4
> (off by default).  Is the former possible on mobile?  Can we upload a
> different colour profile?  If not, we shouldn't hit this problem.

Milan - can you ask around in Gfx team and see if someone can confirm if this pref is even set on Firefox OS?
Comment 41 Milan Sreckovic [:milan] 2013-02-20 08:45:04 PST
This pref is disabled on Firefox OS by default, and the Gfx team doesn't know how to change preferences on Firefox OS :-)
Comment 42 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-26 08:49:13 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

If this is disabled by default then no need to uplift.
Comment 43 Alex Keybl [:akeybl] 2013-03-04 10:43:21 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

Milan - can we request uplift to beta prior to uplifting to ESR17?
Comment 44 Milan Sreckovic [:milan] 2013-03-04 11:48:25 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a.  There from the start of qcms.
User impact if declined: 
This is sec-moderate, not with default setting, and with a modified colour profile.  I'm not convinced in needs to go to beta.  Note this is in aurora/21, it landed before the trains switched.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Low.  This change keeps us in the more common code path.
String or UUID changes made by this patch: n/a
Comment 45 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-04 13:03:32 PST
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

Approving for beta uplift, it hasn't been stated in this bug why we would go outside of the ESR scope to uplift this to that branch since this is only a sec-moderate and the ESR only promises high/critical severity fixes will be landed.  Do we have any reason to believe this bug will affect a large portion of our enterprise users? If not, we should hold off and see if any of them request this fix since we can make exceptions in those cases.
Comment 46 Milan Sreckovic [:milan] 2013-03-05 06:04:04 PST
Don't think we need it for ESR.  Checkin needed to beta.
Comment 47 Ryan VanderMeulen [:RyanVM] 2013-03-05 06:41:32 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/4cee1607a72e
Comment 48 Ioana (away) 2013-03-06 06:01:08 PST
Verified as fixed on Firefox 20 beta 3 - 20130305164032.

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