Jason Tarka Assignee
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170710085521

Steps to reproduce:

- Create a web page that contains a iframe containing another page
- Add a header to the iframed page: "Content-Security-Policy: frame-ancestors 'none'; report-uri /csp/report;"
- Load the parent page in Firefox, and observe the console and network log
  - Notice that an error is logged in the console, and a request is made to the URL specified by `report-uri`
- Change the name of the header on the iframed page to "Content-Security-Policy-Report-Only"
- Reload the parent page, and observe the console & network log


Actual results:

When the header is set to Report-Only:
- No errors are logged in the console
- No request is made to the URL specified by `report-uri`


Expected results:

When the header is set to Report-Only:
- Errors are logged in the console
- Request is made to the URL specified by `report-uri`

	Jason Tarka Assignee
	Comment 1 • 7 years ago

It should be noted that the MDN page for `frame-ancestors`[0] states:

> "This directive is not supported in the <meta> element or by the Content-Security-policy-Report-Only header field."

However reading the W3C spec[1] there is no mention of this. The closest I can find is about using CSP in <meta> elements[2], which states:

> "Note: The Content-Security-Policy-Report-Only header is not supported inside a meta element. Neither are the report-uri, frame-ancestors, and sandbox directives."

This leads me to believe that the MDN page was either referring to an older version of the spec, or the spec was misread. This is further backed up by the fact that Chrome acts on `frame-ancestors` when used in a `Content-Security-Policy-Report-Only` header.


[0]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors
[1]: https://w3c.github.io/webappsec-csp/#directive-frame-ancestors
[2]: https://w3c.github.io/webappsec-csp/#meta-element

	Jason Tarka Assignee
	Comment 2 • 7 years ago

I figured I'd take a look, and after finding the offending code fairly quickly, I decided I'd submit a fix for this myself.

This change removes a check for `frame-ancestors` while in report-only mode, so instead of simply skipping it, it will instead perform the analysis & report violations.

I tested this with my [example web pages][0]; without the change, report-only mode does not report on `frame-ancestors` violations. With the change, `frame-ancestors` violations are reported when in report-only mode.

I've run CPP tests locally and there are no failures. I'm not sure if there are other tests that I should be running.

[0]: https://bugzilla.mozilla.org/attachment.cgi?id=8886295

	Daniel Veditz [:dveditz]
	Comment 3 • 7 years ago

We ought to be reporting these violations in report-only mode, but we shouldn't prevent the child frame from loading in that case. We'll need to write tests to check in with the patch for this bug to prove it's working correctly.

	Christoph Kerschbaumer [:ckerschb]
	Comment 4 • 7 years ago

(In reply to Jason Tarka from comment #2)
> I've run CPP tests locally and there are no failures. I'm not sure if there
> are other tests that I should be running.

Thanks for taking a look at this bug. In fact I agree, we could/should send report violations for frame-ancestors in report-only mode. I am willing to accept that change if you could write a test to make sure it's working correctly. You can find similar tests within dom/security/test/csp. You can run the whole suite using:
./mach test dom/security/test/csp

Thanks for working on this!

	Jason Tarka Assignee
	Comment 5 • 7 years ago

My C++ is rusty, and it'll take me a little while to get to it, but I'll take a look at the existing tests and see what I can do.

	Christoph Kerschbaumer [:ckerschb]
	Comment 6 • 7 years ago

(In reply to Jason Tarka from comment #5)
> My C++ is rusty

You are already done with the C++ part :-)

What you would need is a mochitest (HTML + JS). Here is an example of how you would set that up using our testing infrastructure:
https://bugzilla.mozilla.org/attachment.cgi?id=8892397&action=diff

	Jason Tarka Assignee
	Comment 7 • 7 years ago

It took longer to get to this than I'd hoped, but now the patch included a mochitest to verify that when a CSP violation occurs in report-only mode a report is submitted, and the frame content still loads.

	Christoph Kerschbaumer [:ckerschb]
	Comment 8 • 7 years ago

Comment on attachment 8899499 [details] [diff] [review]
bug1380755.patch

Review of attachment 8899499 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for putting this together. There are really only a few nitpicks. Please address them before we can check those changes in.

::: dom/security/test/csp/file_bug1380755_child.html
@@ +4,5 @@
> +          href='/tests/dom/security/test/csp/file_CSP.sjs?testid=css_self&type=text/css' />
> +
> +  </head>
> +  <body>
> +    <img src="/tests/dom/security/test/csp/file_CSP.sjs?testid=img_self&type=img/png"> </img>

Please remove the external stylesheet as well as the external image loading. I think this can really be a dummy file like:
<html><body>foo</body></html>

::: dom/security/test/csp/mochitest.ini
@@ +90,5 @@
>    file_bug941404.html
>    file_bug941404_xhr.html
>    file_bug941404_xhr.html^headers^
> +  file_bug1380755_child.html
> +  file_bug1380755_child.html^headers^

please rename to file_frame_ancestors_ro.html and file_frame_ancestors_ro.html^headers^. We are trying to get away from using bug numbers as file names.

@@ +247,4 @@
>  [test_bug910139.html]
>  [test_bug909029.html]
>  [test_bug1229639.html]
> +[test_bug1380755.html]

please rename to test_frame_ancestors_ro.html]

::: dom/security/test/csp/test_bug1380755.html
@@ +30,5 @@
> +
> +  is(cspReport["original-policy"], "frame-ancestors 'none'; report-uri http://mochi.test:8888/foo.sjs",
> +     "Incorrect original-policy");
> +     
> +  testResults.reportFired = true;

nit: please remove trailing whitespace, or even better, just remove all empty lines from within checkResults()

@@ +47,5 @@
> +      ok(false, "Could not parse JSON (exception: " + e + ")");
> +    }
> +    try {
> +      // test for the proper values in the report object
> +      checkResults(reportObj);

I would be fine with moving checkResults() within the above try-block to avoid the additonal try-catch; up to you though.

	Jason Tarka Assignee
	Comment 9 • 7 years ago

- Renamed files
- Removed most of the content from the child file
- Removed extraneous whitespace & try...catch
- All CSP tests pass on my local build

	Christoph Kerschbaumer [:ckerschb]
	Comment 10 • 7 years ago

Comment on attachment 8915277 [details] [diff] [review]
bug1380755.patch

Review of attachment 8915277 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long, but I was out on PTO. But you already had my r+. Setting the checkin-need flag for you.

	Jason Tarka Assignee
	Comment 11 • 7 years ago

No worries, and thanks for the feedback.

Since I don't have committer access to the main repos, is there anything I can/need to do from here?

	Christoph Kerschbaumer [:ckerschb]
	Comment 12 • 7 years ago

(In reply to Jason Tarka from comment #11)
> Since I don't have committer access to the main repos, is there anything I
> can/need to do from here?

Nope, I'll make sure it gets into the codebase. Thanks for fixing!

	Pulsebot
	Comment 13 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ee927a4c37
Examine & report on frame-ancestors CSP in report-only mode. r=ckerschb

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 7 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 15 • 7 years ago

It looks like Jason has updated the actual ref page already - thanks for that!

I've just added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#HTTP