Caused by bug 475530. In nsDSURIContentListener::DoContent, after we call mDocShell->CreateContentViewer, we should return the error code we got back from that call if it fails rather than NS_OK. That also will mean we need to propagate the correct error code, e.g. NS_ERROR_CONTENT_BLOCKED, back out rather than returning NS_ERROR_FAILURE, generically. WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /build/m-c/mozilla/content/base/src/nsDocument.cpp, line 2166 WARNING: NS_ENSURE_SUCCESS(docLoaderFactory->CreateInstance("view", aOpenedChannel, aLoadGroup, aContentType, static_cast<nsIContentViewerContainer*>(this), 0L, aContentHandler, aViewer), ((nsresult) 0x80004005L)) failed with result 0x805E0006: file /build/m-c/mozilla/docshell/base/nsDocShell.cpp, line 7236 ###!!! ASSERTION: DoContent returned no listener?: 'abort || m_targetStreamListener', file /build/m-c/mozilla/uriloader/base/nsURILoader.cpp, line 776

I agree that the code in DoContent needs changing; the problem it was hacking around no longer exists. That said, just returning an error here looks bogus to me; that will just look for another place to dispatch the content to (possibly popping up things like helper app dialogs), won't it? It seems like it would make a lot more sense to set *aAbortProcess to true and return NS_OK. Of course that also means we would need to know in this code about the x-frame-options stuff... but it would make a LOT more sense to me to implement that completely in nsDSURIContentListener::DoContent instead of nsDocument::StartDocumentLoad. In particular, it would make it a lot less scary to be calling LoadURI from inside that code (though it still worries me a bit) than the current setup which actually sets up a broken document viewer before bailing out and starting a new load partway through the viewer setup. I'm a little surprised that doesn't completely confuse the docshell. Why _did_ we put this code in nsDocument instead of DoContent? fwiw, the LoadURI call there scares me enough that I think it's a must-fix before we ship this code...

Moves CheckFrameOptions from nsDocument to nsDSURIContentListener. CheckFrameOptions now gets called from nsDSURIContentListener::DoContent instead of nsDocument::StartDocumentLoad. Boris, is this what you had in mind? It's a much smaller patch than the 9K size would indicate, due to moving the check from one place to the other. The logic is essentially the same, though I had to change the signature of CFO to take the nsIRequest that DoContent is processing.

Comment on attachment 462233 [details] [diff] [review] fix >+++ b/docshell/base/nsDSURIContentListener.cpp >+#include "nsContentUtils.h" You can't use nsContentUtils in here; it won't compile in non-libxul builds. Just do_GetService the security manager as needed. >+ PRBool framingAllowed = CheckFrameOptions(request); This is all internal code; just use |bool| and |true| and |false| instead of a mix of those and the PR stuff. >+ if (!framingAllowed) { >+ *aAbortProcess = PR_TRUE; This, though, needs to stay PR_TRUE. Note that you don't need the framingAllowed temporary here. Also, setting *aAbortProcess to PR_FALSE should probably move down below this block, and the null-check of aAbortProcess should go away. >+// Check if X-Frame-Options permits this content to be loaded as a subdocument. s/if/whether/ >+PRBool nsDSURIContentListener::CheckFrameOptions(nsIRequest* request) |bool|, and so forth. >+ // XXXbsterne should this use nsGkAtoms::headerXFO instead of >+ // the NS_LITERAL_CSTRING No; the necko api takes strings. Please remove this comment. >+ PRBool framingAllowed = true; So the only way this can stay |true| at this point is if the header is "sameorigin" and either !topDoc (which I argue can't happen sanely; I would prefer to treat framingAllowed as true in that situation) or the security check fails. So I think it would make more sense to just nix this temporary and have the code look something like this after the window equality comparison: if (xfoHeaderValue.LowerCaseEqualsLiteral("sameorigin")) { // get the top doc, do the security check into rv if (NS_SUCCEEDED(rv)) { return true; } } else { NS_ASSERTION(xfoHeaderValue.LowerCaseEqualsLiteral("deny"), "How did we get here with some random header value?"); } // cancel the load, etc This does raise one question. Do we really want to be using the URI of the top document and of the channel as opposed to comparing principals? Principals would make somewhat more sense to me, though we have to be careful about document.domain (I assume this check is supposed to ignore that, right?). As written, the code will do weird things if the toplevel window is a data: document, or something generated with javascript:, or even just about:blank, as far as I can tell. >+ >+ // We need to check the location of this window and the location of the >+ // top window, if we're not the top. X-F-O: SAMEORIGIN requires that the >+ // document must be same-origin with top window. X-F-O: DENY requires that >+ // the document must never be framed. >+ nsCOMPtr<nsIDOMWindow> thisWindow = do_GetInterface(static_cast<nsIDocShell*>(mDocShell)); >+ nsCOMPtr<nsIDOMWindow> topWindow; >+ thisWindow->GetTop(getter_AddRefs(topWindow)); >+ >+ // if the document is in the top window, it's not in a frame. >+ if (thisWindow == topWindow) >+ return PR_TRUE; >+ >+ // If the value of the header is DENY, then the document >+ // should never be permitted to load as a subdocument. >+ if (xfoHeaderValue.LowerCaseEqualsLiteral("deny")) { >+ framingAllowed = false; >+ } >+ >+ else if (xfoHeaderValue.LowerCaseEqualsLiteral("sameorigin")) { >+ // If the X-Frame-Options value is SAMEORIGIN, then the top frame in the >+ // parent chain must be from the same origin as this document. >+ nsCOMPtr<nsIURI> uri; >+ httpChannel->GetURI(getter_AddRefs(uri)); >+ nsCOMPtr<nsIDOMDocument> topDOMDoc; >+ topWindow->GetDocument(getter_AddRefs(topDOMDoc)); >+ nsCOMPtr<nsIDocument> topDoc = do_QueryInterface(topDOMDoc); >+ if (topDoc) { >+ nsCOMPtr<nsIURI> topUri = topDoc->GetDocumentURI(); >+ nsresult rv = nsContentUtils::GetSecurityManager()-> >+ CheckSameOriginURI(uri, topUri, PR_TRUE); >+ >+ if (NS_FAILED(rv)) { >+ framingAllowed = false; >+ } >+ } >+ } >+ >+ if (!framingAllowed) { >+ // cancel the load and display about:blank >+ httpChannel->Cancel(NS_BINDING_ABORTED); >+ nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(static_cast<nsIDocShell*>(mDocShell))); >+ if (webNav) { >+ webNav->LoadURI(NS_LITERAL_STRING("about:blank").get(), >+ 0, nsnull, nsnull, nsnull); >+ } >+ return PR_FALSE; >+ } >+ } >+ >+ return PR_TRUE; >+} >diff --git a/docshell/base/nsDSURIContentListener.h b/docshell/base/nsDSURIContentListener.h >--- a/docshell/base/nsDSURIContentListener.h >+++ b/docshell/base/nsDSURIContentListener.h >@@ -63,16 +63,20 @@ public: > protected: > nsDSURIContentListener(nsDocShell* aDocShell); > virtual ~nsDSURIContentListener(); > > void DropDocShellreference() { > mDocShell = nsnull; > } > >+ // Determine if X-Frame-Options allows content to be framed >+ // as a subdocument >+ PRBool CheckFrameOptions(nsIRequest* request); >+ > protected: > nsDocShell* mDocShell; > > // Store the parent listener in either of these depending on > // if supports weak references or not. Proper weak refs are > // preferred and encouraged! > nsWeakPtr mWeakParentContentListener; > nsIURIContentListener* mParentContentListener;

Thanks for the great feedback. This patch addresses all of your previous comments except for: > This does raise one question. Do we really want to be using the URI of the top > document and of the channel as opposed to comparing principals? Principals > would make somewhat more sense to me, though we have to be careful about > document.domain (I assume this check is supposed to ignore that, right?). As > written, the code will do weird things if the toplevel window is a data: > document, or something generated with javascript:, or even just about:blank, as > far as I can tell. I don't have enough experience with this code to understand why comparing principals is better or what weird things to expect with the current approach. I can tell you that I tested the data: url case with, e.g. data:text/html,<iframe src="x-f-o:deny page"></iframe> and it was successfully blocked. All the unit tests I wrote for X-Frame-Options originally continue to pass with this patch and the assertion is fixed. Given the blocking 1.9.2 nature of this bug, can we take this patch to fix this bug and follow up (potentially) with a separate bug on switching to principal comparison?

Comment on attachment 465552 [details] [diff] [review] fix v2 With bz on vacation, wondering if jst can provide review...

Missed a qrefresh to address Boris' comment: > Note that you don't need the framingAllowed temporary here. Also, setting > *aAbortProcess to PR_FALSE should probably move down below this block, and the > null-check of aAbortProcess should go away.

Comment on attachment 465794 [details] [diff] [review] fix v3 - In nsDSURIContentListener::CheckFrameOptions(nsIRequest* request): + if (xfoHeaderValue.LowerCaseEqualsLiteral("sameorigin")) { + nsCOMPtr<nsIURI> uri; + httpChannel->GetURI(getter_AddRefs(uri)); + nsCOMPtr<nsIDOMDocument> topDOMDoc; + topWindow->GetDocument(getter_AddRefs(topDOMDoc)); + nsCOMPtr<nsIDocument> topDoc = do_QueryInterface(topDOMDoc); + if (topDoc) { + nsCOMPtr<nsIURI> topUri = topDoc->GetDocumentURI(); I think Boris is right in that this should use the document's principal and not the uri in order for this to work sanely when dealing with about:blank, javascript: URIs, etc. While most cases that should be blocked with this code will be blocked, it seems there are cases where this code will block things that it should not (if top's uri is javascript:blah, for instance, a child frame from the same origin as that of the javascript: uri should be considered same origin, and would be if this used the principals, but won't be with this code). Given that those cases are far from the norm I'd be fine with landing this as is if need be, and filing a followup bug to change this to use principals. r=jst with that in mind.

Same as v3 but gets the top-level URI from that document's principal to do the same-origin check.

Comment on attachment 466813 [details] [diff] [review] fix v4 r=jst But the document.domain issue bz brought up should still be agreed upon one way or another, and in talking through this with Brandon, dveditz, Sid, and Jonas, we agreed that this code should *not* take document.domain into account, and that's how this patch already works. So I say let's land this, write a test that ensures that the behavior doesn't change from the current behavior wrt document.domain, and that test can be a followup to this bug or patch.

> I don't have enough experience with this code to understand why comparing > principals is better Because in general the URI of the page is only loosely related to what its security context is. The principal is what represents the security context. They usually more or less match up for http:// pages, but for other URI schemes all bets are off. I believe the main upshot of comparing uris instead of principals is that things that should not be blocked will be blocked... Are we very sure that there are no null URIs around in that last patch (e.g. that all the principals are not system)?

Had to rebase the patch after bug 593387 landed. Also added the tests we discussed in comment 9.

That patch seems to be missing fixes for a bunch of the stuff from comment 3...

(In reply to comment #12) > That patch seems to be missing fixes for a bunch of the stuff from comment 3... Indeed :-( I did a poor job of rebasing and will take care to incorporate _all_ of the previous feedback for the final version of the patch. jst also pointed out on IRC that: + if (xfoHeaderValue.LowerCaseEqualsLiteral("sameorigin")) { + nsCOMPtr<nsIURI> uri; + httpChannel->GetURI(getter_AddRefs(uri)); + nsCOMPtr<nsIDOMDocument> topDOMDoc; + topWindow->GetDocument(getter_AddRefs(topDOMDoc)); + topDoc = do_QueryInterface(topDOMDoc); is wrong, because we already got the correct "top" document that we want to use for the comparison in the parent chain traversal above.

Comment on attachment 478428 [details] [diff] [review] fix v6 + // Traverse up the parent chain to the top docshell that doesn't have + // a system principal + curDocShellItem->GetParent(getter_AddRefs(parentDocShellItem)); + while (parentDocShellItem) { + PRBool system = PR_FALSE; + topDoc = do_GetInterface(parentDocShellItem); + if (topDoc) { + if (NS_SUCCEEDED(ssm->IsSystemPrincipal(topDoc->NodePrincipal(), + &system)) && system) { + break; + } + } + else { + return false; + } + curDocShellItem = parentDocShellItem; + curDocShellItem->GetParent(getter_AddRefs(parentDocShellItem)); + } You could save yourself the duplicated GetParent() call if you make the above while() loop look like something like this: // Traverse up the parent chain to the top docshell that doesn't have // a system principal while (NS_SUCCEEDED(curDocShellItem->GetParent(getter_AddRefs(parentDocShellItem))) && parentDocShellItem) { PRBool system = PR_FALSE; topDoc = do_GetInterface(parentDocShellItem); if (topDoc) { if (NS_SUCCEEDED(ssm->IsSystemPrincipal(topDoc->NodePrincipal(), &system)) && system) { break; } } else { return false; } curDocShellItem = parentDocShellItem; } r=jst either way.

Carrying forward jst's r+

I assume we'll want this on 1.9.2 as well.

The 1.9.2 checkin will regress us on bug 608662. The 1.9.2 patch in this bug was made before 608662 was filed. How should we resolve this? It's trivial to re-add the null check that fixed the crash, but I don't know how to handle this from a flag-setting perspective. Reed or LegNeato, what's the call?

Is this going to be checked in today? I'm afraid that we'll miss this and have builds generated without it very early Monday morning.

This got checked in over in bug 608662 comment 12 which was about 24 hours ago so we should be good.