616913 - "ASSERTION: Principal mismatch. Not good"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (reloads itself indefinitely)

###!!! ASSERTION: Principal mismatch.  Not good: 'strcmp(jsClass->name, "Location") == 0 ? NS_SUCCEEDED(CheckSameOriginPrincipal(result, principal)) : result == principal', file caps/src/nsScriptSecurityManager.cpp, line 2468

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Veditz [:dveditz]]

Let's see if moz_bug_r_a4 can do anything interesting with this.

Comment 3

[Daniel Veditz [:dveditz]]

Might be one of the new wrappers, which could be legit and simply means the assertion is out of date.

Comment 4

[moz_bug_r_a4]

Attachment: testcase 2 (reloads itself once)

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-11-01+04&enddate=2010-11-02+04

Comment 5

[Johnny Stenback (:jst)]

Boris, can you look into this? I'm not sure if this is a bogus assertion now
due to compartments or new wrappers, of if this is a real problem. Either way,
we need to investigate. Please suggest a sg rating once you've had a look.

Comment 6

[Johnny Stenback (:jst)]

Boris, I'm going to block on understanding this better. If this turns out to not actually be critial, then we can unblock on it as well.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  When we hit this assert, the object whose principal we're asserting about is the History object, of course.  The two principals are not pointer-identical, but both principals for the page in question; no surprise there; presumably one is the pre-reload principal and one is the post-reload one.

What confuses me about the second testcase is that navigator.H is non-null at all.  Why is that?  That shouldn't happen...

Also, there are at least _three_ maybe-different principals here.  All three of aObj->compartment()->principals->nsIPrincipalPtr, result, and principal are different pointers, but happen to have the same codebase in this case.  I guess that's not too surprising for the compartment case, since the compartment's principal only guarantees origin, not principal object identity or anything else.

Comment 8

[Boris Zbarsky [:bzbarsky]]

In any case, the history object is parented to the _outer_ window, not the inner window.  Which is why the identity of the principal it finds via its parent chain changes across the reload.  However the identity of the principal found via the shortcut on the xpconnect things is immutable; that's the root cause of the assert...

Blake, do we do the same sort of always-wrapping for history that we do for location, by any chance?  And why does that .H property persist on the navigator object across the reload?  That seems ... odd.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> In any case, the history object is parented to the _outer_ window, not the

This was a misunderstanding on my part. I'm going to be fixing that over in bug 619359.

> Blake, do we do the same sort of always-wrapping for history that we do for
> location, by any chance?  And why does that .H property persist on the
> navigator object across the reload?  That seems ... odd.

Navigator objects persist across two pages from the same domain (there's a bug filed to make it not do that). So presumably that's what's happening here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I'm going to be fixing that over in bug 619359.

OK.  Sounds like that will also fix this bug, then.

> Navigator objects persist across two pages from the same domain

Gah.  Good to hear we plan to stop doing that!

OK, so evaluation for this bug... If navigator is only preserved same-origin, then the exploit would not work as written for cross-origin stuff.  But the history object being parented to the outer window is indeed a problem... and tracked by bug 619359.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

This will be fixed by the patch in bug 619359 (currently going through try/review).

Comment 12

[Jesse Ruderman]

The testcases no longer assert for me.

This was fixed by the patch in bug 619359, right?  Do the tests from that bug test what we want to test?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> This was fixed by the patch in bug 619359, right?

Yes.

> Do the tests from that bug test what we want to test?

Which tests?

Comment 14

[Jesse Ruderman]

Sigh.