Bug 1954597 - Part 2: Refactor ValidatePrincipal to allow the logic to be used from a content process, r=smaug

This involved moving the logic into ProcessIsolation.cpp, closer to the rest of
the process selection logic. The existing method is preserved, to be used as a
convenience for callers.

As the new logic is closer to the rest of the process selection logic, this
also simplifies the remote type parsing logic to perform direct string compares
against the remoteType's site-origin. If origin keying of processes is added in
the future, this will likely need to be updated to handle that case.

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

docshell/base/BrowsingContextGroup.cpp

@@ -16,6 +16,7 @@
 #include "mozilla/dom/DocGroup.h"
 #include "mozilla/StaticPrefs_dom.h"
 #include "mozilla/ThrottledEventQueue.h"
+#include "mozilla/dom/ProcessIsolation.h"
 #include "nsFocusManager.h"
 #include "nsTHashMap.h"
 
@@ -651,8 +652,13 @@ Maybe<bool> BrowsingContextGroup::UsesOriginAgentCluster(
     return Some(true);
   }
 
-  // NOTE: An in-content equivalent to `ValidatePrincipal`, should probably be
-  // asserted here.
+  // If this assertion fails, we may return `Nothing()` below unexpectedly, as
+  // the parent process may have chosen to not process-switch.
+  MOZ_DIAGNOSTIC_ASSERT(
+      XRE_IsParentProcess() ||
+          ValidatePrincipalCouldPotentiallyBeLoadedBy(
+              aPrincipal, ContentChild::GetSingleton()->GetRemoteType(), {}),
+      "Attempting to create document with unexpected principal");
 
   if (auto entry = mUseOriginAgentCluster.Lookup(aPrincipal)) {
     return Some(entry.Data());

dom/ipc/BrowserParent.cpp

@@ -1410,7 +1410,7 @@ IPCResult BrowserParent::RecvNewWindowGlobal(
 
   // Ensure we never load a document with a content principal in
   // the wrong type of webIsolated process
-  EnumSet<ContentParent::ValidatePrincipalOptions> validationOptions = {};
+  EnumSet<ValidatePrincipalOptions> validationOptions = {};
   nsCOMPtr<nsIURI> docURI = aInit.documentURI();
   if (docURI->SchemeIs("blob") || docURI->SchemeIs("chrome")) {
     // XXXckerschb TODO - Do not use SystemPrincipal for:
@@ -1420,7 +1420,7 @@ IPCResult BrowserParent::RecvNewWindowGlobal(
     //   * chrome://reftest/content/writing-mode/ua-style-sheet-button-1a-ref.html
     //   * chrome://reftest/content/xul-document-load/test003.xhtml
     //   * chrome://reftest/content/forms/input/text/centering-1.xhtml
-    validationOptions = {ContentParent::ValidatePrincipalOptions::AllowSystem};
+    validationOptions = {ValidatePrincipalOptions::AllowSystem};
   }
 
   // Some reftests have frames inside their chrome URIs and those load
@@ -1432,8 +1432,7 @@ IPCResult BrowserParent::RecvNewWindowGlobal(
                       IPC_FAIL(this, "Should have spec for about: URI"));
     if (spec.Equals("about:blank") && wgp &&
         wgp->DocumentPrincipal()->IsSystemPrincipal()) {
-      validationOptions = {
-          ContentParent::ValidatePrincipalOptions::AllowSystem};
+      validationOptions = {ValidatePrincipalOptions::AllowSystem};
     }
   }
 

dom/ipc/ContentParent.cpp

@@ -1259,127 +1259,8 @@ void ContentParent::LogAndAssertFailedPrincipalValidationInfo(
 bool ContentParent::ValidatePrincipal(
     nsIPrincipal* aPrincipal,
     const EnumSet<ValidatePrincipalOptions>& aOptions) {
-  // If the pref says we should not validate, then there is nothing to do
-  if (!StaticPrefs::dom_security_enforceIPCBasedPrincipalVetting()) {
-    return true;
-  }
-
-  // If there is no principal, then there is nothing to validate!
-  if (!aPrincipal) {
-    return aOptions.contains(ValidatePrincipalOptions::AllowNullPtr);
-  }
-
-  // We currently do not track relationships between specific null principals
-  // and content processes, so we can not validate much here - just allow all
-  // null principals we see because they are generally safe anyway!
-  if (aPrincipal->GetIsNullPrincipal()) {
-    return true;
-  }
-
-  // Only allow the system principal if the passed in options flags
-  // request permitting the system principal.
-  if (aPrincipal->IsSystemPrincipal()) {
-    return aOptions.contains(ValidatePrincipalOptions::AllowSystem);
-  }
-
-  // XXXckerschb: we should eliminate the resource carve-out here and always
-  // validate the Principal, see Bug 1686200: Investigate Principal for pdf.js
-  if (aPrincipal->SchemeIs("resource")) {
-    return true;
-  }
-
-  // Validate each inner principal individually, allowing us to catch expanded
-  // principals containing the system principal, etc.
-  if (aPrincipal->GetIsExpandedPrincipal()) {
-    if (!aOptions.contains(ValidatePrincipalOptions::AllowExpanded)) {
-      return false;
-    }
-    // FIXME: There are more constraints on expanded principals in-practice,
-    // such as the structure of extension expanded principals. This may need
-    // to be investigated more in the future.
-    nsCOMPtr<nsIExpandedPrincipal> expandedPrincipal =
-        do_QueryInterface(aPrincipal);
-    const auto& allowList = expandedPrincipal->AllowList();
-    for (const auto& innerPrincipal : allowList) {
-      if (!ValidatePrincipal(innerPrincipal, aOptions)) {
-        return false;
-      }
-    }
-    return true;
-  }
-
-  // A URI with a file:// scheme can never load in a non-file content process
-  // due to sandboxing.
-  if (aPrincipal->SchemeIs("file")) {
-    // If we don't support a separate 'file' process, then we can return here.
-    if (!StaticPrefs::browser_tabs_remote_separateFileUriProcess()) {
-      return true;
-    }
-    return mRemoteType == FILE_REMOTE_TYPE;
-  }
-
-  if (aPrincipal->SchemeIs("about")) {
-    uint32_t flags = 0;
-    nsresult rv = aPrincipal->GetAboutModuleFlags(&flags);
-    if (rv == nsresult::NS_ERROR_FACTORY_NOT_REGISTERED) {
-      // This happens in our tests. There is a race between an about page
-      // getting unregistered and the content process unregistering a blob URL
-      // which it was using.
-      return true;
-    }
-    if (NS_FAILED(rv)) {
-      return false;
-    }
-
-    // Block principals for about: URIs which can't load in this process.
-    if (!(flags & (nsIAboutModule::URI_CAN_LOAD_IN_CHILD |
-                   nsIAboutModule::URI_MUST_LOAD_IN_CHILD))) {
-      return false;
-    }
-    if (flags & nsIAboutModule::URI_MUST_LOAD_IN_EXTENSION_PROCESS) {
-      return mRemoteType == EXTENSION_REMOTE_TYPE;
-    }
-    return true;
-  }
-
-  // Web content can contain extension content frames, so any content process
-  // may send us an extension's principal.
-  // NOTE: We don't check AddonPolicy here, as that could disappear if the
-  // add-on is disabled or uninstalled. As this is a lax check, looking at the
-  // scheme should be sufficient.
-  if (aPrincipal->SchemeIs("moz-extension")) {
-    return true;
-  }
-
-  // If the remote type doesn't have an origin suffix, we can do no further
-  // principal validation with it.
-  int32_t equalIdx = mRemoteType.FindChar('=');
-  if (equalIdx == kNotFound) {
-    return true;
-  }
-
-  // Split out the remote type prefix and the origin suffix.
-  nsDependentCSubstring typePrefix(mRemoteType, 0, equalIdx);
-  nsDependentCSubstring typeOrigin(mRemoteType, equalIdx + 1);
-
-  // Only validate webIsolated remote types for now. This should be expanded in
-  // the future.
-  if (typePrefix != FISSION_WEB_REMOTE_TYPE) {
-    return true;
-  }
-
-  // Trim any OriginAttributes from the origin, as those will not be validated.
-  int32_t suffixIdx = typeOrigin.RFindChar('^');
-  nsDependentCSubstring typeOriginNoSuffix(typeOrigin, 0, suffixIdx);
-
-  // NOTE: Currently every webIsolated remote type is site-origin keyed, meaning
-  // we can unconditionally compare site origins. If this changes in the future,
-  // this logic will need to be updated to reflect that.
-  nsAutoCString siteOriginNoSuffix;
-  if (NS_FAILED(aPrincipal->GetSiteOriginNoSuffix(siteOriginNoSuffix))) {
-    return false;
-  }
-  return siteOriginNoSuffix == typeOriginNoSuffix;
+  return ValidatePrincipalCouldPotentiallyBeLoadedBy(aPrincipal, mRemoteType,
+                                                     aOptions);
 }
 
 /*static*/

dom/ipc/ContentParent.h

@@ -11,6 +11,7 @@
 #include "mozilla/dom/ipc/IdType.h"
 #include "mozilla/dom/MessageManagerCallback.h"
 #include "mozilla/dom/MediaSessionBinding.h"
+#include "mozilla/dom/ProcessIsolation.h"
 #include "mozilla/dom/RemoteBrowser.h"
 #include "mozilla/dom/RemoteType.h"
 #include "mozilla/dom/JSProcessActorParent.h"
@@ -657,11 +658,6 @@ class ContentParent final : public PContentParent,
 
   // Whenever receiving a Principal we need to validate that Principal case
   // by case, where we grant individual callsites to customize the checks!
-  enum class ValidatePrincipalOptions {
-    AllowNullPtr,  // Not a NullPrincipal but a nullptr as Principal.
-    AllowSystem,
-    AllowExpanded,
-  };
   bool ValidatePrincipal(
       nsIPrincipal* aPrincipal,
       const EnumSet<ValidatePrincipalOptions>& aOptions = {});

dom/ipc/ProcessIsolation.cpp

@@ -1149,4 +1149,131 @@ bool IsIsolateHighValueSiteEnabled() {
              WebContentIsolationStrategy::IsolateHighValue;
 }
 
+bool ValidatePrincipalCouldPotentiallyBeLoadedBy(
+    nsIPrincipal* aPrincipal, const nsACString& aRemoteType,
+    const EnumSet<ValidatePrincipalOptions>& aOptions) {
+  // If the pref says we should not validate, then there is nothing to do
+  if (!StaticPrefs::dom_security_enforceIPCBasedPrincipalVetting()) {
+    return true;
+  }
+
+  // Don't bother validating principals from the parent process.
+  if (aRemoteType == NOT_REMOTE_TYPE) {
+    return true;
+  }
+
+  // If there is no principal, only allow it if AllowNullPtr is specified.
+  if (!aPrincipal) {
+    return aOptions.contains(ValidatePrincipalOptions::AllowNullPtr);
+  }
+
+  // We currently do not track relationships between specific null principals
+  // and content processes, so we can not validate much here.
+  if (aPrincipal->GetIsNullPrincipal()) {
+    return true;
+  }
+
+  // If we have a system principal, only allow it if AllowSystem is passed.
+  if (aPrincipal->IsSystemPrincipal()) {
+    return aOptions.contains(ValidatePrincipalOptions::AllowSystem);
+  }
+
+  // We can load a `resource://` URI in any process. This usually comes up due
+  // to pdf.js and the JSON viewer. See bug 1686200.
+  if (aPrincipal->SchemeIs("resource")) {
+    return true;
+  }
+
+  // Only allow expanded principals if AllowExpanded is passed. Each
+  // sub-principal will be validated independently.
+  if (aPrincipal->GetIsExpandedPrincipal()) {
+    if (!aOptions.contains(ValidatePrincipalOptions::AllowExpanded)) {
+      return false;
+    }
+    // FIXME: There are more constraints on expanded principals in-practice,
+    // such as the structure of extension expanded principals. This may need
+    // to be investigated more in the future.
+    nsCOMPtr<nsIExpandedPrincipal> expandedPrincipal =
+        do_QueryInterface(aPrincipal);
+    const auto& allowList = expandedPrincipal->AllowList();
+    for (const auto& innerPrincipal : allowList) {
+      if (!ValidatePrincipalCouldPotentiallyBeLoadedBy(innerPrincipal,
+                                                       aRemoteType, aOptions)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  // A URI with a file:// scheme can never load in a non-file content process
+  // due to sandboxing.
+  if (aPrincipal->SchemeIs("file")) {
+    // If we don't support a separate 'file' process, then we can return here.
+    if (!StaticPrefs::browser_tabs_remote_separateFileUriProcess()) {
+      return true;
+    }
+    return aRemoteType == FILE_REMOTE_TYPE;
+  }
+
+  if (aPrincipal->SchemeIs("about")) {
+    uint32_t flags = 0;
+    nsresult rv = aPrincipal->GetAboutModuleFlags(&flags);
+    // In tests, we can race between about: pages being unregistered, and a
+    // content process unregistering a Blob URL. To be safe here, we fail open
+    // if no about module is present.
+    if (NS_FAILED(rv)) {
+      return false;
+    }
+
+    // Block principals for about: URIs which can't load in this process.
+    if (!(flags & (nsIAboutModule::URI_CAN_LOAD_IN_CHILD |
+                   nsIAboutModule::URI_MUST_LOAD_IN_CHILD))) {
+      return false;
+    }
+    if (flags & nsIAboutModule::URI_MUST_LOAD_IN_EXTENSION_PROCESS) {
+      return aRemoteType == EXTENSION_REMOTE_TYPE;
+    }
+    return true;
+  }
+
+  // Web content can contain extension content frames, so any content process
+  // may send us an extension's principal.
+  // NOTE: We don't check AddonPolicy here, as that can disappear if the add-on
+  // is disabled or uninstalled. As this is a lax check, looking at the scheme
+  // should be sufficient.
+  if (aPrincipal->SchemeIs("moz-extension")) {
+    return true;
+  }
+
+  // If the remote type doesn't have an origin suffix, we can do no further
+  // principal validation with it.
+  int32_t equalIdx = aRemoteType.FindChar('=');
+  if (equalIdx == kNotFound) {
+    return true;
+  }
+
+  // Split out the remote type prefix and the origin suffix.
+  nsDependentCSubstring typePrefix(aRemoteType, 0, equalIdx);
+  nsDependentCSubstring typeOrigin(aRemoteType, equalIdx + 1);
+
+  // Only validate webIsolated remote types for now. This should be expanded in
+  // the future.
+  if (typePrefix != FISSION_WEB_REMOTE_TYPE) {
+    return true;
+  }
+
+  // Trim any OriginAttributes from the origin, as those will not be validated.
+  int32_t suffixIdx = typeOrigin.RFindChar('^');
+  nsDependentCSubstring typeOriginNoSuffix(typeOrigin, 0, suffixIdx);
+
+  // NOTE: Currently every webIsolated remote type is site-origin keyed, meaning
+  // we can unconditionally compare site origins. If this changes in the future,
+  // this logic will need to be updated to reflect that.
+  nsAutoCString siteOriginNoSuffix;
+  if (NS_FAILED(aPrincipal->GetSiteOriginNoSuffix(siteOriginNoSuffix))) {
+    return false;
+  }
+  return siteOriginNoSuffix == typeOriginNoSuffix;
+}
+
 }  // namespace mozilla::dom

dom/ipc/ProcessIsolation.h

@@ -107,6 +107,22 @@ void AddHighValuePermission(const nsACString& aOrigin,
  */
 bool IsIsolateHighValueSiteEnabled();
 
+/**
+ * Perform a lax check that a process with the given RemoteType could
+ * potentially load a Document or run script with the given principal.
+ *
+ * WARNING: This is intentionally a lax check, to avoid false positives in
+ * assertions, and should NOT be used for process isolation decisions.
+ */
+enum class ValidatePrincipalOptions {
+  AllowNullPtr,  // Not a NullPrincipal but a nullptr as Principal.
+  AllowSystem,
+  AllowExpanded,
+};
+bool ValidatePrincipalCouldPotentiallyBeLoadedBy(
+    nsIPrincipal* aPrincipal, const nsACString& aRemoteType,
+    const EnumSet<ValidatePrincipalOptions>& aOptions);
+
 }  // namespace mozilla::dom
 
 #endif

widget/ClipboardWriteRequestParent.cpp

@@ -61,9 +61,9 @@ IPCResult ClipboardWriteRequestParent::RecvSetData(
     const IPCTransferable& aTransferable) {
   if (!mManager->ValidatePrincipal(
           aTransferable.dataPrincipal(),
-          {ContentParent::ValidatePrincipalOptions::AllowNullPtr,
-           ContentParent::ValidatePrincipalOptions::AllowExpanded,
-           ContentParent::ValidatePrincipalOptions::AllowSystem})) {
+          {dom::ValidatePrincipalOptions::AllowNullPtr,
+           dom::ValidatePrincipalOptions::AllowExpanded,
+           dom::ValidatePrincipalOptions::AllowSystem})) {
     ContentParent::LogAndAssertFailedPrincipalValidationInfo(
         aTransferable.dataPrincipal(), __func__);
   }