839621 - qcms_transform_create can return null, and some of the calling code isn't ready for it

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

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

When qcms_transform_create fails, like in 722831, a couple of places that call it indirectly through gfxPlatform methods are not ready for it.

Comment 1

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

Attachment: Check for null pointers (indirectly) returned by qcms_transform_create

These are the only places that weren't already directly, or indirectly checking for the nullptr result.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 711950 [details] [diff] [review]
Check for null pointers (indirectly) returned by qcms_transform_create

I haven't touched this code before so forwarding to Jeff.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 711950 [details] [diff] [review]
Check for null pointers (indirectly) returned by qcms_transform_create

Review of attachment 711950 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if it would be better to not return null from qcms_transform_create and just have an empty transform instead...

Comment 4

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

Comment on attachment 711950 [details] [diff] [review]
Check for null pointers (indirectly) returned by qcms_transform_create

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unclear, now that Bug 722831 is removing the calls we knew were ending here.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really.  Null pointer checks.
Which older supported branches are affected by this flaw?
All, since QMCS 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?

How likely is this patch to cause regressions; how much testing does it need?
Null pointer checks, we should be fine.

I do not think we need to uplift this. The cause has been removed with fixing 722831, so we don't have a currently known way of hitting this bug.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bfaea1b9b6d

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2bfaea1b9b6d

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> I do not think we need to uplift this. The cause has been removed with
> fixing 722831, so we don't have a currently known way of hitting this bug.

So you're suggesting we uplift bug 722831 instead? Bug in bug 722831 comment you seemed to say this bug is the right fix. "Bug 839621 stops the security sensitive part of this, but this one actually fixes the visuals."

Comment 8

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

Right - I was suggesting we just uplift bug 722831.  While this bug contains the eventual security related failure, bug 722831 is the only (known) way of getting to this code.  This one is basically future proofing.  However, not knowing the STR to hit this bug once bug 722831 is fixed may not be enough of a reason to keep this one from being uplifted as well.  I have to defer somebody more experienced on that.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Right - I was suggesting we just uplift bug 722831.  While this bug contains
> the eventual security related failure, bug 722831 is the only (known) way of
> getting to this code.  This one is basically future proofing. 

Sounds good, we'll take bug 722831 on Aurora/Beta 20.

> However, not
> knowing the STR to hit this bug once bug 722831 is fixed may not be enough
> of a reason to keep this one from being uplifted as well.  I have to defer
> somebody more experienced on that.

Who can we ask? If we're not certain, we should just land these low risk null checks sooner rather than later to get testing.

Comment 10

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

Sure, let me prep the patches for esr17 and 20.

Comment 11

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

Actually, the patches trivially apply to esr17, beta and aurora.

Comment 12

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

Comment on attachment 711950 [details] [diff] [review]
Check for null pointers (indirectly) returned by qcms_transform_create

On Alex's suggestion in Comment 9, asking for uplifts.
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Null check, but with bug 722831 fixed, we don't have an STR to hit these with null pointers.
Fix Landed on Version:21
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 711950 [details] [diff] [review]
Check for null pointers (indirectly) returned by qcms_transform_create

[Triage Comment]

We already have this on Aurora now, post-merge so only approving for uplift to mozilla-beta (FF20) and mozilla-esr17.  Also go ahead with landing to mozilla-b2g18 branch tip if it applies cleanly.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07a8701aa96a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/48a165b5efd9
https://hg.mozilla.org/releases/mozilla-esr17/rev/757cf20a83c6

Comment 15

[Alex Keybl [:akeybl]]

Since Firefox 20 is now released, v1.0.1 branches are still open, and this is considered low risk, we can uplift to v1.0.1. Marking as tef+ and flipping status-b2g18-v1.0.1 to affected.