Ian Melven :imelven Reporter
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #725490 +++

bug 725490 is about X-Frame-Options: SAMEORIGIN only checking that the top level frame is same origin, when all ancestors should be checked for same-origin-ness 

this is how the CSP UI Safety Draft specifies the frame-options CSP directive:
https://dvcs.w3.org/hg/user-interface-safety/raw-file/ca2e54aaf765/user-interface-safety.html#frame-options 

"If the directive-value contains the keyword-source 'self' alone to indicate that the resource may be embedded only if all ancestors are in the same origin as the protected resource."

there is already some level of support for this in Gecko's CSP implementation due to its support for the non-spec frame-ancestors directive, see https://wiki.mozilla.org/Security/CSP/Specification (now deprecated by the W3C CSP spec)

	Devdatta Akhawe [:devd]
	Comment 1 • 11 years ago

I think you mean frame-options; frame-ancestors is already implemented. This is a relatively minor fix: frame-ancestors is already implemented in Mozilla. We would only need to change line 196 to look for the "frame-options". (and a couple of other changes). That said, I would wait for the UI Safety spec to finalize.

	Ian Melven :imelven Reporter
	Comment 2 • 11 years ago

(In reply to Devdatta Akhawe [:devd] from comment #1)
> I think you mean frame-options; frame-ancestors is already implemented.

thanks, I kept mixing them up ! 

> This is a relatively minor fix: frame-ancestors is already implemented in
> Mozilla. We would only need to change line 196 to look for the
> "frame-options". (and a couple of other changes). That said, I would wait
> for the UI Safety spec to finalize.

fair enough, looks like we need to resolve a couple of issues in that section of the spec

	Ian Melven :imelven Reporter
	Comment 3 • 10 years ago

This has now become frame-ancestors in the CSP 1.1 spec.

There is code already in the existing CSP implementation to handle frame-ancestors, it just needs to be aligned with the spec (and perhaps moved behind the pref gating CSP 1.1 features that aren't in CSP 1.0)

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 4 • 10 years ago

I don't think it needs to be moved behind a gating pref.  It's fairly well tested and mature so we should just bring the implementation into alignment with CSP 1.1 spec.

	Devdatta Akhawe [:devd]
	Comment 5 • 10 years ago

Whats the work needed for alignment? My reading suggests it is already aligned with CSP1.1 spec (http://w3c.github.io/webappsec/specs/content-security-policy/csp-specification.dev.html#frame-ancestors)

	Devdatta Akhawe [:devd]
	Comment 6 • 10 years ago

Sorry, I think Step 2.2 and 2.3 to ensure that the frame-ancestors policy does not leak needs to be implemented. Although, I am not sure how big a threat "leaking the frame policy for a document" is.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 7 • 10 years ago

The current draft says this about 2.2 and 2.3:
"The user agent MAY implement these steps by instead redirecting the user to friendly error page in a unique origin"

That's our current M.O.  We show a "blocked by CSP" error page.

The current editor's draft does say to do something we don't:
"When generating a violation report for a frame-ancestors violation, the user agent MUST NOT include the value of the embedding ancestor as a blocked-uri value unless it is same-origin with the protected resource, as disclosing the value of cross-origin ancestors is a violation of the Same-Origin Policy."

So that means we need a patch that:
* drops the ancestor's URI when it's cross origin.
* disables the frame-ancestors check for report-only policies (when "monitoring")

It also says to ignore frame-ancestors for policies in meta tags.  I think that'll happen automatically since the meta tags are parsed after the document is inited, but will put a note in

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 8 • 10 years ago

... bug 663570.  I'll put a note in bug 663570.  Pressed enter too soon.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 9 • 10 years ago

Hey Dev, can you check my work to see if I'm reading/interpreting the spec the same way you're reading it?

I really don't think it's critical to fix the old backend (contentSecurityPolicy.js) since we're replacing it very soon, but I went ahead and fixed it in both contentSecurityPolicy.js and nsCSPContext.cpp.  

Chris: can you look at this patch and see if it works for you?  Also, I don't think we need tests for this since the changes are really straightforward (skip report-only policies, drop from reports URIs that are cross-origin), but we could probably write some.

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

Comment on attachment 8431939 [details] [diff] [review]
proposed patch

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

Sid, JS looks right, in C++ you got it the wrong way round - easy fix. Please update and r=me.

::: content/base/src/nsCSPContext.cpp
@@ +1041,5 @@
> +
> +    // According to the W3C CSP spec, frame-ancestors checks are ignored for
> +    // report-only policies (when "monitoring").
> +    if (!mPolicies[i]->getReportOnlyFlag()) {
> +      continue;

I think you want to remove the !, so it looks like this:

if (mPolicies[i]->getReportOnlyFlag()) {
  continue;

Why are one-liners always the hardest to get right?

	Devdatta Akhawe [:devd]
	Comment 11 • 10 years ago

Comment on attachment 8431939 [details] [diff] [review]
proposed patch

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

Hey! Yes. This works as I think the spec wants it.

Correct me if I am wrong, but I believe the current patch also disables logging to the console for frame-ancestors violations. This might be a nit, but I think this can affect usability of CSP a bit. I wonder if we can log to the console but not send network reports. For example, asyncReportViolation could have another argument, something like "isSensitive" or "isTainted", and the report won't be sent to the network (or a future imaginary csp-violation JS event). As CSP evolves, I can imagine there being more directives where sending the report would be a violation of same-origin policy and similar report attenuation is needed.

Another nit: I believe the current patch does not include the "ignore XFO if CSP frame ancestors exists." Obviously, that can be a separate bug but I just wanted to put in your radar in case it wasn't already.

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 12 • 10 years ago

Hey devd, thanks for the feedback.  I filed followup bugs for your suggestions since they're slightly tangental.  Both good ideas.
Ignore frame-options: bug 1024557
Local-only violation reports for CSP: bug 1024562

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 13 • 10 years ago

Figured I'd run this through try with bug 805929:
https://tbpl.mozilla.org/?tree=Try&rev=23d220cee0d8

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 14 • 10 years ago

Looks like test_CSP_frameancestors.html fails now because of null URIs (blocked because of cross-origin blockage).  I fixed the tests to catch this exception via try/catch and handle null URIs since it doesn't matter if we have the URI in the violation report.

Looks like this (also in the /csp/ dir):

--- a/content/base/test/xcsp/test_CSP_frameancestors.html	Thu Jun 12 09:45:14 2014 -0700
+++ b/content/base/test/xcsp/test_CSP_frameancestors.html	Thu Jun 12 13:42:52 2014 -0700
@@ -37,17 +37,26 @@ function examiner() {
 examiner.prototype  = {
   observe: function(subject, topic, data) {
     // subject should be an nsURI, and should be either allowed or blocked.
     if (!SpecialPowers.can_QI(subject))
       return;
 
     if (topic === "csp-on-violate-policy") {
       //these were blocked... record that they were blocked
-      var asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");
+
+      var asciiSpec = subject;
+      // Except CSP prohibits cross-origin URI reporting during frame ancestors
+      // checks so this may not be an nsIURI.
+      try {
+        SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");
+      } catch (ex) {
+        // was not an nsIURI, so it was probably a cross-origin report.
+      }
+
       window.frameBlocked(asciiSpec, data);
     }
   },

Pushed to try again.  https://tbpl.mozilla.org/?tree=Try&rev=5bef0f1eaeee

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 15 • 10 years ago

grobinson: ckerschb already r+ed the non-test changes here, but since he's on vacation would you please let me know if my addtional changes mentioned in comment 14 are acceptable (they are only test changes, but I need a sanity check)?

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8439482 [details] [diff] [review]
proposed patch

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

::: content/base/test/csp/test_CSP_frameancestors.html
@@ +46,5 @@
> +
> +      // Except CSP prohibits cross-origin URI reporting during frame ancestors
> +      // checks so this URI could be null.
> +      try {
> +        SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");

augh, there should be an assignment operator in the front there (asciiSpec = Special...).  Won't affect success/failure of the test, but I'll correct this before landing.

::: content/base/test/xcsp/test_CSP_frameancestors.html
@@ +46,5 @@
> +      var asciiSpec = subject;
> +      // Except CSP prohibits cross-origin URI reporting during frame ancestors
> +      // checks so this may not be an nsIURI.
> +      try {
> +        SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");

and here too.

	Garrett Robinson [:grobinson]
	Comment 17 • 10 years ago

Comment on attachment 8439482 [details] [diff] [review]
proposed patch

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

Overall, changes lgtm. The try run from Comment 14 is all green, which concerns me given the lack of assignment that you spotted in Comment 16. How are the tests passing without that assignment?

::: content/base/test/csp/test_CSP_frameancestors.html
@@ +46,5 @@
> +
> +      // Except CSP prohibits cross-origin URI reporting during frame ancestors
> +      // checks so this URI could be null.
> +      try {
> +        SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");

Are the tests passing on try without this assignment??

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 18 • 10 years ago

The tests pass without the assignment because it's a useless assignment when you're not debugging the test.

http://mxr.mozilla.org/mozilla-central/source/content/base/test/csp/test_CSP_frameancestors.html?force=1#76

The URI is not used at all by frameBlocked(), unless you put a dump in there to see what's wrong.  I could modify the tests to not do the assignment at all, but I'm not convinced it matters either way (since someone seeing failures in that test would have to get the nsIURI out of the subject arg if they're trying to debug it.  What do you think, Garrett?

	Garrett Robinson [:grobinson]
	Comment 19 • 10 years ago

Thanks for clarifying. lgtm!

	Sid Stamm [:geekboy or :sstamm] Assignee
	Comment 20 • 10 years ago

Thanks Garrett.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68085964611d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/68085964611d

	Jean-Yves Perrier [:teoli]
	Comment 22 • 9 years ago

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/33
and
https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives