The default bug view has changed. See this FAQ.
Bug 722831 (CVE-2013-0792)

some grayscale png images display improperly when "gfx.color_management.enablev4" enabled

RESOLVED FIXED in Firefox 20

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Tobias Schula, Assigned: milan)

Tracking

({sec-moderate})

11 Branch
mozilla21
x86
All
sec-moderate
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 wontfix, firefox20+ verified, firefox21+ fixed, firefox-esr1720+ wontfix, b2g1820+ wontfix, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [adv-main20+])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 593204 [details]
My display profile, made with dispcalGUI

Comment 2

5 years ago
Same here on win7 x64

Comment 3

5 years ago
It happens with my OEM dell u2711 profile, but is not reproduced if I set preinstalled Adobe RGB profile.

Comment 4

5 years ago
Created attachment 609107 [details]
Dell u2711 OEM display profile

Comment 5

5 years ago
Created attachment 609108 [details]
Adobe RGB 1998 from photoshop CS5 distro

Updated

5 years ago
Component: Untriaged → GFX: Color Management
Product: Firefox → Core
QA Contact: untriaged → color-management
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Group: mozilla-confidential
Component: GFX: Color Management → ImageLib
QA Contact: color-management → imagelib
Group: mozilla-confidential → core-security

Updated

5 years ago
OS: Linux → All
Summary: some grayscale png imagese display improperly when "gfx.color_management.enablev4" enabled → some grayscale png images display improperly when "gfx.color_management.enablev4" enabled
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?
Assignee: nobody → joe

Comment 9

5 years ago
(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
I would be surprised if you couldn't embed such a profile inside an image. For now I'll say this is sec-high.
Keywords: sec-high
Duplicate of this bug: 804137
Duplicate of this bug: 791456
Any updates on getting this sec-high fixed? It has been over six months.
It appears that this was fixed by some refactoring or another. We definitely handle null mTransforms now.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
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).
This is still broken in current nightlies.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 17

4 years ago
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).
(Reporter)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
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.
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.
(Assignee)

Comment 21

4 years ago
(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.
(Assignee)

Comment 22

4 years ago
We don't like the profile, qcms_transform_create returns null.
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?
Oh, sorry, it's the profile in the image that's not being accepted, right? I got a bit confused.
(Assignee)

Comment 25

4 years ago
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.
Attachment #711873 - Flags: feedback?(joe)
Attachment #711873 - Flags: feedback?(jmuizelaar)
Attachment #711873 - Flags: feedback?(bgirard)
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.
Attachment #711873 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Comment 27

4 years ago
(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?
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.
(Assignee)

Updated

4 years ago
Assignee: joe → milan
(Assignee)

Updated

4 years ago
Depends on: 839621
(Assignee)

Comment 29

4 years ago
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.
Attachment #711873 - Attachment is obsolete: true
Attachment #711873 - Flags: feedback?(joe)
Attachment #711873 - Flags: feedback?(jmuizelaar)
Attachment #711951 - Flags: review?(bgirard)

Updated

4 years ago
Attachment #711951 - Flags: review?(bgirard) → review+
(Assignee)

Comment 30

4 years ago
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.
Attachment #711951 - Flags: sec-approval?
lowering overall severity because this is not (yet?) a default pref setting.
status-b2g18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox20: --- → +
tracking-firefox21: --- → +
Keywords: sec-high → sec-moderate
Comment on attachment 711951 [details] [diff] [review]
The caching only works for RGB, so only call for RGB

sec-approval+
Attachment #711951 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a101d8932e91

Can we get a test for this?
status-firefox21: affected → fixed
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a101d8932e91
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
status-b2g18-v1.0.1: --- → affected
(Assignee)

Comment 36

4 years ago
(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?
Can we get an uplift nomination here to affected branches?
Flags: needinfo?(milan)
status-firefox19: affected → wontfix
(Assignee)

Comment 38

4 years ago
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.
Attachment #711951 - Flags: approval-mozilla-esr17?
Flags: needinfo?(milan)
(Assignee)

Comment 39

4 years ago
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.
Attachment #711951 - Flags: approval-mozilla-b2g18?
(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?
(Assignee)

Comment 41

4 years ago
This pref is disabled on Firefox OS by default, and the Gfx team doesn't know how to change preferences on Firefox OS :-)
tracking-b2g18: --- → 20+
tracking-firefox-esr17: --- → 20+
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.
Attachment #711951 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18-
status-b2g18: affected → wontfix
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: affected → wontfix

Updated

4 years ago
Attachment #711951 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
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?
Attachment #711951 - Flags: approval-mozilla-esr17+ → approval-mozilla-esr17?
(Assignee)

Comment 44

4 years ago
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
Attachment #711951 - Flags: approval-mozilla-beta?
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.
Attachment #711951 - Flags: approval-mozilla-esr17?
Attachment #711951 - Flags: approval-mozilla-esr17-
Attachment #711951 - Flags: approval-mozilla-beta?
Attachment #711951 - Flags: approval-mozilla-beta+
(Assignee)

Comment 46

4 years ago
Don't think we need it for ESR.  Checkin needed to beta.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/4cee1607a72e
status-firefox20: affected → fixed
Keywords: checkin-needed
status-firefox-esr17: affected → wontfix

Comment 48

4 years ago
Verified as fixed on Firefox 20 beta 3 - 20130305164032.
status-firefox20: fixed → verified
Whiteboard: [adv-main20+]
Alias: CVE-2013-0792
Group: core-security
You need to log in before you can comment on or make changes to this bug.