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

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

({sec-high})

unspecified
mozilla21
x86
All
Points:
---
Dependency tree / graph
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)

When qcms_transform_create fails, like in 722831, a couple of places that call it indirectly through gfxPlatform methods are not ready for it.
These are the only places that weren't already directly, or indirectly checking for the nullptr result.
Attachment #711950 - Flags: review?
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+
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+
https://hg.mozilla.org/mozilla-central/rev/2bfaea1b9b6d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
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."
Flags: needinfo?(milan)
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)
(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.
Sure, let me prep the patches for esr17 and 20.
Actually, the patches trivially apply to esr17, beta and aurora.
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?
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?
Whiteboard: [adv-main20+][adv-esr1705+]
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.