Bug 846978 - disable frame-ancestors checks when CSP is report-only and fix cross-origin frame-ancestors violation URI leak. r=ckerschb,grobinson

content/base/src/contentSecurityPolicy.js

@@ -638,7 +638,7 @@ ContentSecurityPolicy.prototype = {
         break;
     }
     var violationMessage = null;
-    if (blockedUri["asciiSpec"]) {
+    if (blockedUri && blockedUri["asciiSpec"]) {
       let localizeString = policy._reportOnlyMode ? "CSPROViolationWithURI" : "CSPViolationWithURI";
       violationMessage = CSPLocalizer.getFormatStr(localizeString, [violatedDirective, blockedUri.asciiSpec]);
     } else {
@@ -666,6 +666,10 @@ ContentSecurityPolicy.prototype = {
     // to be triggered if any policy wants it.
     var permitted = true;
     for (let i = 0; i < this._policies.length; i++) {
+      // spec says don't check the policies that are report-only (monitored)
+      if (this._policies[i]._reportOnlyMode) {
+        continue;
+      }
       if (!this._permitsAncestryInternal(docShell, this._policies[i], i)) {
         permitted = false;
       }
@@ -715,7 +719,20 @@ ContentSecurityPolicy.prototype = {
         let directive = policy._directives[cspContext];
         let violatedPolicy = 'frame-ancestors ' + directive.toString();
 
-        this._asyncReportViolation(ancestors[i], null, violatedPolicy,
+        // spec says don't report ancestors for cross-origin violations (it is
+        // a violation of same-origin)
+        let ssm = Services.scriptSecurityManager;
+        let blockedURI = null;
+        try {
+          if (Services.scriptSecurityManager
+                      .checkSameOriginURI(ancestors[i], this._requestOrigin, false)) {
+            blockedURI = ancestors[i];
+          }
+        } catch (ex) {
+          // cross-origin, don't send the ancestor
+        }
+
+        this._asyncReportViolation(blockedURI, null, violatedPolicy,
                                    policyIndex);
 
         // need to lie if we are testing in report-only mode

content/base/src/nsCSPContext.cpp

@@ -639,6 +639,8 @@ nsCSPContext::SendReports(nsISupports* aBlockedContentSource,
     csp_report.AppendASCII(reportBlockedURI.get());
   }
   else {
+    // this can happen for frame-ancestors violation where the violating
+    // ancestor is cross-origin.
     NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report.");
   }
   csp_report.AppendASCII("\", ");
@@ -1036,6 +1038,13 @@ nsCSPContext::PermitsAncestry(nsIDocShell* aDocShell, bool* outPermitsAncestry)
   // Now that we've got the ancestry chain in ancestorsArray, time to check
   // them against any CSP.
   for (uint32_t i = 0; i < mPolicies.Length(); i++) {
+
+    // According to the W3C CSP spec, frame-ancestors checks are ignored for
+    // report-only policies (when "monitoring").
+    if (mPolicies[i]->getReportOnlyFlag()) {
+      continue;
+    }
+
     for (uint32_t a = 0; a < ancestorsArray.Length(); a++) {
       // TODO(sid) the mapping from frame-ancestors context to TYPE_DOCUMENT is
       // forced. while this works for now, we will implement something in
@@ -1052,7 +1061,11 @@ nsCSPContext::PermitsAncestry(nsIDocShell* aDocShell, bool* outPermitsAncestry)
                                  EmptyString(), // no nonce
                                  violatedDirective)) {
         // Policy is violated
-        this->AsyncReportViolation(ancestorsArray[a],
+        // Send reports, but omit the ancestor URI if cross-origin as per spec
+        // (it is a violation of the same-origin policy).
+        bool okToSendAncestor = NS_SecurityCompareURIs(ancestorsArray[a], mSelfURI, true);
+
+        this->AsyncReportViolation((okToSendAncestor ? ancestorsArray[a] : nullptr),
                                    mSelfURI,
                                    violatedDirective,
                                    i,             /* policy index        */
@@ -1060,9 +1073,7 @@ nsCSPContext::PermitsAncestry(nsIDocShell* aDocShell, bool* outPermitsAncestry)
                                    EmptyString(), /* no source file      */
                                    EmptyString(), /* no script sample    */
                                    0);            /* no line number      */
-        if (!mPolicies[i]->getReportOnlyFlag()) {
+        *outPermitsAncestry = false;
-          *outPermitsAncestry = false;
-        }
       }
     }
   }

content/base/test/csp/test_CSP_frameancestors.html

@@ -42,7 +42,16 @@ examiner.prototype  = {
 
     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 URI could be null.
+      try {
+        asciiSpec = 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);
     }
   },

content/base/test/xcsp/test_CSP_frameancestors.html

@@ -42,7 +42,16 @@ examiner.prototype  = {
 
     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 {
+        asciiSpec = 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);
     }
   },