Last Comment Bug 876762 - (CVE-2013-1725) ABORT: bad scope for new JSObjects: 'js::IsObjectInContextCompartment(lccx.GetScopeForNewJSObjects(), cx)' under ReparentWrapper / document.open
(CVE-2013-1725)
: ABORT: bad scope for new JSObjects: 'js::IsObjectInContextCompartment(lccx.Ge...
Status: RESOLVED FIXED
[adv-main24+][adv-esr1709+]
: csectype-uninitialized, sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-28 10:09 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2014-11-19 20:11 PST (History)
11 users (show)
bobbyholley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
+
fixed
24+
fixed
24+
fixed
fixed


Attachments
Stack (126.21 KB, text/plain)
2013-05-28 10:09 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details
Stack (3.13 KB, text/plain)
2013-05-28 12:13 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details
Remove support for mScopeForNewJSObjects. v1 (17.09 KB, patch)
2013-05-28 18:26 PDT, Bobby Holley (:bholley) (busy with Stylo)
luke: review+
Details | Diff | Splinter Review
Remove support for mScopeForNewJSObjects on esr17. v1 r=luke (15.58 KB, patch)
2013-08-26 10:49 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2013-05-28 10:09:34 PDT
Created attachment 754887 [details]
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] 2013-05-28 11:30:06 PDT
Did you mean to attach something else, Ms2ger, that looks like a patch...
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2013-05-28 12:13:06 PDT
Created attachment 754944 [details]
Stack

Yep.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-05-28 18:26:16 PDT
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) (busy with Stylo) 2013-05-28 18:26:29 PDT
Created attachment 755124 [details] [diff] [review]
Remove support for mScopeForNewJSObjects. v1
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-05-28 18:27:52 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ebeafcde1234
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-05-28 18:39:34 PDT
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] 2013-05-29 04:40:23 PDT
Comment on attachment 755124 [details] [diff] [review]
Remove support for mScopeForNewJSObjects. v1

Good riddance!
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2013-05-29 09:51:35 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd394e3894ac
Comment 9 Andrew McCreight [:mccr8] 2013-05-29 11:06:01 PDT
Does this need to be backported?  I don't understand your first sentence in comment 6.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-05-29 17:19:21 PDT
(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] 2013-05-30 09:30:03 PDT
https://hg.mozilla.org/mozilla-central/rev/dd394e3894ac
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2013-05-30 10:00:59 PDT
The failing mechanism is being removed, so I don't think we need test coverage.
Comment 13 Al Billings [:abillings] 2013-06-13 16:59:10 PDT
(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) (busy with Stylo) 2013-06-13 17:08:29 PDT
(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] 2013-08-22 17:14:34 PDT
Release management, we should take this on ESR. This got missed along the way.
Comment 16 bhavana bajaj [:bajaj] 2013-08-23 14:07:29 PDT
needinfo on :bholley to help with the backport on esr17 branch
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2013-08-26 10:49:14 PDT
Created attachment 795527 [details] [diff] [review]
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) (busy with Stylo) 2013-08-26 10:50:06 PDT
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 2013-08-28 09:18:37 PDT
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] 2013-08-28 10:03:40 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/2c58bfd55714
Comment 21 Matt Wobensmith [:mwobensmith][:matt:] 2013-09-11 14:45:13 PDT
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) (busy with Stylo) 2013-09-11 15:25:53 PDT
(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:] 2013-09-16 10:13:32 PDT
Thanks Bobby. QA is going to pass on verification based on that info.
Comment 24 bhavana bajaj [:bajaj] 2013-10-15 14:50:32 PDT
NI on :bholley for a backport on b2g18.
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2013-10-16 01:44:38 PDT
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] 2013-10-16 05:54:47 PDT
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] 2013-10-16 06:08:15 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0c8834ee5bb2

Note You need to log in before you can comment on or make changes to this bug.