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.
Steps to Reproduce:
1. go to http://ebenda.org/2011/signed.call/
2. click link
Function a() inside signed jar is executed from unsinged page.
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.
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
but you can
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.
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 220.127.116.11
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...
Created attachment 533310 [details] [diff] [review]
Proposed fix v1
Why didn't the tests for the old bug catch this regression?
> 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
>+ // 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 =
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.
(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.
Created attachment 533639 [details] [diff] [review]
Proposed fix v2
This patch is green on Try and addresses the other comments.
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.
Oh, and we should really add a test...
I landed http://hg.mozilla.org/tracemonkey/rev/80771239f430 with a test.
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).
cdleary-bot mozilla-central merge info:
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
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?
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.