Bug 1073952: inherit CSP into iframe sandbox srcdoc r=ckerschb,dveditz
☠☠ backed out by cabc87c3fa0a ☠ ☠
authorFrederik Braun <fbraun+gh@mozilla.com>
Fri, 27 Jan 2017 01:05:00 +0100
changeset 467417 1e631015acc8513043c0a3ad1c4a31b50679f7b5
parent 467416 5410a208a5cf0b473de1787a6c3a5829f2208c11
child 467418 5534087efac3af79ced4efff4c81444e326009f6
push id43165
push userbmo:kmckinley@mozilla.com
push dateFri, 27 Jan 2017 19:43:53 +0000
reviewersckerschb, dveditz
bugs1073952
milestone54.0a1
Bug 1073952: inherit CSP into iframe sandbox srcdoc r=ckerschb,dveditz MozReview-Commit-ID: KTzCLoDfYnd
caps/BasePrincipal.cpp
caps/BasePrincipal.h
caps/nsIPrincipal.idl
caps/nsNullPrincipal.cpp
caps/nsNullPrincipal.h
caps/nsScriptSecurityManager.cpp
caps/nsSystemPrincipal.cpp
caps/nsSystemPrincipal.h
dom/security/test/csp/file_iframe_sandbox_srcdoc.html
dom/security/test/csp/file_iframe_sandbox_srcdoc.html^headers^
dom/security/test/csp/mochitest.ini
dom/security/test/csp/test_iframe_sandbox_srcdoc.html
--- a/caps/BasePrincipal.cpp
+++ b/caps/BasePrincipal.cpp
@@ -429,16 +429,31 @@ BasePrincipal::CheckMayLoad(nsIURI* aURI
 NS_IMETHODIMP
 BasePrincipal::GetCsp(nsIContentSecurityPolicy** aCsp)
 {
   NS_IF_ADDREF(*aCsp = mCSP);
   return NS_OK;
 }
 
 NS_IMETHODIMP
+BasePrincipal::SetCsp(nsIContentSecurityPolicy* aCsp)
+{
+  // Never destroy an existing CSP on the principal.
+  // This method should only be called in rare cases.
+
+  MOZ_ASSERT(!mCSP, "do not destroy an existing CSP");
+  if (mCSP) {
+    return NS_ERROR_ALREADY_INITIALIZED;
+  }
+
+  mCSP = aCsp;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 BasePrincipal::EnsureCSP(nsIDOMDocument* aDocument,
                          nsIContentSecurityPolicy** aCSP)
 {
   if (mCSP) {
     // if there is a CSP already associated with this principal
     // then just return that - do not overwrite it!!!
     NS_IF_ADDREF(*aCSP = mCSP);
     return NS_OK;
--- a/caps/BasePrincipal.h
+++ b/caps/BasePrincipal.h
@@ -207,16 +207,17 @@ public:
   NS_IMETHOD GetOrigin(nsACString& aOrigin) final;
   NS_IMETHOD GetOriginNoSuffix(nsACString& aOrigin) final;
   NS_IMETHOD Equals(nsIPrincipal* other, bool* _retval) final;
   NS_IMETHOD EqualsConsideringDomain(nsIPrincipal* other, bool* _retval) final;
   NS_IMETHOD Subsumes(nsIPrincipal* other, bool* _retval) final;
   NS_IMETHOD SubsumesConsideringDomain(nsIPrincipal* other, bool* _retval) final;
   NS_IMETHOD CheckMayLoad(nsIURI* uri, bool report, bool allowIfInheritsPrincipal) final;
   NS_IMETHOD GetCsp(nsIContentSecurityPolicy** aCsp) override;
+  NS_IMETHOD SetCsp(nsIContentSecurityPolicy* aCsp) override;
   NS_IMETHOD EnsureCSP(nsIDOMDocument* aDocument, nsIContentSecurityPolicy** aCSP) override;
   NS_IMETHOD GetPreloadCsp(nsIContentSecurityPolicy** aPreloadCSP) override;
   NS_IMETHOD EnsurePreloadCSP(nsIDOMDocument* aDocument, nsIContentSecurityPolicy** aCSP) override;
   NS_IMETHOD GetCspJSON(nsAString& outCSPinJSON) override;
   NS_IMETHOD GetIsNullPrincipal(bool* aResult) override;
   NS_IMETHOD GetIsCodebasePrincipal(bool* aResult) override;
   NS_IMETHOD GetIsExpandedPrincipal(bool* aResult) override;
   NS_IMETHOD GetIsSystemPrincipal(bool* aResult) override;
--- a/caps/nsIPrincipal.idl
+++ b/caps/nsIPrincipal.idl
@@ -132,20 +132,21 @@ interface nsIPrincipal : nsISerializable
      *                                   loader.
      * @throws NS_ERROR_DOM_BAD_URI if the load is not allowed.
      */
     void checkMayLoad(in nsIURI uri, in boolean report,
                       in boolean allowIfInheritsPrincipal);
 
     /**
      * A Content Security Policy associated with this principal.
-     *
      * Use this function to query the associated CSP with this principal.
+     * Please *only* use this function to *set* a CSP when you know exactly what you are doing.
+     * Most likely you want to call ensureCSP instead of setCSP.
      */
-    [noscript] readonly attribute nsIContentSecurityPolicy csp;
+    [noscript] attribute nsIContentSecurityPolicy csp;
 
     /*
      * Use this function to query a CSP associated with this principal.
      * If no CSP is associated with this principal then one is created
      * internally and setRequestContext is called on the CSP using aDocument.
      *
      * Please note if aDocument is null, then setRequestContext on the
      * CSP object is called using the current principal.
--- a/caps/nsNullPrincipal.cpp
+++ b/caps/nsNullPrincipal.cpp
@@ -102,16 +102,30 @@ nsNullPrincipal::GetScriptLocation(nsACS
 NS_IMETHODIMP
 nsNullPrincipal::GetHashValue(uint32_t *aResult)
 {
   *aResult = (NS_PTR_TO_INT32(this) >> 2);
   return NS_OK;
 }
 
 NS_IMETHODIMP
+nsNullPrincipal::SetCsp(nsIContentSecurityPolicy* aCsp) {
+  // Never destroy an existing CSP on the principal.
+  // This method should only be called in rare cases.
+
+  MOZ_ASSERT(!mCSP, "do not destroy an existing CSP");
+  if (mCSP) {
+    return NS_ERROR_ALREADY_INITIALIZED;
+  }
+
+  mCSP = aCsp;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 nsNullPrincipal::GetURI(nsIURI** aURI)
 {
   return NS_EnsureSafeToReturn(mURI, aURI);
 }
 
 NS_IMETHODIMP
 nsNullPrincipal::GetDomain(nsIURI** aDomain)
 {
@@ -182,9 +196,8 @@ nsNullPrincipal::Write(nsIObjectOutputSt
   nsAutoCString suffix;
   OriginAttributesRef().CreateSuffix(suffix);
 
   nsresult rv = aStream->WriteStringZ(suffix.get());
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
-
--- a/caps/nsNullPrincipal.h
+++ b/caps/nsNullPrincipal.h
@@ -37,16 +37,17 @@ public:
   // Other consumers should use the Create and CreateWithInheritedAttributes
   // methods.
   nsNullPrincipal() {}
 
   NS_DECL_NSISERIALIZABLE
 
   NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override;
   NS_IMETHOD GetHashValue(uint32_t* aHashValue) override;
+  NS_IMETHOD SetCsp(nsIContentSecurityPolicy* aCsp) override;
   NS_IMETHOD GetURI(nsIURI** aURI) override;
   NS_IMETHOD GetDomain(nsIURI** aDomain) override;
   NS_IMETHOD SetDomain(nsIURI* aDomain) override;
   NS_IMETHOD GetBaseDomain(nsACString& aBaseDomain) override;
   nsresult GetOriginInternal(nsACString& aOrigin) override;
 
   static already_AddRefed<nsNullPrincipal> CreateWithInheritedAttributes(nsIPrincipal* aInheritFrom);
 
--- a/caps/nsScriptSecurityManager.cpp
+++ b/caps/nsScriptSecurityManager.cpp
@@ -298,16 +298,40 @@ nsScriptSecurityManager::GetChannelResul
               prin =
                 nsNullPrincipal::CreateWithInheritedAttributes(loadInfo->LoadingPrincipal());
             } else {
               OriginAttributes attrs;
               loadInfo->GetOriginAttributes(&attrs);
               attrs.StripAttributes(OriginAttributes::STRIP_ADDON_ID);
               prin = nsNullPrincipal::Create(attrs);
             }
+            // if the new NullPrincipal (above) loads an iframe[srcdoc], we
+            // need to inherit an existing CSP to avoid bypasses (bug 1073952).
+            // We continue inheriting for nested frames with e.g., data: URLs.
+            if (loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_SUBDOCUMENT) {
+              nsCOMPtr<nsIURI> uri;
+              aChannel->GetURI(getter_AddRefs(uri));
+              nsAutoCString URISpec;
+              uri->GetSpec(URISpec);
+              bool isData = (NS_SUCCEEDED(uri->SchemeIs("data", &isData)) && isData);
+              if (URISpec.EqualsLiteral("about:srcdoc") || isData) {
+                nsCOMPtr<nsIPrincipal> principalToInherit = loadInfo->PrincipalToInherit();
+                if (!principalToInherit) {
+                  principalToInherit = loadInfo->TriggeringPrincipal();
+                }
+                nsCOMPtr<nsIContentSecurityPolicy> originalCsp;
+                principalToInherit->GetCsp(getter_AddRefs(originalCsp));
+                // if the principalToInherit had a CSP,
+                // add it to the newly created NullPrincipal.
+                if (originalCsp) {
+                  nsresult rv = prin->SetCsp(originalCsp);
+                  NS_ENSURE_SUCCESS(rv, rv);
+                }
+              }
+            }
             prin.forget(aPrincipal);
             return NS_OK;
         }
 
         bool forceInherit = loadInfo->GetForceInheritPrincipal();
         if (aIgnoreSandboxing && !forceInherit) {
           // Check if SEC_FORCE_INHERIT_PRINCIPAL was dropped because of
           // sandboxing:
@@ -330,17 +354,17 @@ nsScriptSecurityManager::GetChannelResul
         // not to loads that it might have redirected to.
         if (loadInfo->RedirectChain().IsEmpty() &&
             (securityFlags == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS ||
              securityFlags == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS ||
              securityFlags == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS)) {
 
             nsCOMPtr<nsIURI> uri;
             nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
-            NS_ENSURE_SUCCESS(rv, rv); 
+            NS_ENSURE_SUCCESS(rv, rv);
             nsCOMPtr<nsIPrincipal> principalToInherit = loadInfo->PrincipalToInherit();
             if (!principalToInherit) {
               principalToInherit = loadInfo->TriggeringPrincipal();
             }
             bool inheritForAboutBlank = loadInfo->GetAboutBlankInherits();
 
             if (nsContentUtils::ChannelShouldInheritPrincipal(principalToInherit,
                                                               uri,
--- a/caps/nsSystemPrincipal.cpp
+++ b/caps/nsSystemPrincipal.cpp
@@ -44,17 +44,17 @@ nsSystemPrincipal::GetScriptLocation(nsA
 
 NS_IMETHODIMP
 nsSystemPrincipal::GetHashValue(uint32_t *result)
 {
     *result = NS_PTR_TO_INT32(this);
     return NS_OK;
 }
 
-NS_IMETHODIMP 
+NS_IMETHODIMP
 nsSystemPrincipal::GetURI(nsIURI** aURI)
 {
     *aURI = nullptr;
     return NS_OK;
 }
 
 nsresult
 nsSystemPrincipal::GetOriginInternal(nsACString& aOrigin)
@@ -66,16 +66,25 @@ nsSystemPrincipal::GetOriginInternal(nsA
 NS_IMETHODIMP
 nsSystemPrincipal::GetCsp(nsIContentSecurityPolicy** aCsp)
 {
   *aCsp = nullptr;
   return NS_OK;
 }
 
 NS_IMETHODIMP
+nsSystemPrincipal::SetCsp(nsIContentSecurityPolicy* aCsp)
+{
+  // Never destroy an existing CSP on the principal.
+  // This method should only be called in rare cases.
+
+  return NS_ERROR_FAILURE;
+}
+
+NS_IMETHODIMP
 nsSystemPrincipal::EnsureCSP(nsIDOMDocument* aDocument,
                              nsIContentSecurityPolicy** aCSP)
 {
   // CSP on a system principal makes no sense
   return NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
--- a/caps/nsSystemPrincipal.h
+++ b/caps/nsSystemPrincipal.h
@@ -25,16 +25,17 @@ class nsSystemPrincipal final : public m
 public:
   NS_DECL_NSISERIALIZABLE
   NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override;
   NS_IMETHOD GetHashValue(uint32_t* aHashValue) override;
   NS_IMETHOD GetURI(nsIURI** aURI) override;
   NS_IMETHOD GetDomain(nsIURI** aDomain) override;
   NS_IMETHOD SetDomain(nsIURI* aDomain) override;
   NS_IMETHOD GetCsp(nsIContentSecurityPolicy** aCsp) override;
+  NS_IMETHOD SetCsp(nsIContentSecurityPolicy* aCsp) override;
   NS_IMETHOD EnsureCSP(nsIDOMDocument* aDocument, nsIContentSecurityPolicy** aCSP) override;
   NS_IMETHOD GetPreloadCsp(nsIContentSecurityPolicy** aPreloadCSP) override;
   NS_IMETHOD EnsurePreloadCSP(nsIDOMDocument* aDocument, nsIContentSecurityPolicy** aCSP) override;
   NS_IMETHOD GetBaseDomain(nsACString& aBaseDomain) override;
   nsresult GetOriginInternal(nsACString& aOrigin) override;
 
   nsSystemPrincipal() {}
 
new file mode 100644
--- /dev/null
+++ b/dom/security/test/csp/file_iframe_sandbox_srcdoc.html
@@ -0,0 +1,11 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Bug 1073952 - CSP should restrict scripts in srcdoc iframe even if sandboxed</title>
+</head>
+<body>
+<iframe srcdoc="<img src=x onerror='parent.postMessage({result: `unexpected-csp-violation`}, `*`);'>"
+        sandbox="allow-scripts"></iframe>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/dom/security/test/csp/file_iframe_sandbox_srcdoc.html^headers^
@@ -0,0 +1,1 @@
+content-security-policy: default-src *;
--- a/dom/security/test/csp/mochitest.ini
+++ b/dom/security/test/csp/mochitest.ini
@@ -198,16 +198,18 @@ support-files =
   file_strict_dynamic_parser_inserted_doc_write.html
   file_strict_dynamic_parser_inserted_doc_write_correct_nonce.html
   file_strict_dynamic_non_parser_inserted.html
   file_strict_dynamic_non_parser_inserted_inline.html
   file_strict_dynamic_unsafe_eval.html
   file_strict_dynamic_default_src.html
   file_strict_dynamic_default_src.js
   file_upgrade_insecure_navigation.sjs
+  file_iframe_sandbox_srcdoc.html
+  file_iframe_sandbox_srcdoc.html^headers^
 
 [test_base-uri.html]
 [test_blob_data_schemes.html]
 [test_connect-src.html]
 [test_CSP.html]
 [test_allow_https_schemes.html]
 [test_bug663567.html]
 [test_bug802872.html]
@@ -285,8 +287,9 @@ tags = mcb
 [test_sendbeacon.html]
 [test_upgrade_insecure_docwrite_iframe.html]
 [test_bug1242019.html]
 [test_bug1312272.html]
 [test_strict_dynamic.html]
 [test_strict_dynamic_parser_inserted.html]
 [test_strict_dynamic_default_src.html]
 [test_upgrade_insecure_navigation.html]
+[test_iframe_sandbox_srcdoc.html]
new file mode 100644
--- /dev/null
+++ b/dom/security/test/csp/test_iframe_sandbox_srcdoc.html
@@ -0,0 +1,62 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Bug 1073952 - CSP should restrict scripts in srcdoc iframe even if sandboxed</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display">Bug 1073952</p>
+<iframe style="width:200px;height:200px;" id='cspframe'></iframe>
+<script class="testbody" type="text/javascript">
+
+// This is used to watch the blocked data bounce off CSP and allowed data
+// get sent out to the wire.
+function examiner() {
+  SpecialPowers.addObserver(this, "csp-on-violate-policy", false);
+}
+
+examiner.prototype  = {
+  observe: function(subject, topic, data) {
+
+    if(topic === "csp-on-violate-policy") {
+      var violationString = SpecialPowers.getPrivilegedProps(SpecialPowers.
+                             do_QueryInterface(subject, "nsISupportsCString"), "data");
+      // the violation subject for inline script violations is unfortunately vague,
+      // all we can do is match the string.
+      if (!violationString.includes("Inline Script")) {
+        return
+      }
+      ok(true, "CSP inherited into sandboxed srcdoc iframe, script blocked.");
+      window.finish();
+    }
+  },
+
+  // must eventually call this to remove the listener,
+  // or mochitests might get borked.
+  remove: function() {
+    SpecialPowers.removeObserver(this, "csp-on-violate-policy");
+  }
+}
+
+window.examiner = new examiner();
+
+function finish() {
+  window.examiner.remove();
+  SimpleTest.finish();
+}
+
+addEventListener("message", function(e) {
+  ok(false, "We should not execute JS in srcdoc iframe.");
+  window.finish();
+})
+SimpleTest.waitForExplicitFinish();
+
+// save this for last so that our listeners are registered.
+// ... this loads the testbed of good and bad requests.
+document.getElementById('cspframe').src = 'file_iframe_sandbox_srcdoc.html';
+
+</script>
+</body>
+</html>