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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ but only on 2/19 or later (after we ship).

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 5

[Alex Keybl [:akeybl]]

(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)?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Should be super-safe patch.

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 712612 [details] [diff] [review]
patch

Approving for v1-train uplift.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/979456163be1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/979456163be1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65ce204912f1
https://hg.mozilla.org/releases/mozilla-beta/rev/fe0904c73e1c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/393e9a617ffa
https://hg.mozilla.org/releases/mozilla-esr17/rev/7c616543869d

Comment 13

[u279076]

Is there a PoC we can use to verify this is fixed?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

No PoC

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8ec3367e3624

Comment 18

[u279076]

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.