The default bug view has changed. See this FAQ.
Bug 758344 (CVE-2012-1967)

Arbitrary code execution using javascript: url

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bholley)

Tracking

({sec-critical, testcase})

unspecified
sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ verified, firefox15+ verified, firefox16+ verified, firefox-esr1014+ verified, status1.9.2 wontfix)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox14: --- → +
tracking-firefox15: --- → +
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.
(Reporter)

Comment 5

5 years ago
(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?
(Reporter)

Comment 8

5 years ago
(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.
(Reporter)

Comment 9

5 years ago
Sorry, I only tested on trunk.  On fx10-14, I can use Function.prototype.bind as a replacement.
status1.9.2: unaffected → wontfix
status-firefox13: --- → wontfix
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?
(Reporter)

Comment 13

5 years ago
(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
status-firefox16: --- → affected
tracking-firefox16: --- → +
(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).
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.
Attachment #635668 - Flags: review?(mrbkap)
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.
Attachment #635669 - Flags: review?(mrbkap)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2541e0773ed3
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.
Attachment #635669 - Attachment is obsolete: true
Attachment #635669 - Flags: review?(mrbkap)
Attachment #635679 - Flags: review?(mrbkap)
Repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fb5e8549c38b
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.

Updated

5 years ago
Attachment #635668 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
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.
status-firefox15: affected → fixed
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]
https://hg.mozilla.org/mozilla-central/rev/17a545fb6702
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.
tracking-firefox-esr10: ? → 14+
(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
Last Resolved: 5 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+
Pushed to m-b:
http://hg.mozilla.org/releases/mozilla-beta/rev/181fd83fcdfb
http://hg.mozilla.org/releases/mozilla-beta/rev/dc6100777c36
status-firefox14: affected → fixed
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+
Pushed to mozilla-esr10:
http://hg.mozilla.org/releases/mozilla-esr10/rev/b1af442bf357
http://hg.mozilla.org/releases/mozilla-esr10/rev/a92d2aa57a61
status-firefox-esr10: affected → fixed
Marking fixed-16 because of bug 754202.
status-firefox16: affected → fixed
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.
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox14: fixed → verified
status-firefox15: fixed → verified
status-firefox16: fixed → verified
Alias: CVE-2012-1967
Group: core-security
You need to log in before you can comment on or make changes to this bug.