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 22.214.171.124
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 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.
(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: http://hg.mozilla.org/mozilla-central/rev/80771239f430
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.