[CSP] implement frame-ancestors per the CSP 1.1 spec

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: imelven, Assigned: geekboy)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, sec-want})

Trunk
mozilla33
dev-doc-complete, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want] [CSP 1.1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 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)
(Reporter)

Updated

6 years ago
Blocks: 493857
No longer depends on: 725490
Summary: [CSP] implement Content Security Policy UI Safety frame-ancestors → [CSP] implement Content Security Policy UI Safety frame-ancestors directive
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.
(Reporter)

Comment 2

6 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
Summary: [CSP] implement Content Security Policy UI Safety frame-ancestors directive → [CSP] implement Content Security Policy UI Safety frame-options directive
(Reporter)

Comment 3

5 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)
Blocks: 968586
Summary: [CSP] implement Content Security Policy UI Safety frame-options directive → [CSP] implement frame-ancestors
(Reporter)

Updated

5 years ago
Summary: [CSP] implement frame-ancestors → [CSP] implement frame-ancestors per the CSP 1.1 spec
(Assignee)

Comment 4

5 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.
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)
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.
(Assignee)

Updated

4 years ago
Component: Security → DOM: Security
(Assignee)

Updated

4 years ago
Depends on: 991972
(Assignee)

Comment 7

4 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
(Assignee)

Comment 8

4 years ago
... bug 663570.  I'll put a note in bug 663570.  Pressed enter too soon.
(Assignee)

Comment 9

4 years ago
Created attachment 8431939 [details] [diff] [review]
proposed patch

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.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #8431939 - Flags: review?(mozilla)
Attachment #8431939 - Flags: feedback?(dev.akhawe+mozilla)
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?
Attachment #8431939 - Flags: review?(mozilla) → review+
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.
Attachment #8431939 - Flags: feedback?(dev.akhawe+mozilla)
(Assignee)

Updated

4 years ago
See Also: → bug 1024557
(Assignee)

Updated

4 years ago
Blocks: 1024562
(Assignee)

Comment 12

4 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
(Assignee)

Comment 13

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

Comment 14

4 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
(Assignee)

Comment 15

4 years ago
Created attachment 8439482 [details] [diff] [review]
proposed patch

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)?
Attachment #8431939 - Attachment is obsolete: true
Attachment #8439482 - Flags: review?(grobinson)
(Assignee)

Comment 16

4 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.
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??
Attachment #8439482 - Flags: review?(grobinson) → review+
(Assignee)

Comment 18

4 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?
Flags: needinfo?(grobinson)
Thanks for clarifying. lgtm!
Flags: needinfo?(grobinson)
(Assignee)

Comment 20

4 years ago
Thanks Garrett.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68085964611d
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/68085964611d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Whiteboard: [sg:want] → [sg:want] [CSP 1.1]
(Assignee)

Updated

4 years ago
Blocks: 999656
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.