Last Comment Bug 700561 - "ASSERTION: Principal mismatch. Expect bad things to happen" with view-source, proto
: "ASSERTION: Principal mismatch. Expect bad things to happen" with view-sourc...
Status: VERIFIED FIXED
[sg:nse][qa!][advisory-tracking+]
: assertion, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
Depends on:
Blocks: 326633 594645
  Show dependency treegraph
 
Reported: 2011-11-07 21:31 PST by Jesse Ruderman
Modified: 2012-05-22 16:43 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
-
wontfix
-
wontfix
+
wontfix
-
verified


Attachments
testcase (451 bytes, text/html)
2011-11-07 21:31 PST, Jesse Ruderman
no flags Details
stack traces (6.04 KB, text/plain)
2011-11-07 21:33 PST, Jesse Ruderman
no flags Details
Bug 700561 - Relax short-circuit principal checks on account of inner window reuse. v1 (2.64 KB, patch)
2012-02-13 15:29 PST, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-11-07 21:31:07 PST
Created attachment 572742 [details]
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 2011-11-07 21:33:46 PST
Created attachment 572743 [details]
stack traces
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2011-11-09 16:26:46 PST
(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 2011-11-10 11:28:37 PST
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, jst@mozilla.com) 2011-11-10 13:36:40 PST
Henri, can you investigate here? If this is unrelated to the recent view-source changes let me know...
Comment 5 Daniel Veditz [:dveditz] 2011-11-10 17:39:36 PST
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, jst@mozilla.com) 2011-12-08 13:51:36 PST
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) (busy with Stylo) 2012-01-04 16:45:33 PST
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) (busy with Stylo) 2012-01-04 16:45:54 PST
doh! Actually CCing bz this time.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-01-04 17:12:15 PST
> 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) (busy with Stylo) 2012-01-04 17:30:13 PST
(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 [:bz] (still a bit busy) 2012-01-04 17:36:38 PST
> 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) (busy with Stylo) 2012-01-04 17:51:50 PST
(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, jst@mozilla.com) 2012-01-13 12:41:40 PST
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) (busy with Stylo) 2012-02-13 15:29:41 PST
Created attachment 596819 [details] [diff] [review]
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) (busy with Stylo) 2012-02-16 14:21:03 PST
If my analysis is correct, this bug isn't security-sensitive. Let's see if bz agrees.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-02-17 22:03:39 PST
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) (busy with Stylo) 2012-02-17 23:54:06 PST
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ebdd13c91942
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-02-18 08:59:23 PST
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] 2012-02-20 10:25:27 PST
https://hg.mozilla.org/mozilla-central/rev/44bdafff8b4c
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 13:58:14 PST
Opening this up, and downgrading.
Comment 21 Vlad [QA] 2012-05-21 05:08:09 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.