Backed out changeset 3d924e2a2e54 (bug 1607483) for assertion failures on nsContentSecurityManager.cpp . CLOSED TREE

dom/security/nsContentSecurityManager.cpp

@@ -761,55 +761,32 @@ static void DebugDoContentSecurityCheck(nsIChannel* aChannel,
 }
 
 /* static */
-nsresult nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext(
+nsresult nsContentSecurityManager::CheckSystemPrincipalLoads(
     nsIChannel* aChannel) {
-  // Check and assert that we never allow remote documents/scripts (http:,
+  // Assert that we never use the SystemPrincipal to load remote documents
-  // https:, ...) to load in system privileged contexts.
+  // i.e., HTTP, HTTPS, FTP URLs
   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
 
-  // nothing to do here if we are not loading a resource into a
+  // bail out, if we're not loading with a SystemPrincipal
-  // 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.
@@ -854,6 +831,10 @@ nsresult nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext(
 #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");
@@ -866,10 +847,6 @@ nsresult nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext(
     // 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;
@@ -901,7 +878,7 @@ nsresult nsContentSecurityManager::doContentSecurityCheck(
     DebugDoContentSecurityCheck(aChannel, loadInfo);
   }
 
-  nsresult rv = CheckAllowLoadInSystemPrivilegedContext(aChannel);
+  nsresult rv = CheckSystemPrincipalLoads(aChannel);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // if dealing with a redirected channel then we have already installed

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 CheckAllowLoadInSystemPrivilegedContext(nsIChannel* aChannel);
+  static nsresult CheckSystemPrincipalLoads(nsIChannel* aChannel);
 
   virtual ~nsContentSecurityManager() {}
 };