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 331398 1e631015acc8513043c0a3ad1c4a31b50679f7b5
parent 331397 5410a208a5cf0b473de1787a6c3a5829f2208c11
child 331399 5534087efac3af79ced4efff4c81444e326009f6
push id86236
push usercbook@mozilla.com
push dateFri, 27 Jan 2017 13:15:35 +0000
treeherdermozilla-inbound@7894b49435a1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckerschb, dveditz
bugs1073952
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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>