851987 - Crash [@ nsGlobalWindow::GetChildWindow] with event.view, lookupMethod

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

The crash is reliable under ASan.  It's less reliable in a normal debug build.

While the testcase reminds me of bug 809674, I think this is regression from within the last few days.

Comment 1

[Jesse Ruderman]

Attachment: stack (ASan)

Comment 2

[Bobby Holley (:bholley)]

Yes, this is a regression from bug 850517.

Comment 3

[Bobby Holley (:bholley)]

Attachment: QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

Comment 4

[Bobby Holley (:bholley)]

So, the issue here is that nsIDOMWindow can actually be implemented by XPCWrappedJS in various situations. So a static cast from nsIDOMWindow to nsGlobalWindow isn't safe. I'm pretty sure nsPIDOMWindow is safe, but I'd appreciate if someone could confirm that.

bz did something similar in bug 823228, so aurora is affected as well.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 726006 [details] [diff] [review]
QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

Oh, yikes.  Good catch.

r=me

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 726006 [details] [diff] [review]
QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not super easily. The issue here is a mis-cast, which means that the attacker would need to do some fancy heap exploit stuff to make this work.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Nope.

Which older supported branches are affected by this flaw?

Aurora.

If not all supported branches, which bug introduced the flaw?

bug 823228 and bug 850517.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy backport. The aurora backport should just be one of the two chunks of this patch.

How likely is this patch to cause regressions; how much testing does it need?

Not risky.

Comment 7

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

Comment on attachment 726006 [details] [diff] [review]
QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

Please create an Aurora patch as well and nominate it for inclusion there. 

Sec-approval+

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 726006 [details] [diff] [review]
QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

Requesting aurora approval for this patch. Note that this patch has two hunks, one of which will apply on aurora and one of which won't. That's to be expected, we just want to push the hunk in XPCWrappedNativeXrayTraits::resolveOwnProperty.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 823228
User impact if declined: security
Testing completed (on m-c, etc.): Going into inbound now.
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: None.

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/632ec7d6a9ae

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/632ec7d6a9ae

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 726006 [details] [diff] [review]
QI to nsPIDOMWindow instead of nsIDOMWindow in XPCWrappedJS. v1

low risk fix for a sec-crit regression .Approving for uplift .

[Please keep https://bugzilla.mozilla.org/show_bug.cgi?id=851987#c8 in mind when landing this on aurora ]

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/441c03c44af2

Comment 13

[Bobby Holley (:bholley)]

This testcase uses |Components|, so I don't think it's worth checking in at this point.