722831 - (CVE-2013-0792) some grayscale png images display improperly when "gfx.color_management.enablev4" enabled

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tobias Schula]

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

Attachment: My display profile, made with dispcalGUI

Comment 2

[Ilia Pozhilov]

Same here on win7 x64

Comment 3

[Ilia Pozhilov]

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

Comment 4

[Ilia Pozhilov]

Attachment: Dell u2711 OEM display profile

Comment 5

[Ilia Pozhilov]

Attachment: Adobe RGB 1998 from photoshop CS5 distro

Comment 6

[Benoit Girard (:BenWa)]

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

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

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]

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

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 13

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

Any updates on getting this sec-high fixed? It has been over six months.

Comment 14

[Joe Drew (not getting mail)]

It appears that this was fixed by some refactoring or another. We definitely handle null mTransforms now.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

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

This is still broken in current nightlies.

Comment 17

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

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]

(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] (needinfo for best results)]

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

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] (needinfo for best results)]

(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] (needinfo for best results)]

We don't like the profile, qcms_transform_create returns null.

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

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

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] (needinfo for best results)]

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

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] (needinfo for best results)]

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

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] (needinfo for best results)]

Attachment: 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] (needinfo for best results)]

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

lowering overall severity because this is not (yet?) a default pref setting.

Comment 32

[Daniel Veditz [:dveditz]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a101d8932e91

Can we get a test for this?

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a101d8932e91

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

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] (needinfo for best results)]

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

Can we get an uplift nomination here to affected branches?

Comment 38

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

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] (needinfo for best results)]

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]

(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] (needinfo for best results)]

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]

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

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] (needinfo for best results)]

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]

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] (needinfo for best results)]

Don't think we need it for ESR.  Checkin needed to beta.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4cee1607a72e

Comment 48

[Ioana (away)]

Verified as fixed on Firefox 20 beta 3 - 20130305164032.