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

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Bobby Holley (:bholley)]

(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...

Comment 3

[Jesse Ruderman]

I only recently taught my fuzzer to generate view-source URLs. So this isn't necessarily a regression from last week.

Comment 4

[Johnny Stenback (:jst)]

Henri, can you investigate here? If this is unrelated to the recent view-source changes let me know...

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Johnny Stenback (:jst)]

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.

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Bobby Holley (:bholley)]

doh! Actually CCing bz this time.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Bobby Holley (:bholley)]

(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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Bobby Holley (:bholley)]

(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?

Comment 13

[Johnny Stenback (:jst)]

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

Comment 14

[Bobby Holley (:bholley)]

Attachment: Bug 700561 - Relax short-circuit principal checks on account of inner window reuse. v1

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.

Comment 15

[Bobby Holley (:bholley)]

If my analysis is correct, this bug isn't security-sensitive. Let's see if bz agrees.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 596819 [details] [diff] [review]
Bug 700561 - Relax short-circuit principal checks on account of inner window reuse. v1

r=me

Comment 17

[Bobby Holley (:bholley)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ebdd13c91942

Comment 18

[Bobby Holley (:bholley)]

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?

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44bdafff8b4c

Comment 20

[Johnny Stenback (:jst)]

Opening this up, and downgrading.

Comment 21

[Vlad [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.