Closed Bug 840263 Opened 7 years ago Closed 7 years ago
Document Viewer::Load Complete doesn't guarantee that window object stays alive
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+
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
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)?
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.
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-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+
Is there a PoC we can use to verify this is fixed?
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.
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-]
You need to log in before you can comment on or make changes to this bug.