657267 - (CVE-2011-2993) possible for unsignend script to call functions inside signed scripts (jar)

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Rafael Gieschke]

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]]

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]

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]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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) (inactive)]

Attachment: Proposed fix v1

Comment 8

[Daniel Veditz [:dveditz]]

Why didn't the tests for the old bug catch this regression?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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 [:bzbarsky]]

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) (inactive)]

(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) (inactive)]

Attachment: Proposed fix v2

This patch is green on Try and addresses the other comments.

Comment 13

[Boris Zbarsky [: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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, and we should really add a test...

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

I landed http://hg.mozilla.org/tracemonkey/rev/80771239f430 with a test.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/80771239f430

Comment 18

[Phil Ringnalda (:philor)]

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]]

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)]

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)]

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]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719