Bug 657267 (CVE-2011-2993)

possible for unsignend script to call functions inside signed scripts (jar)

RESOLVED FIXED

Status

()

Firefox
Security
--
critical
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Rafael Gieschke, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {regression})

unspecified
x86_64
Mac OS X
regression
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite +

Firefox Tracking Flags

(firefox5- wontfix, firefox6+ fixed, blocking2.0 ?, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical] against sites who used signed JS [fixed-in-tracemonkey], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

It is possible for unsigned scripts to call functions inside signed scripts on the same host and thus possibly execute code using expanded privileges. Possibly a regression from Firefox 3.6.

Reproducible: Always

Steps to Reproduce:
1. go to http://ebenda.org/2011/signed.call/
2. click link


Actual Results:  
Function a() inside signed jar is executed from unsinged page.

Expected Results:  
Exception is raised.
Like Firefox 3.6.17 does: "Fehler: <http://ebenda.org> wurde die Erlaubnis für das Lesen der Eigenschaft Window.a verweigert.
Quelldatei: javascript:alert(window.frames[0].a('netscape.security.PrivilegeManager.enablePrivilege(\'UniversalXPConnect\');Components.classes[\'@mozilla.org/xre/app-info;1\'].getService(Components.interfaces.nsIXULRuntime).OS'));
Zeile: 1"

If privileges were granted to signer of the signed jar permanently before executing the attack, no confirmation dialog will occur.
Confirming in 4.0.1, this is a regression from Firefox 3.6 and before (caused by Compartments?). For some reason the permission dialog has an empty string for the site name. Haven't checked to see if this is an artifact of this testcase or if that got broken in 4.0.1 in general. The dialog says

   A script from "" is requesting enhanced abilities that are UNSAFE
   and could be used to compromise your machine or data:

Same thing on Aurora (Fx5)

Sort of confirming on 6.0a1 -- the site and jarred site are same-origin, but the enablePrivilege testcase doesn't work. Trying to inject an alert() into the jar: frame also fails with the same error, but you can still get at least simple code run in the frame.E.g. in the testcase you can't do

   javascript:window.frames[0].a('alert(1)');void(0);

but you can

   javascript:alert(window.frames[0].a('3+4'));void(0);

and get the answer '7' as computed inside the jar: frame.

Since we haven't fixed bug 546848 yet even if we can't inject an enablePrivilege call we could still inject code into a context that is doing privileged stuff.

Calling this sg:critical because it's not too far-fetched that you could convince users to click on the dialog based on a site that the user already expects to do so, but in practical terms there may be few such sites to abuse.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: --- → +
tracking-firefox6: --- → ?
Ever confirmed: true
Keywords: regression
Whiteboard: [sg:critical] against sites who used signed JS
(Reporter)

Comment 3

6 years ago
The site string in the permission dialog is empty because I don't have an organizationName in my certificate as StartSSL™ Verified (Class 2) certificates do not inlcude organizationName. If organizationName is missing, Firefox uses the empty string (same in version 3.6 and every version before, I think).
This is a regression of MFSA 2008-23 (http://www.mozilla.org/security/announce/2008/mfsa2008-23.html), originally fixed in Firefox 2.0.0.15
This is fundamentally the same thing as the document.domain thing.  The two pages are in the same compartment, so no security check is done at the boundary...

That said, why are they in the same compartment, exactly?  They have different (unequal) principals all along... Blake?
Ah, because the compartment maps keys off the principal's URI only, not off the principal itself.  I believe this was even discussed at the time, and dismissed for some reasons that seemed spurious...
(Assignee)

Comment 7

6 years ago
Created attachment 533310 [details] [diff] [review]
Proposed fix v1
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #533310 - Flags: review?(bzbarsky)
Why didn't the tests for the old bug catch this regression?
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
> Why didn't the tests for the old bug catch this regression?

Because there weren't any.  At the time we had no infrastructure for testing it, and when the infrastructure was created no one went back and created tests based on it.  See all the dependencies of bug 424488 that are "in-testsuite?".
Comment on attachment 533310 [details] [diff] [review]
Proposed fix v1

This bit:

>+    // If either principal has no URI, it's the saved principal from
>+    // preferences; in that case, test true.

should only happen in the mCert != null case to keep the old code flow.  It also needs to return true, which your changed version does not (which is why happening only when mCert is important: otherwise you'd return true when aOther is the system principal).  I really wish we actually had tests for the capability preference stuff; they would have caught this....

That all applies to Equals.  In the EqualsWithoutDomain() case, I think we can just skip this step because we shouldn't be calling EqualsWithoutDomain() on principals from prefs.

+  *aResult =
+    NS_SUCCEEDED(nsScriptSecurityManager::SecurityCompareURIs(mCodebase,
+                                                              otherURI));

SecurityCompareURIs returns a PRBool.  So this should be:

  *aResult = nsScriptSecurityManager::SecurityCompareURIs(mCodebase, otherURI);

Was this patch tested against this bug's steps to reproduce?

The code in xpc::PtrAndPrincipalHashKey::KeyEquals mixes 2 and 4 space indent.  Pick one, please.
Attachment #533310 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 11

6 years ago
(In reply to comment #10)
> Was this patch tested against this bug's steps to reproduce?

Yeah. But you should have seen the try server run!

> The code in xpc::PtrAndPrincipalHashKey::KeyEquals mixes 2 and 4 space
> indent.  Pick one, please.

Sorry about the dud patch.
(Assignee)

Comment 12

6 years ago
Created attachment 533639 [details] [diff] [review]
Proposed fix v2

This patch is green on Try and addresses the other comments.
Attachment #533310 - Attachment is obsolete: true
Attachment #533639 - Flags: review?(bzbarsky)
Comment on attachment 533639 [details] [diff] [review]
Proposed fix v2

>+  NS_ASSERTION(otherURI && mCodebase,
>+               "shouldn't be calling this on principals from preferences");

That's bogus; otherURI can be null if aOther is a system principal.  We want to return false in that case, which is what SecurityCompareURIs will do, so that's fine. But we can't assert anything about otherURI!

We can probably assert mCodebase, yes.

Should the new method be called equalsIgnoringDomain?

r=me with the nit above fixed.
Attachment #533639 - Flags: review?(bzbarsky) → review+
Oh, and we should really add a test...
(Assignee)

Comment 15

6 years ago
I landed http://hg.mozilla.org/tracemonkey/rev/80771239f430 with a test.
Whiteboard: [sg:critical] against sites who used signed JS → [sg:critical] against sites who used signed JS [fixed-in-tracemonkey]
Flags: in-testsuite+
(Assignee)

Comment 16

6 years ago
Note that I also landed http://hg.mozilla.org/tracemonkey/rev/d844c5cfbb43 to fix a bug in the test (and to make the output slightly more useful if it fails).
(Assignee)

Updated

6 years ago
Blocks: 658625
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/80771239f430
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Cherrypicked disabling the test on Android too in http://hg.mozilla.org/mozilla-central/rev/290993adeb2e, discovered that bug 660497 meant it wasn't actually disabled on Android, worked around that in http://hg.mozilla.org/mozilla-central/rev/8d752162e810, and pushed them both to aurora in http://hg.mozilla.org/releases/mozilla-aurora/rev/e8ab15d84da6 and http://hg.mozilla.org/releases/mozilla-aurora/rev/d8b797975700

Comment 19

6 years ago
This appears to have made it onto the Firefox 6 train so marking it tracking and fixed there. What about Firefox 5? Is this something we can take there with confidence?
status-firefox6: --- → fixed
tracking-firefox6: ? → +
Blake, can you attach a rolled up patch of the followups that were needed here? We'd considering this for beta, but need to weigh the risk vs reward etc, and doing that w/o a combined patch is tricky...
Per discussion with mrbkap and dveditz we've decided that it's too late to fix this for 5, but it's already fixed for 6.
status-firefox5: affected → wontfix
tracking-firefox5: + → -
Blocks: 646840
Alias: CVE-2011-2993
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty-
You need to log in before you can comment on or make changes to this bug.