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

RESOLVED FIXED in Firefox 20

Status

()

Core
GFX: Color Management
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

({sec-high})

unspecified
mozilla21
x86
All
sec-high
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main20+][adv-esr1705+])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
When qcms_transform_create fails, like in 722831, a couple of places that call it indirectly through gfxPlatform methods are not ready for it.
(Assignee)

Comment 1

5 years ago
Created attachment 711950 [details] [diff] [review]
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.
Attachment #711950 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #711950 - Flags: review? → review?(bgirard)
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.
Attachment #711950 - Flags: review?(bgirard) → review?(jmuizelaar)
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...
Attachment #711950 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 4

5 years ago
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.
Attachment #711950 - Flags: sec-approval?
Attachment #711950 - Flags: sec-approval? → sec-approval+
status-b2g18: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox20: --- → ?
tracking-firefox21: --- → +
tracking-firefox-esr17: --- → ?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bfaea1b9b6d
Flags: in-testsuite?
Keywords: checkin-needed

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/2bfaea1b9b6d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(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."
status-firefox18: affected → ---
status-firefox19: affected → wontfix
Flags: needinfo?(milan)
(Assignee)

Comment 8

5 years ago
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.
Flags: needinfo?(milan)

Comment 9

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

Comment 10

5 years ago
Sure, let me prep the patches for esr17 and 20.
(Assignee)

Comment 11

5 years ago
Actually, the patches trivially apply to esr17, beta and aurora.
(Assignee)

Comment 12

5 years ago
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:
Attachment #711950 - Flags: approval-mozilla-esr17?
Attachment #711950 - Flags: approval-mozilla-beta?
Attachment #711950 - Flags: approval-mozilla-aurora?
tracking-b2g18: ? → +
tracking-firefox20: ? → +
tracking-firefox-esr17: ? → 20+
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.
Attachment #711950 - Flags: approval-mozilla-esr17?
Attachment #711950 - Flags: approval-mozilla-esr17+
Attachment #711950 - Flags: approval-mozilla-beta?
Attachment #711950 - Flags: approval-mozilla-beta+
Attachment #711950 - Flags: approval-mozilla-b2g18+
Attachment #711950 - Flags: approval-mozilla-aurora?
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
status-b2g18: affected → fixed
status-firefox20: affected → fixed
status-firefox-esr17: affected → fixed
Keywords: checkin-needed
Whiteboard: [adv-main20+][adv-esr1705+]

Updated

5 years ago
tracking-b2g18: + → 20+
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.
status-b2g18-v1.0.1: wontfix → affected
Group: core-security
You need to log in before you can comment on or make changes to this bug.