834732 - Do a better job of maintaining the JS context stack

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The code creates a special JSContext for the sandbox and pushes it. After evaluation, however, it begins to use the caller's cx again while the sandcx is still on the stack. There are calls to JS_Wrap*, which mean that we'll enter the XPConnect wrapper callbacks with |cx != nsContentUtils::GetCurrentJSContext()|, which is bad.

I don't believe this is a security issue, because the sandbox's cx is presumably always less privileged than its caller, so it's not a problem security-wise if we use that instead of the caller. We should nonetheless fix this.

Comment 1

[Bobby Holley (:bholley)]

So actually, my assertions seem to be catching a number of other places in the code where we don't properly push a cx. :-(

Given that we depend on the JSContext stack to determine SOW access (see the nsContentUtils::GetCurrentJSContext() assertion in AccessCheck.cpp), I'm marking this s-s for now.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

This is green, modulo one b-c test that hasn't come in yet. Uploading patches and flagging Blake for review.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Get rid of footgun bool param for nsCxPusher and use an explicit enum. v1

The goal here is to get rid of this crap entirely, and make nsCxPusher always
push. But that's a scary change, so we do it in chunks. This patch, in particular,
should have zero behavioral change. This means preserving some very wrong behavior.
For instance, currently SafeAutoJSContext never pushes a damn thing, because the
safe JSContext doesn't have an associated nsIScriptContext. We preserve this
behavior, and in fact convert various similarly-buggy consumers to
SafeAutoJSContext, so that we can hoist the behavioral change into a subsequent
patch.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make SafeAutoJSContext actually push something. v1

Currently it never does, because the SafeJSContext doesn't have an
nsIScriptContext behind it. :-(

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make nsDocShell::AddState push the JSContext if it grabs one, and remove push in nsStructuredCloneContainer. v1

This gets rid of one of the last consumers of REQUIRE_SCRIPT_CONTEXT.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 4 - Move cx pushing into nsPluginProtoChainInstallRunner. v2

This gets rid of the last use of REQUIRE_SCRIPT_CONTEXT. \o/

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove PushBehavior. v1

Now that we only have ALWAYS_PUSH and ASSERT_SCRIPT_CONTEXT, we have uniform
release-mode behavior everywhere. Remove the crap.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make nsCxPusher.Push(JSContext*) infallible. v1

We leave the nsIDOMEventTarget* versions fallible for now, but this makes the
common case a lot simpler. Note that this means that pushing a null JSContext,
a bug, is no longer handled at runtime. But I think we should just assert
against it, since there are already callers that don't check the return value.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 7 - Implement stricter cx handling in xpc_EvalInSandbox. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 8 - Push a cx in nsWindowSH::NewResolve. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 9 - Push a cx in nsPrincipal::SetDomain. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 10 - Push a cx in AdoptNode. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 11 - Improve pushing in nsJSEnvironment. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 12 - Push any cx we grab off the object for XPCWrappedJS. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 13 - Assert proper cx stack handling in WrapperFactory::Rewrap. v1

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708042 [details] [diff] [review]
Part 2 - Make SafeAutoJSContext actually push something. v1

It is mind boggling to me that this code was written like this and has lasted as long as it has.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708046 [details] [diff] [review]
Part 6 - Make nsCxPusher.Push(JSContext*) infallible. v1

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

::: layout/base/nsRefreshDriver.cpp
@@ +556,5 @@
>  
>    nsCxPusher pusher;
> +  pusher.PushNull();
> +  DoTick();
> +  pusher.Pop();

Is there any reason to keep the explicit Pop here?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708047 [details] [diff] [review]
Part 7 - Implement stricter cx handling in xpc_EvalInSandbox. v1

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

Thanks for doing this. As you might have noticed, the changes to this function were done piecemeal and without any overarching architecture. This is much cleaner.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3965,2 @@
>  
> +    // Whew!

Amen.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708048 [details] [diff] [review]
Part 8 - Push a cx in nsWindowSH::NewResolve. v1

Do we have a bug on getting rid of this? It makes no sense to switch contexts here and I suspect that this code was a wallpaper over some bug that has long been lost to the depths of time and has probably been fixed for real for a while now.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708050 [details] [diff] [review]
Part 10 - Push a cx in AdoptNode. v1

This isn't sufficient because we need to push the cx in GetContextAndScope. For this bug, we can do the easy thing and pass the cx pusher in, but please file a followup in trying to avoid do any context switching here.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 708052 [details] [diff] [review]
Part 12 - Push any cx we grab off the object for XPCWrappedJS. v1

This is unnecessary because the XPCCallContext pushes the context for you. That being said, if you want to land the comment you added, please do.

Comment 26

[Bobby Holley (:bholley)]

I'm just going to take a guess at sec-high here.

Comment 27

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #23)
> Do we have a bug on getting rid of this? It makes no sense to switch
> contexts here and I suspect that this code was a wallpaper over some bug
> that has long been lost to the depths of time and has probably been fixed
> for real for a while now.

It'll probably go away soonish, either as bz converts stuff to the new bindings or as I remove JSContext stuff.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #24)
> Comment on attachment 708050 [details] [diff] [review]
> Part 10 - Push a cx in AdoptNode. v1
> 
> This isn't sufficient because we need to push the cx in GetContextAndScope.
> For this bug, we can do the easy thing and pass the cx pusher in, but please
> file a followup in trying to avoid do any context switching here.

Filed bug 839467.

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 10 - Push a cx in AdoptNode. v2

Comment 30

[Bobby Holley (:bholley)]

Attachment: Part 10 - Push a cx in AdoptNode. v3

Comment 31

[Bobby Holley (:bholley)]

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

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 708053 [details] [diff] [review]
Part 13 - Assert proper cx stack handling in WrapperFactory::Rewrap. v1

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

It depends. It's clear that we don't do a good job of pushing JSContexts everywhere, and some of these patches highlight exactly where we don't do that. On the other hand, understanding how to turn that into an exploit is very nontrivial. Indeed, it's not even immediately clear to me how o do that.

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

Not really.

Which older supported branches are affected by this flaw?

All of them, pretty much. This is just an issue of us being sloppy.

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

They will probably be rather time-consuming, but pretty straightforward.

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

Testing would be good. Most of this patch is just adding an assertion about the cx stack and fixing all the places where we hit that assert with our test suite. However, there may be paths to hit the assertion that we don't test on tinderbox.

I recommend that we check this in to inbound immediately, let it bake for a while, and then look at backporting.

Comment 33

[Bobby Holley (:bholley)]

This is green on try, so just waiting on sec-approval here.

Comment 34

[Daniel Veditz [:dveditz]]

landing now means it will be bumped to Aurora in a week. Assuming the sec-approval request is for the whole shebang and not just adding an assert in part 13 does that give us enough testing for a patch of this size?

On the other hand if we are going to want this in Firefox 21 anyway then landing now saves you one branch to worry about.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #34)
> landing now means it will be bumped to Aurora in a week. Assuming the
> sec-approval request is for the whole shebang and not just adding an assert
> in part 13 does that give us enough testing for a patch of this size?

It's on the safe side for a patch of this size, modulo crashes due to hitting the assertion in ways our test suite didn't catch. Basically, each one of these patches is plugging a hole where we weren't pushing a JSContext but should have been. I'd like to check it in for this cycle.
 
> On the other hand if we are going to want this in Firefox 21 anyway then
> landing now saves you one branch to worry about.

Indeed.

Comment 36

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

We're too late in the cycle to land now. We're building this week to ship next week. I'd say that this has sec-approval for landing after we fork for the current release and branching.

Comment 37

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

Actually, forget that. Just land it now!

Comment 38

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=ed1ef766c293

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out for b2g build failures and mochitest-1 crashes on OSX.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69f329fc7ee

https://tbpl.mozilla.org/php/getParsedLog.php?id=19683369&tree=Mozilla-Inbound

/builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/system/gonk/SystemWorkerManager.cpp: In member function 'nsresult mozilla::dom::gonk::SystemWorkerManager::Init()':
/builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/system/gonk/SystemWorkerManager.cpp:345: error: 'AutoSafeJSContext' was not declared in this scope
/builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/system/gonk/SystemWorkerManager.cpp:345: error: expected ';' before 'cx'
/builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/system/gonk/SystemWorkerManager.cpp:347: error: 'cx' was not declared in this scope
make[7]: Leaving directory `/builds/slave/m-in-ics_a7_g-0000000000000000/build/obj-b2g/dom/system/gonk'

https://tbpl.mozilla.org/php/getParsedLog.php?id=19684857&tree=Mozilla-Inbound

225902 INFO TEST-START | /tests/content/xbl/test/test_bug821850.xhtml
++DOMWINDOW == 45 (0x10c28e138) [serial = 3508] [outer = 0x11c4f5008]
JavaScript error: , line 0: can't access dead object
JavaScript error: , line 0: can't access dead object
JavaScript error: , line 0: can't access dead object
JavaScript error: , line 0: can't access dead object
[Parent 432] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004003: file ../../../../intl/uconv/src/nsCharsetConverterManager.cpp, line 301
225903 INFO TEST-PASS | /tests/content/xbl/test/test_bug821850.xhtml | Set contentVal
[Parent 432] WARNING: NS_ENSURE_TRUE(globalObject) failed: file ../../../../content/base/src/nsDocument.cpp, line 6834
[Parent 432] WARNING: NS_ENSURE_TRUE(globalObject) failed: file ../../../../content/base/src/nsDocument.cpp, line 6834
Assertion failure: XPCJSRuntime::Get()->GetJSContextStack()->Peek() == cx, at ../../../../js/xpconnect/wrappers/WrapperFactory.cpp:346
TEST-UNEXPECTED-FAIL | /tests/content/xbl/test/test_bug821850.xhtml | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:40:02.951440
INFO | automation.py | Reading PID log: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpECKwbLpidlog
PROCESS-CRASH | /tests/content/xbl/test/test_bug821850.xhtml | application crashed [@ xpc::WrapperFactory::Rewrap(JSContext*, JSObject*, JSObject*, JSObject*, JSObject*, unsigned int)]
Crash dump filename: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpQ8nX5k/minidumps/07EC9850-7EED-466C-B5EE-DEADB65C8686.dmp
Operating system: Mac OS X
                  10.7.2 11C74
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!xpc::WrapperFactory::Rewrap(JSContext*, JSObject*, JSObject*, JSObject*, JSObject*, unsigned int) [WrapperFactory.cpp:e650f1bab42b : 338 + 0x0]
    rbx = 0x00007fff761fd630   r12 = 0x000000011c4f55b0
    r13 = 0x000000010f303080   r14 = 0x0000000000000001
    r15 = 0x000000011c4f55b0   rip = 0x000000010225b57b
    rsp = 0x00007fff5fbfb9e0   rbp = 0x00007fff5fbfba90
    Found by: given as instruction pointer in context
 1  XUL!MarkInternal<JSObject> [Root.h:e650f1bab42b : 1018 + 0x4]
    rip = 0x00000001034c1548   rsp = 0x00007fff5fbfba60
    rbp = 0x00007fff5fbfba90
    Found by: stack scanning
 2  XUL!JSCompartment::wrap(JSContext*, JS::MutableHandle<JS::Value>, JS::Handle<JSObject*>) [jscompartment.cpp:e650f1bab42b : 387 + 0x16]
    rip = 0x000000010327058f   rsp = 0x00007fff5fbfbaa0
    rbp = 0x00007fff5fbfbb70
    Found by: stack scanning
 3  libsystem_c.dylib + 0xa11a3
    rip = 0x00007fff90d7d1a4   rsp = 0x00007fff5fbfbae0
    rbp = 0x00007fff5fbfbb70
    Found by: stack scanning
 4  libmozalloc.dylib!moz_xmalloc [mozalloc.cpp:e650f1bab42b : 54 + 0x4]
    rip = 0x00000001000cc96e   rsp = 0x00007fff5fbfbb00
    rbp = 0x00007fff5fbfbb70
    Found by: stack scanning
 5  XUL!JSCompartment::wrap(JSContext*, JSObject**, JSObject*) [jscompartment.cpp:e650f1bab42b : 426 + 0x4]
    rip = 0x0000000103270dbc   rsp = 0x00007fff5fbfbb80
    rbp = 0x00007fff5fbfbbb0
    Found by: stack scanning
 6  XUL!JS_WrapObject(JSContext*, JSObject**) [jsapi.cpp:e650f1bab42b : 1507 + 0x10]
    rip = 0x000000010322c87c   rsp = 0x00007fff5fbfbbc0

Comment 40

[Bobby Holley (:bholley)]

Sigh, turns out there's actually a lot more work do be done. Spent the day on this and pushing to try:

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

Comment 41

[Bobby Holley (:bholley)]

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

Comment 42

[Bobby Holley (:bholley)]

Note - I had to disable an assertion in bug 834697 because of one of these. I'll re-enable it in this patch stack.

Comment 43

[Bobby Holley (:bholley)]

Attachment: Part 12.1 - Introduce AutoPushJSContext. v1

It's annoying to add yet another RAII class, but we're solving a different
problem here. In particular, there are lots of callers that grab a cx off
of an nsIScriptContext and use it without pushing. Most of the time this
is ok, since that cx is also the active cx. But it's hard to tell when we
might potentially violate that invariant. What's more, we don't want to just
use an nsCxPusher, because that does expensive things even when the cx matches
the one on the top of the stack. Most of these consumers should just switch
to AutoJSContext, but doing such a change en masse is a pretty risky thing to
do. So let's introduce a class that gives us good performance in the common case
and correctness in the uncommon case.

Comment 44

[Bobby Holley (:bholley)]

Attachment: Part 12.2 - Audit callers of GetNativeContext and use AutoPushJSContext where appropriate. v1

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 713904 [details] [diff] [review]
Part 12.1 - Introduce AutoPushJSContext. v1

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

::: content/base/public/nsContentUtils.h
@@ +2315,5 @@
> + */
> +class NS_STACK_CLASS AutoPushJSContext {
> +  nsCxPusher mPusher;
> +  JSContext* mCx;
> +  public:

Nit: newline before public. Also, should public be in column 0 to distinguish it from the declarations above?

@@ +2317,5 @@
> +  nsCxPusher mPusher;
> +  JSContext* mCx;
> +  public:
> +    AutoPushJSContext(JSContext* aCx) : mCx(aCx) {
> +      if (mCx && mCx != nsContentUtils::GetCurrentJSContext()) {

IMO this should either assert mCx is non-null or allow pushing a null context. Silently doing nothing is scary.

Comment 46

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 713905 [details] [diff] [review]
Part 12.2 - Audit callers of GetNativeContext and use AutoPushJSContext where appropriate. v1

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

I read through this and didn't see anything glaringly obvious. I think I've also managed to convince myself that this can't change behavior in cases that weren't already broken.

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 713904 [details] [diff] [review]
Part 12.1 - Introduce AutoPushJSContext. v1

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

bholley pointed out that the uses of this class weren't consistent with my request, so r+. I asked him on IRC to add a comment about null contexts so that future users of the class will at least think about them.

Comment 48

[Bobby Holley (:bholley)]

Gave this a full try push to be extra safe, given how big of a change it is:
https://tbpl.mozilla.org/?tree=Try&rev=bc9876e864ad

Comment 49

[Bobby Holley (:bholley)]

All good except for some b2g build bustage I think. Fixed and repushing for b2g builds:

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

Comment 50

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=a38bbae7a53b

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7736f5d80843
https://hg.mozilla.org/mozilla-central/rev/ebd40f774d14
https://hg.mozilla.org/mozilla-central/rev/34e3e1156a7f
https://hg.mozilla.org/mozilla-central/rev/90724209f555
https://hg.mozilla.org/mozilla-central/rev/471fe31fc325
https://hg.mozilla.org/mozilla-central/rev/d6766dee457e
https://hg.mozilla.org/mozilla-central/rev/19593a7c78f1
https://hg.mozilla.org/mozilla-central/rev/45e051989f29
https://hg.mozilla.org/mozilla-central/rev/7b5cc572e391
https://hg.mozilla.org/mozilla-central/rev/e065138ccd48
https://hg.mozilla.org/mozilla-central/rev/79519aeac030
https://hg.mozilla.org/mozilla-central/rev/0e9081767ffd
https://hg.mozilla.org/mozilla-central/rev/a099a2fcdc4e
https://hg.mozilla.org/mozilla-central/rev/a38bbae7a53b

Comment 52

[Daniel Veditz [:dveditz]]

Without any known way to trigger the potential vulnerabilities it's safer to not try to back-port this anywhere.

Comment 53

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #52)
> Without any known way to trigger the potential vulnerabilities it's safer to
> not try to back-port this anywhere.

Ok. moz_bug_r_a4, if you want to look at this, I'd suggest trying to find a way to create a mismatch between the stack-top cx and the cx being used by running code. Then, see if you can get the code to query the subject principal (using the cx stack), and exploit the resulting principal. ;-)