Closed Bug 1031658 Opened 6 years ago Closed 6 years ago

Popup blocker bypass (accidental Facebook tab)

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 - ---
firefox-esr31 --- unaffected

People

(Reporter: jruderman, Assigned: geekboy)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, sec-low)

Attachments

(1 file, 1 obsolete file)

Hmm, not too much work on popup blocking recently.
Jst, might remember some more about that code.
I don't get any new tab when loading that page.
Try in a blank profile? It happens reliably for me on Mac.
Here's what I see so far:

1)  nsDSURIContentListener::DoContent returns failure (why, I haven't figured out yet, but the CSP assertions I'm seeing are not that encouraging).

2)  The URI loader tries to find content handlers for the MIME type involved (text/html).

3)  It finds http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js which goes ahead and opens a new tab with the content.
Alright, before the DoContent failure I see this:

[41484] WARNING: aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474: file ../../../../mozilla/content/base/src/nsCSPContext.cpp, line 297
[41484] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mozilla/content/base/src/nsCSPContext.cpp, line 1029
[41484] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mozilla/content/base/src/nsDocument.cpp, line 2835
[41484] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../mozilla/content/base/src/nsDocument.cpp, line 2606
[41484] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../mozilla/layout/build/nsContentDLF.cpp, line 405
[41484] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../mozilla/docshell/base/nsDocShell.cpp, line 8462
So for those failures, we're landing in nsDSURIContentListener::DoContent, calling nsDocShell::CreateContentViewer which does various stuff and then ends up in nsDocument::StartDocumentLoad which does nsDocument::InitCSP.

InitCSP calls nsCSPContext::PermitsAncestry, with a docshell.  The parent of that docshell has something like wyciwyg://817/http://www.pantagraph.com/lifestyles/hypermiler-goes-to-extreme-for-incredible-gas-mileage/article_4d6e2343-3da3-55f5-9470-6655af5bd50b.html as the current URI, because it's generated via document.open.  This code is neither getting the exposable URI nor looking at the actual origin; presumably it should do one or the other, because what it's doing right now is not very sensible.  So then it clones the the URI, tries to SetUserPass on it, and that of course fails, so this code bails out.

But just bailing out means "hey, don't load this in this docshell but keep trying to load it".  That does NOT seem like the right behavior for CSP failures!

So there are several CSP bugs here:

1)  This code is totally using the wrong URIs.  I'm not sure which ones it should be using, but not the ones it's using.

2)  Failures in this code should presumably kill the load, not just bail out to trying a different handler.

3)  The asserts.  Those are:

###!!! ASSERTION: Can not convert CSPDirective into nsContentPolicyType: 'false', file ../../../../mozilla/content/base/src/nsCSPUtils.cpp, line 671

mDirective is CSP_CONNECT_SRC.  I get that assert very reliably on this site.

This is a problem on nightly, not beta/aurora, so presumably effective regression from turning on this C++ code...
Blocks: 925004
Component: General → DOM: Security
Flags: needinfo?(sstamm)
Flags: needinfo?(mozilla)
Flags: needinfo?(grobinson)
Keywords: regression
(In reply to Boris Zbarsky [:bz] from comment #6)
> So for those failures, we're landing in nsDSURIContentListener::DoContent,
> calling nsDocShell::CreateContentViewer which does various stuff and then
> ends up in nsDocument::StartDocumentLoad which does nsDocument::InitCSP.
> 
> InitCSP calls nsCSPContext::PermitsAncestry, with a docshell.  The parent of
> that docshell has something like
> wyciwyg://817/http://www.pantagraph.com/lifestyles/hypermiler-goes-to-
> extreme-for-incredible-gas-mileage/article_4d6e2343-3da3-55f5-9470-
> 6655af5bd50b.html as the current URI, because it's generated via
> document.open.  This code is neither getting the exposable URI nor looking
> at the actual origin; presumably it should do one or the other, because what
> it's doing right now is not very sensible.  So then it clones the the URI,
> tries to SetUserPass on it, and that of course fails, so this code bails out.
> 
> But just bailing out means "hey, don't load this in this docshell but keep
> trying to load it".  That does NOT seem like the right behavior for CSP
> failures!
> 
> So there are several CSP bugs here:
> 
> 1)  This code is totally using the wrong URIs.  I'm not sure which ones it
> should be using, but not the ones it's using.
> 
> 2)  Failures in this code should presumably kill the load, not just bail out
> to trying a different handler.
> 
> 3)  The asserts.  Those are:
> 
> ###!!! ASSERTION: Can not convert CSPDirective into nsContentPolicyType:
> 'false', file ../../../../mozilla/content/base/src/nsCSPUtils.cpp, line 671
> 
> mDirective is CSP_CONNECT_SRC.  I get that assert very reliably on this site.
> 
> This is a problem on nightly, not beta/aurora, so presumably effective
> regression from turning on this C++ code...

Thanks for bringing it to my attention. The problem is that some contentTypes are missing in our mapping table [1] which caused that assertion. We have identified the problem, the fixing patch got r+ed; currently working on testcoverage - will land shortly.

https://bugzilla.mozilla.org/show_bug.cgi?id=1031530
Flags: needinfo?(mozilla)
Great.  So let's focus on #1 and #2 in this bug.
1) What URI should be used?  Should we be grabbing the inner URI if it's nested?  I'm not 100% confident which URIs to pull out of the various DocShellTreeItems and currentURI looked appropriate.

2) The main difference between old and new CSP backend is that the new one doesn't accept failure of SetUserPass inside PermitsAncestry (bails early via NS_ENSURE_SUCCESS at nsCSPContext.cpp:1029) whereas the old one did (via empty catch block).  This NS_ENSURE_SUCCESS is likely the cause of different behavior once we enabled the new CSP backend.  I'm confirming now.

I agree that bailing early is bad, but failure to SetUserPass in this case is okay (that's just an attempt to remove any UserPass from the check to avoid using it in comparison or accidentally leaking it via CSP violation reporting).  We should convert this to allow SetUserPass to fail quietly.  Though it shouldn't fail due to wyciwyg.

3) Chris already commented on that.
Flags: needinfo?(sstamm)
For #1, what does the spec say?  Does it talk about document URIs, or document origins?  Those are quite different in about:blank cases, note, so it's even possible the spec is buggy depending on what the desired behavior in those cases is.

So let's think about the desired behavior.  If I have a site at "http://foo.example.com/something/else" that has an "about:blank" subframe and inside that it creates an <iframe> (let's call it "iframe X") pointing to some site that uses CSP, what is the relevant URI for CSP checks in iframe X?  Is it "about:blank" or is it "http://foo.example.com/" or is it "http://foo.example.com/something/else"?

> This NS_ENSURE_SUCCESS is likely the cause of different behavior
> once we enabled the new CSP backend.

Yes, that would do it.

> I agree that bailing early is bad

I don't think bailing early is a problem.  It's bailing from this code but continuing the load anyway that's a problem, no?
(In reply to Boris Zbarsky [:bz] from comment #10)
> For #1, what does the spec say?  Does it talk about document URIs, or
> document origins?  Those are quite different in about:blank cases, note, so
> it's even possible the spec is buggy depending on what the desired behavior
> in those cases is.
> 
> So let's think about the desired behavior.  If I have a site at
> "http://foo.example.com/something/else" that has an "about:blank" subframe
> and inside that it creates an <iframe> (let's call it "iframe X") pointing
> to some site that uses CSP, what is the relevant URI for CSP checks in
> iframe X?  Is it "about:blank" or is it "http://foo.example.com/" or is it
> "http://foo.example.com/something/else"?
> 
> > This NS_ENSURE_SUCCESS is likely the cause of different behavior
> > once we enabled the new CSP backend.
> 
> Yes, that would do it.
> 
> > I agree that bailing early is bad
> 
> I don't think bailing early is a problem.  It's bailing from this code but
> continuing the load anyway that's a problem, no?

Yeah, you're right.  Instead of propogating the error out of InitCSP and out of StartDocumentLoad, we should catch it and block the load if PermitsAncestry fails:

2833     // PermitsAncestry sends violation reports when necessary
2834     rv = csp->PermitsAncestry(docShell, &safeAncestry);
2835     NS_ENSURE_SUCCESS(rv, rv);
2836 
2837     if (!safeAncestry) {

Should be:

2833     // PermitsAncestry sends violation reports when necessary
2834     rv = csp->PermitsAncestry(docShell, &safeAncestry);
2835 
2836     if (NS_FAILED(rv) || !safeAncestry) {
Yes, that sounds much better!
(In reply to Boris Zbarsky [:bz] from comment #10)
> For #1, what does the spec say?  Does it talk about document URIs, or
> document origins?  Those are quite different in about:blank cases, note, so
> it's even possible the spec is buggy depending on what the desired behavior
> in those cases is.

Well, the spec is not done yet (frame-ancestors didn't make it into CSP 1.0).  

The working draft refers to "active document" and "document's URL".
https://w3c.github.io/webappsec/specs/content-security-policy/#directive-frame-ancestors

> So let's think about the desired behavior.  If I have a site at
> "http://foo.example.com/something/else" that has an "about:blank" subframe
> and inside that it creates an <iframe> (let's call it "iframe X") pointing
> to some site that uses CSP, what is the relevant URI for CSP checks in
> iframe X?  Is it "about:blank" or is it "http://foo.example.com/" or is it
> "http://foo.example.com/something/else"?

Thinking "out loud" here... My gut feeling is that we want to know the active document's URI for all frames up to the top.  So we'd be concerned with both about:blank (which is weird, hm...) and the top document at foo.example.com.  If I had to choose, I'd just whitelist about:blank in general, but proceed to check its parent to make sure that's also authorized.

Are there other cases aside from the about:blank-created frame that would end up with a document where the currentURI is different than a run-of-the-mill http document (like this bug's wyciwyg: URI)?
Attached patch WIP (obsolete) — Splinter Review
Here's a patch for the PermitsAncestry failure case handling.  Still not sure what to do about the URIs.
Assignee: nobody → sstamm
Flags: needinfo?(grobinson)
> My gut feeling is that we want to know the active document's URI for all frames up to
> the top.

Again, its URI or its origin?

> a document where the currentURI is different than a run-of-the-mill http document

javascript, blob:, data: come to mind.  Oh, about:srcdoc too.
(In reply to Boris Zbarsky [:bz] from comment #15)
> > My gut feeling is that we want to know the active document's URI for all frames up to
> > the top.
> 
> Again, its URI or its origin?

The spec never says origin, only says URL and URI.  So URI.  Eventually paths will be valid as CSP sources too (not just scheme/host/port).

> > a document where the currentURI is different than a run-of-the-mill http document
> 
> javascript, blob:, data: come to mind.  Oh, about:srcdoc too.

I think the first three make sense.  Can about:srcdoc embed frames or iframes from http/https locations (I admit I don't know much about how that or wyciwyg: locations work)?

How did the wyciwyg: location come out of document.open() (comment 6)?  Would it make more sense to grab the inner URI if there is one from that scheme and view-source: as well?
> Can about:srcdoc embed frames or iframes from http/https locations

Sure, why not?  It can do anything that data: can.

wyciwyg is just used for document.open/document.write.  Per spec it doesn't even exist.

> How did the wyciwyg: location come out of document.open() (comment 6)?

It's an internal implementation detail we have for keeping track of the actual written data for reload purposes.

> Would it make more sense to grab the inner URI

There is no inner URI in the wyciwyg case.

What you should do is that if the spec says to use the document URI you should use the document URI.  That will make things consistent and all.  If you have to use the docshell's current URI, you need to CreateExposableURI from it to handle the wyciwyg case.  

And it sounds like we might need spec fixes here for the various things that inherit origins.... or something.
Attached patch proposed patchSplinter Review
Ok, this patch does three things:

1) changes the way errors in PermitsAncestry are handled (if NS_FAILED, CSP blocks the rendering of the protected document)
2) Allows SetUserPass to silently fail (since it probably means there's no userpass)
3) Overhauls the ancestor URI hunting to something simple: docShellTreeItem->GetDocument()->GetDocumentURI().  I think that was unavailable in script, which is why we didn't use that in the old CSP backend.

Note that if you want to test this, there are known bugs in test_CSP_frameancestors.html so you may want to apply the patch in bug 991972 first.
Attachment #8448402 - Attachment is obsolete: true
Attachment #8449096 - Flags: review?(bzbarsky)
Depends on: 991972
Blocks: CSP
Comment on attachment 8449096 [details] [diff] [review]
proposed patch

This seems ok, assuming that there is a followup (with spec issues?) for those cases discussed earlier with blob:, about:srcdoc, about:blank, etc.
Attachment #8449096 - Flags: review?(bzbarsky) → review+
Blocks: 1033675
I filed bug 1033675 for the scheme issues.  Landed this with the test_CSP_frameancestors.html mochitest fix in bug 991972.

https://hg.mozilla.org/integration/mozilla-inbound/rev/edcc53a65a0b
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/edcc53a65a0b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.