Closed Bug 700561 Opened 10 years ago Closed 10 years ago

"ASSERTION: Principal mismatch. Expect bad things to happen" with view-source, proto

Categories

(Core :: XPConnect, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox8 - wontfix
firefox9 - wontfix
firefox10 - wontfix
firefox11 - wontfix
firefox12 + wontfix
firefox13 - verified

People

(Reporter: jruderman, Assigned: bholley)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:nse][qa!][advisory-tracking+])

Attachments

(3 files)

Attached file testcase
###!!! ASSERTION: Principal mismatch.  Expect bad things to happen: '!objPrin || objPrin->GetPrincipal() == principal', file js/xpconnect/src/XPCWrappedNative.cpp, line 2948

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

The second assertion is the same as in bug 696331, but the testcases are completely different.
Attached file stack traces
(In reply to Jesse Ruderman from comment #0)
> The second assertion is the same as in bug 696331, but the testcases are
> completely different.

It's a pretty generic assertion, so I'd be surprised if they were related. The stack traces are also quite different.

Henri, think it's possible that this is a view source regression? Seems plausible given that it popped up a few days after the landing...
Whiteboard: [sg:critical]
I only recently taught my fuzzer to generate view-source URLs. So this isn't necessarily a regression from last week.
Henri, can you investigate here? If this is unrelated to the recent view-source changes let me know...
Assignee: nobody → hsivonen
I see these assertions in an old Firefox 8 aurora tree (cset a5a5c583c381, beginning of Sept) so it has nothing to do with the recent view-source changes.
Bobby, this looks more like something you could dig into than Henri since this does not look related to any of the recent view-source related changes.
Assignee: hsivonen → bobbyholley+bmo
So. We're file://foo. We create an iframe, set its source to view-source:file://foo, insert the iframe into the DOM to kick off the load, and grab a reference to the contentDocument.

Now, this document is about:blank, and it gets ejected from the iframe as soon as control returns to the event loop. But thanks to the magic of inner window reuse, the inner window referenced by that about:blank document transforms into 'view-source:file://blah'. So the XPCWrappedNative of the document lives in a scope with a global object, and thus a principal, of 'view-source:file://blah'.

This seems kind of wrong, but I guess it doesn't matter for plain old about:blank. I'm curious what kind of checks we have in place to avoid this being a security issue. By my understanding, the whole point of inner window reuse is so that doing |newIframe.contentWindow.src='http://www.example.com'; newIframe.contentWindow.onfoo=function() {..}| will result in the |onfoo| handler being present for example.com. But what's to stop someone from doing something nastier to the about:blank window (since the about:blank document inherits the parent's principal), and then loading a victim page in the iframe?

But I digress. The scope is tied to the global object of the inner window, so it happily moves over to view-source:file://foo. But the document itself still has the initial principal it was given by the about:blank generation code in nsDocShell::EnsureContentViewer, which is the inherited principal "file://foo".

So when we do our debugging check where we compare the scope principal and the object principal, we assert: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#2957

This is a similar (but distinct) issue to bug 696331, in that it also involves the document principal and window principal getting out of sync. It seems to me like it would be great if we could just kill the document principal altogether, and always use the one from the inner window. Can we?

CCing bz, with the hopes that he'll weigh in on some of this stuff.
doh! Actually CCing bz this time.
> Now, this document is about:blank

And its principal is file://foo

> transforms into 'view-source:file://blah'.

No, view-source:file://foo.  Which happens to be same-origin with file://foo

> I'm curious what kind of checks we have in place to avoid this being a security issue. 

Inner window reuse only happens if the about:blank and the page being loaded are same-origin.  So you can only victimize things that are already same-origin with you, which you can just do directly.

> It seems to me like it would be great if we could just kill the document principal
> altogether, and always use the one from the inner window. Can we?

We have documents without inner windows, no?

The other option is to mutate the principal of the about:blank document whose inner window got reused to match the inner's new principal.
(In reply to Boris Zbarsky (:bz) from comment #9)

> > transforms into 'view-source:file://blah'.
> No, view-source:file://foo.

Err, right. I don't know where that came from. s/blah/foo/g in comment 9.

> Inner window reuse only happens if the about:blank and the page being loaded
> are same-origin.  So you can only victimize things that are already
> same-origin with you, which you can just do directly.

Ah, I see! Thanks for the clarification.

> > It seems to me like it would be great if we could just kill the document principal
> > altogether, and always use the one from the inner window. Can we?
> 
> We have documents without inner windows, no?

Ah. How does that happen? Just curious.

> The other option is to mutate the principal of the about:blank document
> whose inner window got reused to match the inner's new principal.

That sort of seems like whack-a-mole to me, but maybe I'm just pessimistic.

What about having the document hold onto a principal only in cases where it doesn't have an inner window? That would avoid the problem of keeping things in sync with our window, but still give us a story when we don't have one.
> Ah. How does that happen? Just curious.

XHR responseXML, DOMParser, documents created from a DOMImplementation via createDocument(), that sort of thing.

> What about having the document hold onto a principal only in cases where it doesn't have
> an inner window?

We could do that, I guess.... Note that this would also add a level of indirection to NodePrincipal() on all nodes.  Maybe that's OK.
(In reply to Boris Zbarsky (:bz) from comment #11)
> > What about having the document hold onto a principal only in cases where it doesn't have
> > an inner window?
> 
> We could do that, I guess.... Note that this would also add a level of
> indirection to NodePrincipal() on all nodes.  Maybe that's OK.

jst and mrbkap - what do you guys think? Is this a good idea? Or should we just sync up the principals wherever we find mismatches?
From discussing this with bholley earlier in the week this looks like it might be a bug resulting from our inconsistency with what nsIDocument::GetScriptGlobalObject() returns (bug 431767). Bobby was going to look into that once he had a reference to that bug, which he now has :)
Taking another look here, this doesn't seem to be related to bug 431767. As far as I can tell, everything is returning the right answer here.

As long as we're re-using inner windows in same-origin-but-different-principal cases, we can have wrappers whose underlying native has a different principal than the scope. We appear to be fine with this (it sounds like we're eventually moving towards per-origin principals anyway), so I think the answer here is to relax the assertions.

Flagging bz for review.
Attachment #596819 - Flags: review?(bzbarsky)
If my analysis is correct, this bug isn't security-sensitive. Let's see if bz agrees.
Comment on attachment 596819 [details] [diff] [review]
Bug 700561 - Relax short-circuit principal checks on account of inner window reuse. v1

r=me
Attachment #596819 - Flags: review?(bzbarsky) → review+
pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/44bdafff8b4c

I think this bug can be un-hidden. Do you agree, bz?
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/44bdafff8b4c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Opening this up, and downgrading.
Whiteboard: [sg:nse] → [sg:nse][qa+]
I've verified this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120517 Firefox/13.0 deta debug build.

There is no assertion in the console when running the testcase from the description.
Setting the flag and also the resolution to Verified Fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:nse][qa+] → [sg:nse][qa!]
Whiteboard: [sg:nse][qa!] → [sg:nse][qa!][advisory-tracking+]
You need to log in before you can comment on or make changes to this bug.