From 003b30393d3d8b85a902b339384b78d50ce05a73 Mon Sep 17 00:00:00 2001
From: Christoph Kerschbaumer <ckerschb@christophkerschbaumer.com>
Date: Tue, 14 Jan 2020 17:42:18 +0000
Subject: [PATCH] Bug 1607483: Disallow loading http(s) scripts into system
 privileged contexts. r=tjr

Differential Revision: https://phabricator.services.mozilla.com/D58962

--HG--
extra : moz-landing-system : lando
---
 dom/security/nsContentSecurityManager.cpp | 55 ++++++++++++++++-------
 dom/security/nsContentSecurityManager.h   |  2 +-
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp
index f5a14ea886b0..8e6d51150e0a 100644
--- a/dom/security/nsContentSecurityManager.cpp
+++ b/dom/security/nsContentSecurityManager.cpp
@@ -761,32 +761,55 @@ static void DebugDoContentSecurityCheck(nsIChannel* aChannel,
 }
 
 /* static */
-nsresult nsContentSecurityManager::CheckSystemPrincipalLoads(
+nsresult nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext(
     nsIChannel* aChannel) {
-  // Assert that we never use the SystemPrincipal to load remote documents
-  // i.e., HTTP, HTTPS, FTP URLs
+  // Check and assert that we never allow remote documents/scripts (http:,
+  // https:, ...) to load in system privileged contexts.
   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
 
-  // bail out, if we're not loading with a SystemPrincipal
+  // nothing to do here if we are not loading a resource into a
+  // system prvileged context.
   if (!loadInfo->LoadingPrincipal() ||
       !loadInfo->LoadingPrincipal()->IsSystemPrincipal()) {
     return NS_OK;
   }
-  nsContentPolicyType contentPolicyType =
-      loadInfo->GetExternalContentPolicyType();
-  if ((contentPolicyType != nsIContentPolicy::TYPE_DOCUMENT) &&
-      (contentPolicyType != nsIContentPolicy::TYPE_SUBDOCUMENT)) {
-    return NS_OK;
-  }
+
   nsCOMPtr<nsIURI> finalURI;
   NS_GetFinalChannelURI(aChannel, getter_AddRefs(finalURI));
-  // bail out, if URL isn't pointing to remote resource
+
+  // nothing to do here if we are not loading a resource using http:, https:,
+  // etc.
   if (!nsContentUtils::SchemeIs(finalURI, "http") &&
       !nsContentUtils::SchemeIs(finalURI, "https") &&
       !nsContentUtils::SchemeIs(finalURI, "ftp")) {
     return NS_OK;
   }
 
+  nsContentPolicyType contentPolicyType =
+      loadInfo->GetExternalContentPolicyType();
+
+  // We distinguish between 2 cases:
+  // a) remote scripts
+  //    which should never be loaded into system privileged contexts
+  // b) remote documents/frames
+  //    which generally should also never be loaded into system
+  //    privileged contexts but with some exceptions, like e.g. the
+  //    discoverURL.
+  if (contentPolicyType == nsIContentPolicy::TYPE_SCRIPT) {
+    MOZ_LOG(sCSMLog, LogLevel::Warning,
+            ("Do not load remote scripts into system privileged contexts"));
+    MOZ_ASSERT(false,
+               "Do not load remote scripts into system privileged contexts");
+    // Bug 1607673: Do not only assert but cancel the channel and
+    // return NS_ERROR_CONTENT_BLOCKED.
+    return NS_OK;
+  }
+
+  if ((contentPolicyType != nsIContentPolicy::TYPE_DOCUMENT) &&
+      (contentPolicyType != nsIContentPolicy::TYPE_SUBDOCUMENT)) {
+    return NS_OK;
+  }
+
   // FIXME The discovery feature in about:addons uses the SystemPrincpal.
   // We should remove the exception for AMO with bug 1544011.
   // We should remove the exception for Firefox Accounts with bug 1561318.
@@ -831,10 +854,6 @@ nsresult nsContentSecurityManager::CheckSystemPrincipalLoads(
 #endif
   nsAutoCString requestedURL;
   finalURI->GetAsciiSpec(requestedURL);
-  MOZ_LOG(
-      sCSMLog, LogLevel::Verbose,
-      ("SystemPrincipal must not load remote documents. URL: %s", requestedURL)
-          .get());
   if (xpc::AreNonLocalConnectionsDisabled()) {
     bool disallowSystemPrincipalRemoteDocuments = Preferences::GetBool(
         "security.disallow_non_local_systemprincipal_in_tests");
@@ -847,6 +866,10 @@ nsresult nsContentSecurityManager::CheckSystemPrincipalLoads(
     // but other mochitest are exempt from this
     return NS_OK;
   }
+  MOZ_LOG(
+      sCSMLog, LogLevel::Warning,
+      ("SystemPrincipal must not load remote documents. URL: %s", requestedURL)
+          .get());
   MOZ_ASSERT(false, "SystemPrincipal must not load remote documents.");
   aChannel->Cancel(NS_ERROR_CONTENT_BLOCKED);
   return NS_ERROR_CONTENT_BLOCKED;
@@ -878,7 +901,7 @@ nsresult nsContentSecurityManager::doContentSecurityCheck(
     DebugDoContentSecurityCheck(aChannel, loadInfo);
   }
 
-  nsresult rv = CheckSystemPrincipalLoads(aChannel);
+  nsresult rv = CheckAllowLoadInSystemPrivilegedContext(aChannel);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // if dealing with a redirected channel then we have already installed
diff --git a/dom/security/nsContentSecurityManager.h b/dom/security/nsContentSecurityManager.h
index b133ed9feb2f..74f4a766cdc7 100644
--- a/dom/security/nsContentSecurityManager.h
+++ b/dom/security/nsContentSecurityManager.h
@@ -41,7 +41,7 @@ class nsContentSecurityManager : public nsIContentSecurityManager,
  private:
   static nsresult CheckChannel(nsIChannel* aChannel);
   static nsresult CheckFTPSubresourceLoad(nsIChannel* aChannel);
-  static nsresult CheckSystemPrincipalLoads(nsIChannel* aChannel);
+  static nsresult CheckAllowLoadInSystemPrivilegedContext(nsIChannel* aChannel);
 
   virtual ~nsContentSecurityManager() {}
 };