[FIX] nsDocumentViewer::LoadComplete doesn't guarantee that window object stays alive

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

unspecified
mozilla22
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19- wontfix, firefox20+ fixed, firefox21+ fixed, firefox22 fixed, firefox-esr1720+ fixed, b2g1820+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

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

Attachments

(1 attachment)

Assignee

Description

6 years ago
Posted patch patchSplinter Review
I was debugging bug 840019, and noticed that we may have a security bug there, not only
a null pointer crash.
SetReadyStateInternal dispatches readystatechanged, yet we dispatch load event
after that to window, and window is just a raw pointer.

The patch fixes both this bug and bug 840019.
Attachment #712612 - Flags: review?(hsivonen)
Attachment #712612 - Flags: review?(hsivonen) → review+
Assignee

Comment 1

6 years ago
Comment on attachment 712612 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't have the exploit, but making a raw pointer to nsCOMPtr shows pretty explicitly what is happening.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment will be about making sure null mDocument isn't accessed.

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should work with all the supported branches 

How likely is this patch to cause regressions; how much testing does it need?
Should be very safe.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old stuff
User impact if declined: possible security sensitive crashes
Testing completed (on m-c, etc.): hasn't landed yet
Risk to taking this patch (and alternatives if risky): should be safe 
String or UUID changes made by this patch: NA
Attachment #712612 - Flags: sec-approval?
Attachment #712612 - Flags: approval-mozilla-esr17?
Attachment #712612 - Flags: approval-mozilla-beta?
Attachment #712612 - Flags: approval-mozilla-b2g18?
Attachment #712612 - Flags: approval-mozilla-aurora?
Pretty sure we're totally done for Firefox 19, but I'd take this safe patch if we need to respin. If it doesn't make 19 though I'd prefer landing it next cycle.
Sec-approval+ but only on 2/19 or later (after we ship).
Attachment #712612 - Flags: sec-approval? → sec-approval+
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Pretty sure we're totally done for Firefox 19, but I'd take this safe patch
> if we need to respin. If it doesn't make 19 though I'd prefer landing it
> next cycle.

Yeah, agreed this is too late for FF19. What makes this bug more critical than other security fixes? Or perhaps you're suggesting the fix is so low risk that we should be comfortable with taking it in a 19.0.1 (if spun up)?
Assignee

Comment 6

6 years ago
Should be super-safe patch.
I would take it in a re-spin of 19.0.0, but if do then we have to take it on the ESR release as well. It's not "worse" than other criticals, but being a simple patch we don't want to leave it hanging out there for a cycle.

We probably don't want to take it in a 19.0.1 unless we're also planning an ESR extra-release.
Assignee

Updated

6 years ago
Summary: nsDocumentViewer::LoadComplete doesn't guarantee that window object stays alive → [FIX] nsDocumentViewer::LoadComplete doesn't guarantee that window object stays alive
Comment on attachment 712612 [details] [diff] [review]
patch

Approving for beta/aurora with the intent that it land on Firefox 20 which is Aurora today and will be on Beta tomorrow morning.  Flagging mozilla-release approval nom in case there is a 19.0.1 so this will be on the radar.

We've also gone to build with the ESR17 shipping alongside FF 19 so approving for ESR17 landing that will go with FF20.
Attachment #712612 - Flags: approval-mozilla-release?
Attachment #712612 - Flags: approval-mozilla-esr17?
Attachment #712612 - Flags: approval-mozilla-esr17+
Attachment #712612 - Flags: approval-mozilla-beta?
Attachment #712612 - Flags: approval-mozilla-beta+
Attachment #712612 - Flags: approval-mozilla-aurora?
Attachment #712612 - Flags: approval-mozilla-aurora+
Comment on attachment 712612 [details] [diff] [review]
patch

Approving for v1-train uplift.
Attachment #712612 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/mozilla-central/rev/979456163be1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Is there a PoC we can use to verify this is fixed?
Assignee

Comment 14

6 years ago
No PoC
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.
blocking-b2g: --- → tef+
Duplicate of this bug: 853511
Tagging [qa-] since we cannot verify this fixed based on comment 13 and 14. Please drop this tag and add the verifyme keyword if you can advise us on verification. Thank you.
Whiteboard: [adv-main20+][adv-esr1705+] → [adv-main20+][adv-esr1705+][qa-]
Attachment #712612 - Flags: approval-mozilla-release?
Group: core-security
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.