Bug 1638711 - Do document security checks in parent process. r=ckerschb
☠☠ backed out by 1caff03435d2 ☠ ☠
authorMatt Woodrow <mwoodrow@mozilla.com>
Wed, 27 May 2020 00:32:05 +0000
changeset 532330 164b842bea998ca4609f5fa7ba880c3a6163a22a
parent 532329 c9e6c7008ec907f4f86237df0757f2cb307f0190
child 532331 bebe76fca022c9aa4c02ab8829dba59c2301f5a9
push id37454
push userccoroiu@mozilla.com
push dateWed, 27 May 2020 16:14:31 +0000
treeherdermozilla-central@a1dd9afbfdf5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckerschb
bugs1638711
milestone78.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 1638711 - Do document security checks in parent process. r=ckerschb Differential Revision: https://phabricator.services.mozilla.com/D75720
dom/security/nsCSPService.cpp
dom/security/nsMixedContentBlocker.cpp
netwerk/base/LoadInfo.cpp
netwerk/ipc/DocumentChannelChild.cpp
--- a/dom/security/nsCSPService.cpp
+++ b/dom/security/nsCSPService.cpp
@@ -15,16 +15,17 @@
 #include "nsIContentSecurityPolicy.h"
 #include "nsError.h"
 #include "nsIAsyncVerifyRedirectCallback.h"
 #include "nsAsyncRedirectVerifyHelper.h"
 #include "nsContentUtils.h"
 #include "nsContentPolicyUtils.h"
 #include "nsNetUtil.h"
 #include "mozilla/net/DocumentLoadListener.h"
+#include "mozilla/net/DocumentChannel.h"
 
 using namespace mozilla;
 
 static LazyLogModule gCspPRLog("CSP");
 
 CSPService::CSPService() = default;
 
 CSPService::~CSPService() = default;
@@ -246,16 +247,26 @@ CSPService::AsyncOnChannelRedirect(nsICh
     // DocumentLoadListener, since these are fully supported and we don't
     // expose the redirects to the content process. We can't do this for all
     // request types yet because we don't serialize nsICSPEventListener.
     if (parentChannel && !docListener) {
       return NS_OK;
     }
   }
 
+  // Don't do these checks if we're switching from DocumentChannel
+  // to a real channel. In that case, we should already have done
+  // the checks in the parent process. AsyncOnChannelRedirect
+  // isn't called in the content process if we switch process,
+  // so checking here would just hide bugs in the process switch
+  // cases.
+  if (RefPtr<net::DocumentChannel> docChannel = do_QueryObject(oldChannel)) {
+    return NS_OK;
+  }
+
   nsCOMPtr<nsIURI> newUri;
   nsresult rv = newChannel->GetURI(getter_AddRefs(newUri));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsILoadInfo> loadInfo = oldChannel->LoadInfo();
 
   /* Since redirecting channels don't call into nsIContentPolicy, we call our
    * Content Policy implementation directly when redirects occur using the
--- a/dom/security/nsMixedContentBlocker.cpp
+++ b/dom/security/nsMixedContentBlocker.cpp
@@ -41,16 +41,17 @@
 #include "mozilla/StaticPrefs_dom.h"
 #include "mozilla/StaticPrefs_fission.h"
 #include "mozilla/StaticPrefs_security.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/ipc/URIUtils.h"
 #include "mozilla/net/DNS.h"
 #include "mozilla/net/DocumentLoadListener.h"
+#include "mozilla/net/DocumentChannel.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 enum nsMixedContentBlockerMessageType { eBlocked = 0x00, eUserOverride = 0x01 };
 
 // Whitelist of hostnames that should be considered secure contexts even when
 // served over http:// or ws://
@@ -138,16 +139,26 @@ nsMixedContentBlocker::AsyncOnChannelRed
   nsCOMPtr<nsIParentChannel> is_ipc_channel;
   NS_QueryNotificationCallbacks(aNewChannel, is_ipc_channel);
   RefPtr<net::DocumentLoadListener> docListener =
       do_QueryObject(is_ipc_channel);
   if (is_ipc_channel && !docListener) {
     return NS_OK;
   }
 
+  // Don't do these checks if we're switching from DocumentChannel
+  // to a real channel. In that case, we should already have done
+  // the checks in the parent process. AsyncOnChannelRedirect
+  // isn't called in the content process if we switch process,
+  // so checking here would just hide bugs in the process switch
+  // cases.
+  if (RefPtr<net::DocumentChannel> docChannel = do_QueryObject(aOldChannel)) {
+    return NS_OK;
+  }
+
   nsresult rv;
   nsCOMPtr<nsIURI> oldUri;
   rv = aOldChannel->GetURI(getter_AddRefs(oldUri));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIURI> newUri;
   rv = aNewChannel->GetURI(getter_AddRefs(newUri));
   NS_ENSURE_SUCCESS(rv, rv);
--- a/netwerk/base/LoadInfo.cpp
+++ b/netwerk/base/LoadInfo.cpp
@@ -565,20 +565,17 @@ LoadInfo::LoadInfo(dom::CanonicalBrowsin
       mForceInheritPrincipalDropped(false),
       mInnerWindowID(0),
       mOuterWindowID(0),
       mParentOuterWindowID(0),
       mTopOuterWindowID(0),
       mFrameOuterWindowID(aFrameOuterWindowID),
       mBrowsingContextID(0),
       mFrameBrowsingContextID(0),
-      // annyG: we are mimicking the old LoadInfo since it has gone through
-      // security checks in the content and we wouldn't reach this point
-      // if the load got blocked earlier.
-      mInitialSecurityCheckDone(true),
+      mInitialSecurityCheckDone(false),
       mIsThirdPartyContext(false),
       mIsThirdPartyContextToTopWindow(true),
       mIsFormSubmission(false),
       mSendCSPViolationEvents(true),
       mRequestBlockingReason(BLOCKING_REASON_NONE),
       mForcePreflight(false),
       mIsPreflight(false),
       mLoadTriggeredFromExternal(false),
--- a/netwerk/ipc/DocumentChannelChild.cpp
+++ b/netwerk/ipc/DocumentChannelChild.cpp
@@ -41,18 +41,16 @@ DocumentChannelChild::~DocumentChannelCh
   LOG(("DocumentChannelChild dtor [this=%p]", this));
 }
 
 NS_IMETHODIMP
 DocumentChannelChild::AsyncOpen(nsIStreamListener* aListener) {
   nsresult rv = NS_OK;
 
   nsCOMPtr<nsIStreamListener> listener = aListener;
-  rv = nsContentSecurityManager::doContentSecurityCheck(this, listener);
-  NS_ENSURE_SUCCESS(rv, rv);
 
   NS_ENSURE_TRUE(gNeckoChild, NS_ERROR_FAILURE);
   NS_ENSURE_ARG_POINTER(listener);
   NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
   NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED);
 
   // Port checked in parent, but duplicate here so we can return with error
   // immediately, as we've done since before e10s.