Last Comment Bug 758344 - (CVE-2012-1967) Arbitrary code execution using javascript: url
(CVE-2012-1967)
: Arbitrary code execution using javascript: url
Status: VERIFIED FIXED
[advisory-tracking+]
: sec-critical, testcase
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on: 754202 762920
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 12:58 PDT by moz_bug_r_a4
Modified: 2012-07-20 18:02 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
14+
verified
wontfix


Attachments
Make ContextHolder implement nsIScriptContextPrincipal. v1 (3.85 KB, patch)
2012-06-22 02:07 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review
Push more stuff when evaluating in a sandbox. v1 (3.81 KB, patch)
2012-06-22 02:08 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Push more stuff when evaluating in a sandbox. v2 (3.87 KB, patch)
2012-06-22 03:19 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description moz_bug_r_a4 2012-05-24 12:58:16 PDT
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 (PTO through June 13) 2012-05-24 13:20:57 PDT
Hm, hopefully my patches in bug 754202 will fix this.
Comment 3 Daniel Veditz [:dveditz] 2012-05-25 11:27:16 PDT
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 (PTO through June 13) 2012-05-25 16:34:25 PDT
(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 2012-05-25 18:43:44 PDT
(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) (please use needinfo!) 2012-05-25 18:51:01 PDT
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 2012-05-25 19:46:41 PDT
(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 2012-05-25 20:31:49 PDT
Sorry, I only tested on trunk.  On fx10-14, I can use Function.prototype.bind as a replacement.
Comment 11 David Bolter [:davidb] 2012-06-07 13:54:30 PDT
(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 (PTO through June 13) 2012-06-07 15:13:59 PDT
moz_bug_r_a4, can you check if this is fixed on trunk?
Comment 13 moz_bug_r_a4 2012-06-08 08:04:01 PDT
(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 (PTO through June 13) 2012-06-22 01:20:18 PDT
(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 (PTO through June 13) 2012-06-22 01:21:50 PDT
(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 (PTO through June 13) 2012-06-22 02:07:37 PDT
Created attachment 635668 [details] [diff] [review]
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 (PTO through June 13) 2012-06-22 02:08:11 PDT
Created attachment 635669 [details] [diff] [review]
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 (PTO through June 13) 2012-06-22 02:14:31 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2541e0773ed3
Comment 19 Bobby Holley (PTO through June 13) 2012-06-22 03:19:42 PDT
Created attachment 635679 [details] [diff] [review]
Push more stuff when evaluating in a sandbox. v2

Whoops, I forgot to set mPushed in the last one. Sorry.
Comment 20 Bobby Holley (PTO through June 13) 2012-06-22 03:22:17 PDT
Repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fb5e8549c38b
Comment 21 Bobby Holley (PTO through June 13) 2012-06-22 05:26:49 PDT
This one looks green.
Comment 22 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-06-22 14:15:46 PDT
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) (please use needinfo!) 2012-06-22 15:02:27 PDT
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 (PTO through June 13) 2012-06-24 07:51:24 PDT
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 (PTO through June 13) 2012-06-24 07:52:19 PDT
Comment on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

Requesting m-a approval, see comment 24.
Comment 26 Bobby Holley (PTO through June 13) 2012-06-25 01:45:49 PDT
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] 2012-06-26 09:49:25 PDT
(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 (PTO through June 13) 2012-06-26 13:24:45 PDT
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 (PTO through June 13) 2012-06-26 13:56:07 PDT
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] 2012-06-27 03:39:45 PDT
https://hg.mozilla.org/mozilla-central/rev/17a545fb6702
Comment 31 Bobby Holley (PTO through June 13) 2012-06-28 15:02:59 PDT
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] 2012-06-28 16:31:46 PDT
> 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 (PTO through June 13) 2012-06-29 00:57:19 PDT
(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 (PTO through June 13) 2012-06-29 02:11:12 PDT
Marking as fixed for m-c.
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-02 13:36:29 PDT
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 37 Bobby Holley (PTO through June 13) 2012-07-03 04:03:27 PDT
These patches apply to mozilla-esr10 with minor rebasing. Flagging for approval.
Comment 38 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-06 00:51:33 PDT
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 40 Bobby Holley (PTO through June 13) 2012-07-06 01:11:35 PDT
Marking fixed-16 because of bug 754202.
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 16:10:44 PDT
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.

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