876762 - (CVE-2013-1725) ABORT: bad scope for new JSObjects: 'js::IsObjectInContextCompartment(lccx.GetScopeForNewJSObjects(), cx)' under ReparentWrapper / document.open

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Stack

STR:

1. Paste the following into the live dom viewer

<iframe src=about:blank></iframe>
...<script>
onload = function() {
document.open()

}
</script>

2. Type something random on the empty line

Comment 1

[Andrew McCreight [:mccr8]]

Did you mean to attach something else, Ms2ger, that looks like a patch...

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Stack

Yep.

Comment 3

[Bobby Holley (:bholley)]

So, this is basically the result of the stuff in BindingUtils.cpp creating an XPCLazyCallContext without either passing in a wrapper or calling SetScopeForNewJSObjects, which is verboten. But I'm pretty sure this whole mechanism is unnecessary since CPG, since we can now just grab the scope off of cx->compartment.

I'll attach a patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Remove support for mScopeForNewJSObjects. v1

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ebeafcde1234

Comment 6

[Bobby Holley (:bholley)]

So, until the recent rooting work, it looks like mScopeForNewJSObjects would remain uninitialized for XPCCallContexts without a wrapper unless somebody explicitly set it, and it's clear that here (and perhaps elsewhere) we don't.

I'm not really sure what the severity of "entering the compartment of a JSObject* that is uninitialized stack memory" is, but I'm going to guess sec-high to be safe. This would likely be pretty hard to exploit though.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 755124 [details] [diff] [review]
Remove support for mScopeForNewJSObjects. v1

Good riddance!

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd394e3894ac

Comment 9

[Andrew McCreight [:mccr8]]

Does this need to be backported?  I don't understand your first sentence in comment 6.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Does this need to be backported?  I don't understand your first sentence in
> comment 6.

When these objects became rooted, they became null-initialized.

We could try to spot-fix the sketchy parts on branches, but I think it's safe to just backport this patch.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd394e3894ac

Comment 12

[Bobby Holley (:bholley)]

The failing mechanism is being removed, so I don't think we need test coverage.

Comment 13

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

(In reply to Bobby Holley (:bholley) from comment #10)
> (In reply to Andrew McCreight [:mccr8] from comment #9)
> > Does this need to be backported?  I don't understand your first sentence in
> > comment 6.
> 
> When these objects became rooted, they became null-initialized.
> 
> We could try to spot-fix the sketchy parts on branches, but I think it's
> safe to just backport this patch.

How far back does this need to go? Does it affect ESR17?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #13)
> How far back does this need to go? Does it affect ESR17?

It does.

Comment 15

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

Release management, we should take this on ESR. This got missed along the way.

Comment 16

[bhavana bajaj [:bajaj]]

needinfo on :bholley to help with the backport on esr17 branch

Comment 17

[Bobby Holley (:bholley)]

Attachment: Remove support for mScopeForNewJSObjects on esr17. v1 r=luke

I can't compile esr17 locally, so I don't know if this compiles. But we don't
have try either, so I guess we should just crashland it.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 795527 [details] [diff] [review]
Remove support for mScopeForNewJSObjects on esr17. v1 r=luke

[Approval Request Comment]
User impact if declined: sec 
Fix Landed on Version: 24
Risk to taking this patch (and alternatives if risky): Lowish risk 
String or UUID changes made by this patch: None

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 795527 [details] [diff] [review]
Remove support for mScopeForNewJSObjects on esr17. v1 r=luke

There's still time so please do land and check that the landing sticks on esr17.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/2c58bfd55714

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Comment 12 says that by removing some code, this no longer needs a test. If that is the case, let us know. If not, QA would like to verify that it's fixed. Thanks.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Matt Wobensmith from comment #21)
> Comment 12 says that by removing some code, this no longer needs a test. If
> that is the case, let us know. If not, QA would like to verify that it's
> fixed. Thanks.

yes, this patch doesn't need verification.

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Bobby. QA is going to pass on verification based on that info.

Comment 24

[bhavana bajaj [:bajaj]]

NI on :bholley for a backport on b2g18.

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 795527 [details] [diff] [review]
Remove support for mScopeForNewJSObjects on esr17. v1 r=luke

I would guess that the esr17 fix will also apply to b2g18. Let me know if there are nontrivial conflicts.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 795527 [details] [diff] [review]
Remove support for mScopeForNewJSObjects on esr17. v1 r=luke

Got a+ from bajaj over email to land this.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8834ee5bb2

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/0c8834ee5bb2