758344 - (CVE-2012-1967) Arbitrary code execution using javascript: url

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

When javascript: url is evaluated using a sandbox, it's possible to run arbitrary code with the chrome privileges.

In xpc_EvalInSandbox, when calling JS_ValueToString(sandcx->GetJSContext(), v), nsScriptSecurityManager::GetPrincipalAndFrame returns null (which is treated as the system principal) since there is no frame nor clamped principal and GetScriptContextPrincipalFromJSContext(cx) returns null.

Comment 2

[Bobby Holley (:bholley)]

Hm, hopefully my patches in bug 754202 will fix this.

Comment 3

[Daniel Veditz [:dveditz]]

Whatever happened to removing lookupMethod (bug 693733)? Or even Components entirely, but that might break a lot more.

3.6.x is safe, but we need this fixed on all currently supported versions.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Whatever happened to removing lookupMethod (bug 693733)? Or even Components
> entirely, but that might break a lot more.

This involves a crazy xbl trick that I haven't had time to work up. And it's almost certainly not backportable.

Comment 5

[moz_bug_r_a4]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> 3.6.x is safe, but we need this fixed on all currently supported versions.

3.6.x is also exploitable.  I'll attach a testcase for 3.6.x.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

We should probably make ContextHolder in XPCComponents.cpp implement nsIScriptContextPrincipal *and* clamp the principal in xpc_EvalInSandbox, for belt-and-braces.

dveditz, I doubt very much that removing lookupMethod fixes this bug for good... moz_bug_r_a4, can you use Function.prototype.bind as a replacement?

Comment 8

[moz_bug_r_a4]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> moz_bug_r_a4, can you use Function.prototype.bind as a replacement?

No. this bug's testcases rely on lookupMethod to access privileged objects, which are needed to run arbitrary code since there is no useful objects in Sandbox's scope.

Comment 9

[moz_bug_r_a4]

Sorry, I only tested on trunk.  On fx10-14, I can use Function.prototype.bind as a replacement.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Bobby Holley (:bholley) from comment #2)
> Hm, hopefully my patches in bug 754202 will fix this.

Nice work Bobby (I saw your fix over there landed). Can you confirm if it fixes this bug?

Comment 12

[Bobby Holley (:bholley)]

moz_bug_r_a4, can you check if this is fixed on trunk?

Comment 13

[moz_bug_r_a4]

(In reply to Bobby Holley (:bholley) from comment #12)
> moz_bug_r_a4, can you check if this is fixed on trunk?

The testcase can still get privilege on trunk due to bug 762920.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> We should probably make ContextHolder in XPCComponents.cpp implement
> nsIScriptContextPrincipal

This alone isn't good enough, because the UniversalXPConnect check in CheckObjectAccessImpl will see no code on the stack and just return true. It doesn't ever check for nsIScriptContextPrincipal the way that GetPrincipalAndFrameDoes, which IMO makes nsIScriptContextPrincipal pretty useless. But hey, can't hurt.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #14)
> (In reply to Blake Kaplan (:mrbkap) from comment #7)
> which IMO makes nsIScriptContextPrincipal pretty
> useless. But hey, can't hurt.

(Actually, I think it definitely can hurt to have ineffective extra security measures. But I don't plan to land this patch on trunk, since bug 754202 will fix this up properly there).

Comment 16

[Bobby Holley (:bholley)]

Attachment: Make ContextHolder implement nsIScriptContextPrincipal. v1

Given comment 14 and comment 15, I'll let blake decide if we want this or not.

Note that both of these patches will only go on branches.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Push more stuff when evaluating in a sandbox. v1

This fixes the testcase. I took some liberty with the naming given that this is only going on branch.

Comment 18

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2541e0773ed3

Comment 19

[Bobby Holley (:bholley)]

Attachment: Push more stuff when evaluating in a sandbox. v2

Whoops, I forgot to set mPushed in the last one. Sorry.

Comment 20

[Bobby Holley (:bholley)]

Repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fb5e8549c38b

Comment 21

[Bobby Holley (:bholley)]

This one looks green.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 635679 [details] [diff] [review]
Push more stuff when evaluating in a sandbox. v2

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3770,5 @@
>                               jsVersion, false, retval);
>  }
>  
> +struct NS_STACK_CLASS AutoSecurityJunkPusher {
> +

Nit: brace on the next line.

@@ +3782,5 @@
> +     : mCx(cx)
> +     , mPrincipal(principal)
> +     , mStack(XPCPerThreadData::GetData(cx)->GetJSContextStack())
> +     , mSSM(XPCWrapper::GetSecurityManager())
> +     , mPushed(false) { }

Another nit: please put the braces on the following line (also, what's with the crazy indentation?).

@@ +3785,5 @@
> +     , mSSM(XPCWrapper::GetSecurityManager())
> +     , mPushed(false) { }
> +
> +    bool Push() {
> +

Nit: Extra blank line.

@@ +3787,5 @@
> +
> +    bool Push() {
> +
> +        // First, push the js context.
> +        if (!mStack->Push(mCx)) {

Should this null-check mStack?

@@ +3794,5 @@
> +        }
> +
> +        // Then, push the principal.
> +        nsresult rv = mSSM->PushContextPrincipal(mCx, nsnull, mPrincipal);
> +        if (NS_FAILED(rv))

If this fails, we need to pop mCx off of the stack, right?

@@ +3811,5 @@
> +    ~AutoSecurityJunkPusher() {
> +        if (mPushed)
> +            Pop();
> +    }
> +

Nit: extra line.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

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

I think we should do this, even if it isn't effective. IMO it's worth having as many globals as we can be consistent in what they implement.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3644,5 @@
>  
> +    nsIScriptObjectPrincipal * GetObjectPrincipal() { return this; }
> +    nsIPrincipal * GetPrincipal() { return mPrincipal; }
> +
> +

Nit: extra line here.

Comment 24

[Bobby Holley (:bholley)]

Dan, Lukas - how do we want to proceed here? I'm just about to land bug 754202, which obviates the need (and the machinery) to do this on m-c. Should I land this directly to m-a? What's our timeframe like for getting this onto beta?

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

Requesting m-a approval, see comment 24.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

Given that beta is go-to-build on tuesday, pre-emptively flagging for approval there as well.

Comment 27

[Alex Keybl [:akeybl]]

(In reply to Bobby Holley (:bholley) from comment #26)
> Comment on attachment 635668 [details] [diff] [review]
> Make ContextHolder implement nsIScriptContextPrincipal. v1
> 
> Given that beta is go-to-build on tuesday, pre-emptively flagging for
> approval there as well.

Sadly we had to spin an out-of-band beta over the weekend, so the next beta is delayed. Let's start on Aurora.

Comment 28

[Bobby Holley (:bholley)]

Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/139156d111b6
http://hg.mozilla.org/releases/mozilla-aurora/rev/c529dbb61c9d

Leaving this flagged for m-b approval, since we'll want it there as well.

Comment 29

[Bobby Holley (:bholley)]

I landed the nsIScriptContextPrincipal patch to m-i to see if it fixes bug 768355:

http://hg.mozilla.org/integration/mozilla-inbound/rev/17a545fb6702

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/17a545fb6702

Comment 31

[Bobby Holley (:bholley)]

Now that bug 754202 landed, this should be fixed on m-c. And, dare I say it, every other exploit using this trick? Let's hope it sticks. :-)

Still needs to be fixed on beta though.

Comment 32

[Daniel Veditz [:dveditz]]

> Still needs to be fixed on beta though.

And ESR-10, if possible.

Given the size/number of patches in bug 754202 and the fact that it's been through a back-out cycle it may be a bad idea pushing this into the last Beta for Fx14. If it sticks we need to get it into Aurora before that uplifts, though.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #32)

> Given the size/number of patches in bug 754202 and the fact that it's been
> through a back-out cycle it may be a bad idea pushing this into the last
> Beta for Fx14. If it sticks we need to get it into Aurora before that
> uplifts, though.

Err, this bug has patches specifically for uplift (already landed on aurora, waiting for beta approval). The ones in bug 754202 were never intended to land anywhere but m-c.

Comment 34

[Bobby Holley (:bholley)]

Marking as fixed for m-c.

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

[Triage comment]
This has been on aurora for several days now without any mention of fallout, so approving for beta. Please also nominate (or put up new patches) for ESR10 landing.

Comment 36

[Bobby Holley (:bholley)]

Pushed to m-b:
http://hg.mozilla.org/releases/mozilla-beta/rev/181fd83fcdfb
http://hg.mozilla.org/releases/mozilla-beta/rev/dc6100777c36

Comment 37

[Bobby Holley (:bholley)]

These patches apply to mozilla-esr10 with minor rebasing. Flagging for approval.

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 635679 [details] [diff] [review]
Push more stuff when evaluating in a sandbox. v2

Thanks for rebasing for ESR - please go ahead and land.

Comment 39

[Bobby Holley (:bholley)]

Pushed to mozilla-esr10:
http://hg.mozilla.org/releases/mozilla-esr10/rev/b1af442bf357
http://hg.mozilla.org/releases/mozilla-esr10/rev/a92d2aa57a61

Comment 40

[Bobby Holley (:bholley)]

Marking fixed-16 because of bug 754202.

Comment 41

[u279076]

Testcases reproduce this bug in Firefox 13.0.1.

Unreproducible with the following builds:
 * Firefox 14.0b12
 * Firefox 15.0a2 2012-07-11
 * Firefox 16.0a1 2012-07-11
 * Firefox 10.0.6esrpre 2012-07-11

Marking verified fixed.