Closed Bug 758344 (CVE-2012-1967) Opened 8 years ago Closed 8 years ago

Arbitrary code execution using javascript: url

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
firefox13 --- wontfix
firefox14 + verified
firefox15 + verified
firefox16 + verified
firefox-esr10 14+ verified
status1.9.2 --- wontfix

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

References

Details

(Keywords: sec-critical, testcase, Whiteboard: [advisory-tracking+])

Attachments

(2 files, 1 obsolete file)

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.
Keywords: sec-critical
OS: Windows XP → All
Hardware: x86 → All
Hm, hopefully my patches in bug 754202 will fix this.
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.
Assignee: nobody → bobbyholley+bmo
Depends on: 693733, 754202
(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.
(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.
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?
(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.
Sorry, I only tested on trunk.  On fx10-14, I can use Function.prototype.bind as a replacement.
No longer depends on: 693733
Keywords: testcase
(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?
moz_bug_r_a4, can you check if this is fixed on trunk?
(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.
Depends on: 762920
(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.
(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).
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.
Attachment #635668 - Flags: review?(mrbkap)
This fixes the testcase. I took some liberty with the naming given that this is only going on branch.
Attachment #635669 - Flags: review?(mrbkap)
Whoops, I forgot to set mPushed in the last one. Sorry.
Attachment #635669 - Attachment is obsolete: true
Attachment #635669 - Flags: review?(mrbkap)
Attachment #635679 - Flags: review?(mrbkap)
This one looks green.
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.
Attachment #635679 - Flags: review?(mrbkap) → review+
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.
Attachment #635668 - Flags: review?(mrbkap) → review+
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 on attachment 635668 [details] [diff] [review]
Make ContextHolder implement nsIScriptContextPrincipal. v1

Requesting m-a approval, see comment 24.
Attachment #635668 - Flags: approval-mozilla-aurora?
Attachment #635679 - Flags: approval-mozilla-aurora?
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.
Attachment #635668 - Flags: approval-mozilla-beta?
Attachment #635679 - Flags: approval-mozilla-beta?
(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.
Attachment #635668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #635679 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
I landed the nsIScriptContextPrincipal patch to m-i to see if it fixes bug 768355:

http://hg.mozilla.org/integration/mozilla-inbound/rev/17a545fb6702
Whiteboard: [Leave open after merge]
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.
> 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.
(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.
Marking as fixed for m-c.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #635668 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #635679 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
These patches apply to mozilla-esr10 with minor rebasing. Flagging for approval.
Attachment #635668 - Flags: approval-mozilla-esr10?
Attachment #635679 - Flags: approval-mozilla-esr10?
Attachment #635668 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
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.
Attachment #635679 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Marking fixed-16 because of bug 754202.
Whiteboard: [Leave open after merge] → [Leave open after merge][advisory-tracking+]
Whiteboard: [Leave open after merge][advisory-tracking+] → [advisory-tracking+]
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.
Alias: CVE-2012-1967
Group: core-security
You need to log in before you can comment on or make changes to this bug.