Bug 1855992 - remove about: specialcase from principal validation in …

…BrowserParent, r=nika,necko-reviewers,jesup,perftest-reviewers Differential Revision: https://phabricator.services.mozilla.com/D189923
dothq · Nov 7, 2023 · f398697 · f398697
1 parent d2f4714
commit f398697
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 15 deletions.
diff --git a/browser/base/content/test/tabs/browser_e10s_about_page_triggeringprincipal.js b/browser/base/content/test/tabs/browser_e10s_about_page_triggeringprincipal.js
@@ -8,7 +8,9 @@ const kAboutPagesRegistered = Promise.all([
     registerCleanupFunction,
     "test-about-principal-child",
     kChildPage,
-    Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD | Ci.nsIAboutModule.ALLOW_SCRIPT
+    Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD |
+      Ci.nsIAboutModule.ALLOW_SCRIPT |
+      Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT
   ),
   BrowserTestUtils.registerAboutPage(
     registerCleanupFunction,

diff --git a/docshell/base/nsAboutRedirector.cpp b/docshell/base/nsAboutRedirector.cpp
@@ -197,6 +197,7 @@ static const RedirEntry kRedirMap[] = {
     {"crashparent", "about:blank", nsIAboutModule::HIDE_FROM_ABOUTABOUT},
     {"crashcontent", "about:blank",
      nsIAboutModule::HIDE_FROM_ABOUTABOUT |
+         nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
          nsIAboutModule::URI_CAN_LOAD_IN_CHILD |
          nsIAboutModule::URI_MUST_LOAD_IN_CHILD},
     {"crashgpu", "about:blank", nsIAboutModule::HIDE_FROM_ABOUTABOUT},
@@ -221,7 +222,8 @@ nsAboutRedirector::NewChannel(nsIURI* aURI, nsILoadInfo* aLoadInfo,
       path.EqualsASCII("crashgpu") || path.EqualsASCII("crashextensions")) {
     bool isExternal;
     aLoadInfo->GetLoadTriggeredFromExternal(&isExternal);
-    if (isExternal) {
+    if (isExternal || !aLoadInfo->TriggeringPrincipal() ||
+        !aLoadInfo->TriggeringPrincipal()->IsSystemPrincipal()) {
       return NS_ERROR_NOT_AVAILABLE;
     }
 

diff --git a/dom/ipc/BrowserParent.cpp b/dom/ipc/BrowserParent.cpp
@@ -1353,10 +1353,8 @@ IPCResult BrowserParent::RecvNewWindowGlobal(
   // the wrong type of webIsolated process
   EnumSet<ContentParent::ValidatePrincipalOptions> validationOptions = {};
   nsCOMPtr<nsIURI> docURI = aInit.documentURI();
-  if (docURI->SchemeIs("about") || docURI->SchemeIs("blob") ||
-      docURI->SchemeIs("chrome")) {
+  if (docURI->SchemeIs("blob") || docURI->SchemeIs("chrome")) {
     // XXXckerschb TODO - Do not use SystemPrincipal for:
-    // Bug 1700639: about:plugins
     // Bug 1699385: Remove allowSystem for blobs
     // Bug 1698087: chrome://devtools/content/shared/webextension-fallback.html
     // chrome reftests, e.g.
@@ -1366,6 +1364,20 @@ IPCResult BrowserParent::RecvNewWindowGlobal(
     validationOptions = {ContentParent::ValidatePrincipalOptions::AllowSystem};
   }
 
+  // Some reftests have frames inside their chrome URIs and those load
+  // about:blank:
+  if (xpc::IsInAutomation() && docURI->SchemeIs("about")) {
+    WindowGlobalParent* wgp = browsingContext->GetParentWindowContext();
+    nsAutoCString spec;
+    NS_ENSURE_SUCCESS(docURI->GetSpec(spec),
+                      IPC_FAIL(this, "Should have spec for about: URI"));
+    if (spec.Equals("about:blank") && wgp &&
+        wgp->DocumentPrincipal()->IsSystemPrincipal()) {
+      validationOptions = {
+          ContentParent::ValidatePrincipalOptions::AllowSystem};
+    }
+  }
+
   if (!mManager->ValidatePrincipal(aInit.principal(), validationOptions)) {
     ContentParent::LogAndAssertFailedPrincipalValidationInfo(aInit.principal(),
                                                              __func__);

diff --git a/netwerk/protocol/about/nsAboutProtocolHandler.cpp b/netwerk/protocol/about/nsAboutProtocolHandler.cpp
@@ -29,14 +29,6 @@ namespace net {
 
 static NS_DEFINE_CID(kNestedAboutURICID, NS_NESTEDABOUTURI_CID);
 
-static bool IsSafeForUntrustedContent(nsIAboutModule* aModule, nsIURI* aURI) {
-  uint32_t flags;
-  nsresult rv = aModule->GetURIFlags(aURI, &flags);
-  NS_ENSURE_SUCCESS(rv, false);
-
-  return (flags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) != 0;
-}
-
 static bool IsSafeToLinkForUntrustedContent(nsIURI* aURI) {
   nsAutoCString path;
   aURI->GetPathQueryRef(path);
@@ -168,6 +160,23 @@ nsAboutProtocolHandler::NewChannel(nsIURI* uri, nsILoadInfo* aLoadInfo,
   }
 
   if (NS_SUCCEEDED(rv)) {
+    uint32_t flags = 0;
+    rv2 = aboutMod->GetURIFlags(uri, &flags);
+    if (NS_FAILED(rv2)) {
+      return NS_ERROR_FAILURE;
+    }
+
+    bool safeForUntrustedContent =
+        (flags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) != 0;
+
+    MOZ_DIAGNOSTIC_ASSERT(
+        safeForUntrustedContent ||
+            (flags & (nsIAboutModule::URI_CAN_LOAD_IN_CHILD |
+                      nsIAboutModule::URI_MUST_LOAD_IN_CHILD)) == 0,
+        "Only unprivileged content should be loaded in child processes. (Did "
+        "you forget to add URI_SAFE_FOR_UNTRUSTED_CONTENT to your about: "
+        "page?)");
+
     // The standard return case:
     rv = aboutMod->NewChannel(uri, aLoadInfo, result);
     if (NS_SUCCEEDED(rv)) {
@@ -195,7 +204,7 @@ nsAboutProtocolHandler::NewChannel(nsIURI* uri, nsILoadInfo* aLoadInfo,
       // owner to null.
       // Note: this relies on aboutMod's newChannel implementation
       // having set the proper originalURI, which probably isn't ideal.
-      if (IsSafeForUntrustedContent(aboutMod, uri)) {
+      if (safeForUntrustedContent) {
         (*result)->SetOwner(nullptr);
       }
 

diff --git a/testing/talos/talos/tests/tabswitch/content/tabswitch-content-process.js b/testing/talos/talos/tests/tabswitch/content/tabswitch-content-process.js
@@ -30,7 +30,8 @@ const TPSProcessScript = {
       getURIFlags(aURI) {
         return (
           Ci.nsIAboutModule.ALLOW_SCRIPT |
-          Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD
+          Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD |
+          Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT
         );
       }
     }