JSD often uses JSContexts without pushing

RESOLVED FIXED in Firefox 24

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug, {sec-moderate})

unspecified
mozilla24
x86
Mac OS X
sec-moderate
Points:
---

Firefox Tracking Flags

(firefox24 fixed, firefox-esr17 wontfix, firefox-esr24 unaffected, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 unaffected)

Details

(Whiteboard: [adv-main24-])

Attachments

(10 attachments, 3 obsolete attachments)

1.58 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
23.91 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
2.53 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
16.33 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
11.39 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
5.72 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
4.13 KB, patch
krizsa
: review+
Details | Diff | Splinter Review
943 bytes, patch
krizsa
: review+
Details | Diff | Splinter Review
7.59 KB, patch
Details | Diff | Splinter Review
3.87 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Sigh. This can lead to compartment mismatches. I'm marking this as sec-moderate because it only really matters when someone is debugging a page with Firebug, and it's not likely that many people will do that to malicious pages.
Group: dom-core-security → core-security
From Honza, in bug 821733:

Another STR:

1) Install Firebug 1.11.4, https://addons.mozilla.org/en-US/firefox/addon/firebug/
2) Open https://getfirebug.com/tests/1.11/commandLine/4434/issue4434.html
3) Follow the steps -> CRASH

Crash in: js::CompartmentChecker::fail 	js/src/jscntxtinlines.h:165

https://crash-stats.mozilla.com/report/index/bp-d4861756-c34a-4288-b804-b1b322130607

Btw. I believe the crash started in this App Changeset: ddb7b23166ef
I'm writing a bunch of patches to remove jsdc->dumbContext and just use the SafeJSContext. Not really the way I wanted to spend my morning, but oh well.
https://tbpl.mozilla.org/?tree=Try&rev=7287732f7bf5
Created attachment 759904 [details] [diff] [review]
Part 1 - Add an RAII class to JSD to save/restore exception state. v1
Attachment #759904 - Flags: review?(gkrizsanits)
Created attachment 759905 [details] [diff] [review]
Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1
Attachment #759905 - Flags: review?(gkrizsanits)
Created attachment 759906 [details] [diff] [review]
Part 3 - Make JS_ClearScriptTraps take a runtime directly. v1

This obviates the need for a context at the JSD callsite.
Attachment #759906 - Flags: review?(gkrizsanits)
Created attachment 759907 [details] [diff] [review]
Part 4 - Include XPConnect and grab the SafeJSContext when nsContentUtils has been torn down. v1

We don't really want to be adding xpconnect/src to more LOCAL_INCLUDES, but JSD
is going away, so let's just do this.
Attachment #759907 - Flags: review?(gkrizsanits)
Created attachment 759908 [details] [diff] [review]
Part 5 - Replace usage of dumbContext with AutoSafeJSContext. v1

dumbContext ends up with jsdc->glob as its default global, so we have to be
very careful to audit for any places where the code might be assuming that
its cx is in the compartment of jsdc->glob. Luckily, the code already seems
pretty explicit about its compartments.
Attachment #759908 - Flags: review?(gkrizsanits)
Created attachment 759909 [details] [diff] [review]
Part 6 - Remove JSD_GetDefaultJSContext and fix callers. v1
Attachment #759909 - Flags: review?(gkrizsanits)
Created attachment 759910 [details] [diff] [review]
Part 7 - Remove dumbContext. v1

Whew.
Attachment #759910 - Flags: review?(gkrizsanits)
Created attachment 759911 [details] [diff] [review]
Part 8 - Push in a few other suspicious places. v1
Attachment #759911 - Flags: review?(gkrizsanits)
Looks like my strategy for jsd_DestroyScriptHookProc went orange on opt builds. Terrence is going to write a patch that lets me use a JSRuntime to root. Once he uploads that, I'll modify Part 4 to do that and drop the LOCAL_INCLUDE.
Attachment #759907 - Flags: review?(gkrizsanits)
Flags: needinfo?(terrence)
Created attachment 760029 [details] [diff] [review]
v0: Add constructor for Rooted<>(JSRuntime*)
Attachment #760029 - Flags: review?(wmccloskey)
Flags: needinfo?(terrence)
Awesome, thanks Terrence!
Created attachment 760032 [details] [diff] [review]
Part 0 - Add constructor for Rooted<>(JSRuntime*). v1

Actually add the new test.
Attachment #760029 - Attachment is obsolete: true
Attachment #760029 - Flags: review?(wmccloskey)
Attachment #760032 - Flags: review?(wmccloskey)
Created attachment 760037 [details] [diff] [review]
Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1
Attachment #759907 - Attachment is obsolete: true
Attachment #760037 - Flags: review?(gkrizsanits)
https://tbpl.mozilla.org/?tree=Try&rev=7c809db43d90
(In reply to Bobby Holley (:bholley) from comment #0)
> Sigh.

Indeed...
Attachment #759904 - Flags: review?(gkrizsanits) → review+
Comment on attachment 759905 [details] [diff] [review]
Part 2 - Stop using clunky C API in JSD and start using RAII classes. v1

Review of attachment 759905 [details] [diff] [review]:
-----------------------------------------------------------------

The dumbContext and cx usage is not very consistent in this patch (sometimes you introduce cx then keep using the dumbContext...) but since you remove the whole thing few patches later I'm just going to ignore it. A few things:

+    JSAutoCompartment ac2(cx, jsdc->glob);
is the 2 really needed here?

in jsd_GetValueString seems like you forgot the auto request
Attachment #759905 - Flags: review?(gkrizsanits) → review+
Attachment #759906 - Flags: review?(gkrizsanits) → review+
Comment on attachment 760037 [details] [diff] [review]
Part 4 - Root directly with a runtime in jsd_DestroyScriptHookProc. v1

Review of attachment 760037 [details] [diff] [review]:
-----------------------------------------------------------------

Why did the previous version go orange? That sounds scary to me...
Attachment #760037 - Flags: review?(gkrizsanits) → review+
Comment on attachment 759908 [details] [diff] [review]
Part 5 - Replace usage of dumbContext with AutoSafeJSContext. v1

Review of attachment 759908 [details] [diff] [review]:
-----------------------------------------------------------------

jsd_NewValue(JSDContext* jsdc, jsval value)
 {
-    JS::RootedValue val(jsdc->dumbContext, value);
-    JSAutoRequest ar(jsdc->dumbContext);
+    AutoSafeJSContext cx;
+    JS::RootedValue val(cx, value);
+    JSAutoRequest ar(cx);

unnecessary JSAutoRequest

 jsd_DropValue(JSDContext* jsdc, JSDValue* jsdval)
 {
     JS_ASSERT(jsdval->nref > 0);
     if(0 == --jsdval->nref)
     {
         jsd_RefreshValue(jsdc, jsdval);
         if(JSVAL_IS_GCTHING(jsdval->val))
         {
-            JSAutoRequest ar(jsdc->dumbContext);
-            JSAutoCompartment ac(jsdc->dumbContext, jsdc->glob);
-            JS_RemoveValueRoot(jsdc->dumbContext, &jsdval->val);
+            AutoSafeJSContext cx;
+            JSAutoRequest ar(cx);

here too
Attachment #759908 - Flags: review?(gkrizsanits) → review+
Attachment #759909 - Flags: review?(gkrizsanits) → review+
Comment on attachment 759910 [details] [diff] [review]
Part 7 - Remove dumbContext. v1

Review of attachment 759910 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #759910 - Flags: review?(gkrizsanits) → review+
Comment on attachment 759911 [details] [diff] [review]
Part 8 - Push in a few other suspicious places. v1

Review of attachment 759911 [details] [diff] [review]:
-----------------------------------------------------------------

-    JSContext *cx = JSD_GetJSContext (mCx, mThreadState);
+    AutoPushJSContext cx(JSD_GetJSContext (mCx, mThreadState));
 
     JS::RootedValue jv(cx);
 
     estate = JS_SaveExceptionState (cx);
     JS_ClearPendingException (cx);
 
     nsCxPusher pusher;
     pusher.Push(cx);

the nsCxPusher should now just go
Attachment #759911 - Flags: review?(gkrizsanits) → review+
Comment on attachment 760032 [details] [diff] [review]
Part 0 - Add constructor for Rooted<>(JSRuntime*). v1

Review of attachment 760032 [details] [diff] [review]:
-----------------------------------------------------------------

We already have rooters in the runtime via mainThread. Can you try re-using those by calling getMainThread or something?
Attachment #760032 - Flags: review?(wmccloskey)
Created attachment 761018 [details] [diff] [review]
Part 0 - Add constructor For Rooted<>(JSRutnime *). v2
Attachment #760032 - Attachment is obsolete: true
Attachment #761018 - Flags: review?(wmccloskey)
Created attachment 761076 [details] [diff] [review]
add JSRuntime ctor for Rooted

Sorry, this is what I meant.
Attachment #761076 - Flags: review?(terrence)
Comment on attachment 761076 [details] [diff] [review]
add JSRuntime ctor for Rooted

Review of attachment 761076 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #761076 - Flags: review?(terrence) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> The dumbContext and cx usage is not very consistent in this patch (sometimes
> you introduce cx then keep using the dumbContext...) but since you remove
> the whole thing few patches later I'm just going to ignore it. A few things:
> 
> +    JSAutoCompartment ac2(cx, jsdc->glob);
> is the 2 really needed here?

Technically not. I think avoiding shadowing is always clearer, though.

> in jsd_GetValueString seems like you forgot the auto request

Good catch.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> Why did the previous version go orange? That sounds scary to me...

Because I was trying to grab the SafeJSContext to use for rooting, but nsContentUtils had already been shut down by that point. The point here is that, with billm's patch, we can just root directly with the runtime, so it's simpler to do that.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23)
> -    JSContext *cx = JSD_GetJSContext (mCx, mThreadState);
> +    AutoPushJSContext cx(JSD_GetJSContext (mCx, mThreadState));
>  
>      JS::RootedValue jv(cx);
>  
>      estate = JS_SaveExceptionState (cx);
>      JS_ClearPendingException (cx);
>  
>      nsCxPusher pusher;
>      pusher.Push(cx);
> 
> the nsCxPusher should now just go

Agreed - technically it's not entirely equivalent (because the AutoPushJSContext might not push), but this code doesn't seem to be relying on that.
https://tbpl.mozilla.org/?tree=Try&rev=a3cf185fe73d
https://tbpl.mozilla.org/?tree=Try&rev=19827850b9a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/444fffdcf768
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77b50dfb8b17
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56c790b9bdcc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d775f43176c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58bd67e4294a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c521067ed542
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30e704196d0a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5f37e9cb1334
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/81d4a3cf9e14
Attachment #761018 - Flags: review?(wmccloskey)
https://hg.mozilla.org/mozilla-central/rev/444fffdcf768
https://hg.mozilla.org/mozilla-central/rev/77b50dfb8b17
https://hg.mozilla.org/mozilla-central/rev/56c790b9bdcc
https://hg.mozilla.org/mozilla-central/rev/d775f43176c8
https://hg.mozilla.org/mozilla-central/rev/58bd67e4294a
https://hg.mozilla.org/mozilla-central/rev/c521067ed542
https://hg.mozilla.org/mozilla-central/rev/30e704196d0a
https://hg.mozilla.org/mozilla-central/rev/5f37e9cb1334
https://hg.mozilla.org/mozilla-central/rev/81d4a3cf9e14
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [adv-main24+]
Whiteboard: [adv-main24+] → [adv-main24-]
status-firefox-esr17: --- → wontfix
status-firefox-esr24: --- → unaffected
status-b2g18: --- → affected
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → unaffected
status-b2g18: affected → wontfix
status-b2g-v1.1hd: affected → wontfix
Group: core-security
You need to log in before you can comment on or make changes to this bug.