Closed
Bug 581226
Opened 14 years ago
Closed 14 years ago
Assertion failure: ASSERTION: DoContent returned no listener?: 'abort || m_targetStreamListener'
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: bsterne, Assigned: bsterne)
References
Details
Attachments
(2 files, 6 obsolete files)
14.36 KB,
patch
|
bsterne
:
review+
|
Details | Diff | Splinter Review |
16.42 KB,
patch
|
jst
:
review+
christian
:
approval1.9.2.13+
|
Details | Diff | Splinter Review |
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
Comment 1•14 years ago
|
||
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...
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Updated•14 years ago
|
Updated•14 years ago
|
blocking2.0: ? → betaN+
Assignee | ||
Comment 2•14 years ago
|
||
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.
Attachment #462233 -
Flags: review?(bzbarsky)
Comment 3•14 years ago
|
||
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;
Attachment #462233 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 4•14 years ago
|
||
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?
Attachment #462233 -
Attachment is obsolete: true
Attachment #465552 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
blocking1.9.2: .9+ → needed
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 465552 [details] [diff] [review]
fix v2
With bz on vacation, wondering if jst can provide review...
Attachment #465552 -
Flags: review?(bzbarsky) → review?(jst)
Assignee | ||
Comment 6•14 years ago
|
||
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.
Attachment #465552 -
Attachment is obsolete: true
Attachment #465794 -
Flags: review?(jst)
Attachment #465552 -
Flags: review?(jst)
Comment 7•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
Same as v3 but gets the top-level URI from that document's principal to do the same-origin check.
Attachment #465794 -
Attachment is obsolete: true
Attachment #466813 -
Flags: review?
Attachment #465794 -
Flags: review?(jst)
Assignee | ||
Updated•14 years ago
|
Attachment #466813 -
Flags: review? → review?(jst)
Comment 9•14 years ago
|
||
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.
Attachment #466813 -
Flags: review?(jst) → review+
Comment 10•14 years ago
|
||
> 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)?
Assignee | ||
Comment 11•14 years ago
|
||
Had to rebase the patch after bug 593387 landed. Also added the tests we discussed in comment 9.
Attachment #466813 -
Attachment is obsolete: true
Attachment #478274 -
Flags: review?(jst)
Assignee | ||
Updated•14 years ago
|
Attachment #478274 -
Attachment is patch: true
Attachment #478274 -
Attachment mime type: application/octet-stream → text/plain
Comment 12•14 years ago
|
||
That patch seems to be missing fixes for a bunch of the stuff from comment 3...
Assignee | ||
Comment 13•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #478274 -
Attachment is obsolete: true
Attachment #478428 -
Flags: review?(jst)
Attachment #478274 -
Flags: review?(jst)
Comment 15•14 years ago
|
||
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.
Attachment #478428 -
Flags: review?(jst) → review+
Assignee | ||
Comment 16•14 years ago
|
||
Carrying forward jst's r+
Attachment #478428 -
Attachment is obsolete: true
Attachment #479575 -
Flags: review+
Assignee | ||
Comment 17•14 years ago
|
||
I assume we'll want this on 1.9.2 as well.
Assignee | ||
Updated•14 years ago
|
Attachment #479577 -
Flags: review?(jst)
Updated•14 years ago
|
Attachment #479577 -
Flags: review?(jst) → review+
Assignee | ||
Comment 19•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Attachment #479577 -
Flags: approval1.9.2.12?
Attachment #479577 -
Flags: approval1.9.2.12? → approval1.9.2.12+
Comment 20•14 years ago
|
||
Assignee | ||
Comment 21•14 years ago
|
||
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?
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
This got checked in over in bug 608662 comment 12 which was about 24 hours ago so we should be good.
You need to log in
before you can comment on or make changes to this bug.
Description
•