1031658 - Popup blocker bypass (accidental Facebook tab)

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Jesse Ruderman]

When I load http://www.pantagraph.com/lifestyles/hypermiler-goes-to-extreme-for-incredible-gas-mileage/article_4d6e2343-3da3-55f5-9470-6655af5bd50b.html I get a new tab with a Facebook button.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Hmm, not too much work on popup blocking recently.
Jst, might remember some more about that code.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I don't get any new tab when loading that page.

Comment 3

[Jesse Ruderman]

Try in a blank profile? It happens reliably for me on Mac.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Great.  So let's focus on #1 and #2 in this bug.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

(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) {

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, that sounds much better!

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: WIP

Here's a patch for the PermitsAncestry failure case handling.  Still not sure what to do about the URIs.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edcc53a65a0b