Last Comment Bug 657267 - (CVE-2011-2993) possible for unsignend script to call functions inside signed scripts (jar)
(CVE-2011-2993)
: possible for unsignend script to call functions inside signed scripts (jar)
Status: RESOLVED FIXED
[sg:critical] against sites who used ...
: regression
Product: Firefox
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: x86_64 Mac OS X
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
http://ebenda.org/2011/signed.call/
Depends on:
Blocks: 658625 646840
  Show dependency treegraph
 
Reported: 2011-05-15 19:16 PDT by Rafael Gieschke
Modified: 2014-06-26 06:50 PDT (History)
13 users (show)
rforbes: sec‑bounty-
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
?
wanted
unaffected
unaffected


Attachments
Proposed fix v1 (13.81 KB, patch)
2011-05-18 09:55 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review-
Details | Diff | Splinter Review
Proposed fix v2 (13.98 KB, patch)
2011-05-19 07:30 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
Details | Diff | Splinter Review

Description Rafael Gieschke 2011-05-15 19:16:21 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2011-05-16 14:11:54 PDT
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.
Comment 3 Rafael Gieschke 2011-05-16 14:23:46 PDT
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).
Comment 4 Daniel Veditz [:dveditz] 2011-05-16 17:32:35 PDT
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
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-05-17 09:49:13 PDT
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?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-05-17 09:51:06 PDT
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...
Comment 7 Blake Kaplan (:mrbkap) 2011-05-18 09:55:15 PDT
Created attachment 533310 [details] [diff] [review]
Proposed fix v1
Comment 8 Daniel Veditz [:dveditz] 2011-05-18 10:17:35 PDT
Why didn't the tests for the old bug catch this regression?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 10:26:28 PDT
> 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 10 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 10:51:30 PDT
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.
Comment 11 Blake Kaplan (:mrbkap) 2011-05-19 03:04:50 PDT
(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.
Comment 12 Blake Kaplan (:mrbkap) 2011-05-19 07:30:16 PDT
Created attachment 533639 [details] [diff] [review]
Proposed fix v2

This patch is green on Try and addresses the other comments.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 07:56:55 PDT
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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-05-19 07:57:45 PDT
Oh, and we should really add a test...
Comment 15 Blake Kaplan (:mrbkap) 2011-05-20 07:49:07 PDT
I landed http://hg.mozilla.org/tracemonkey/rev/80771239f430 with a test.
Comment 16 Blake Kaplan (:mrbkap) 2011-05-20 09:20:17 PDT
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).
Comment 17 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:18:01 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/80771239f430
Comment 18 Phil Ringnalda (:philor) 2011-05-29 12:59:17 PDT
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 Asa Dotzler [:asa] 2011-05-31 18:33:01 PDT
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?
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-02 13:31:51 PDT
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...
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2011-06-03 15:18:55 PDT
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.
Comment 22 Raymond Forbes[:rforbes] 2013-07-19 17:36:04 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

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