From 224b3b9b00c2486a1a694a7a21bc33b4d4f880a7 Mon Sep 17 00:00:00 2001 From: Jonas Sicking Date: Sat, 5 Dec 2015 01:46:20 -0800 Subject: [PATCH] Bug 1226909 part 1: Do security checks in a redirect handler rather than when opening the redirected channel. r=ckerschb --- caps/nsScriptSecurityManager.cpp | 36 ----- caps/nsScriptSecurityManager.h | 3 - dom/security/nsContentSecurityManager.cpp | 126 +++++++++++++----- dom/security/nsContentSecurityManager.h | 5 + .../test/csp/file_redirects_main.html | 1 - dom/security/test/csp/file_redirects_page.sjs | 13 +- .../test/csp/file_redirects_resource.sjs | 7 - dom/security/test/csp/test_redirects.html | 8 +- layout/build/nsLayoutModule.cpp | 1 - netwerk/base/nsIOService.cpp | 6 +- netwerk/build/nsNetCID.h | 9 -- 11 files changed, 109 insertions(+), 106 deletions(-) diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp index 664278f6453..0fbc68c7c45 100644 --- a/caps/nsScriptSecurityManager.cpp +++ b/caps/nsScriptSecurityManager.cpp @@ -431,7 +431,6 @@ nsScriptSecurityManager::IsSystemPrincipal(nsIPrincipal* aPrincipal, //////////////////////////////////// NS_IMPL_ISUPPORTS(nsScriptSecurityManager, nsIScriptSecurityManager, - nsIChannelEventSink, nsIObserver) /////////////////////////////////////////////////// @@ -1236,41 +1235,6 @@ nsScriptSecurityManager::CanGetService(JSContext *cx, return NS_ERROR_DOM_XPCONNECT_ACCESS_DENIED; } -///////////////////////////////////////////// -// Method implementing nsIChannelEventSink // -///////////////////////////////////////////// -NS_IMETHODIMP -nsScriptSecurityManager::AsyncOnChannelRedirect(nsIChannel* oldChannel, - nsIChannel* newChannel, - uint32_t redirFlags, - nsIAsyncVerifyRedirectCallback *cb) -{ - nsCOMPtr oldPrincipal; - GetChannelResultPrincipal(oldChannel, getter_AddRefs(oldPrincipal)); - - nsCOMPtr newURI; - newChannel->GetURI(getter_AddRefs(newURI)); - nsCOMPtr newOriginalURI; - newChannel->GetOriginalURI(getter_AddRefs(newOriginalURI)); - - NS_ENSURE_STATE(oldPrincipal && newURI && newOriginalURI); - - const uint32_t flags = - nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT | - nsIScriptSecurityManager::DISALLOW_SCRIPT; - nsresult rv = CheckLoadURIWithPrincipal(oldPrincipal, newURI, flags); - if (NS_SUCCEEDED(rv) && newOriginalURI != newURI) { - rv = CheckLoadURIWithPrincipal(oldPrincipal, newOriginalURI, flags); - } - - if (NS_FAILED(rv)) - return rv; - - cb->OnRedirectVerifyCallback(NS_OK); - return NS_OK; -} - - ///////////////////////////////////// // Method implementing nsIObserver // ///////////////////////////////////// diff --git a/caps/nsScriptSecurityManager.h b/caps/nsScriptSecurityManager.h index 2636ecb7294..361879dc582 100644 --- a/caps/nsScriptSecurityManager.h +++ b/caps/nsScriptSecurityManager.h @@ -14,7 +14,6 @@ #include "nsIAddonPolicyService.h" #include "nsIPrincipal.h" #include "nsCOMPtr.h" -#include "nsIChannelEventSink.h" #include "nsIObserver.h" #include "nsServiceManagerUtils.h" #include "plstr.h" @@ -39,7 +38,6 @@ class PrincipalOriginAttributes; { 0xba, 0x18, 0x00, 0x60, 0xb0, 0xf1, 0x99, 0xa2 }} class nsScriptSecurityManager final : public nsIScriptSecurityManager, - public nsIChannelEventSink, public nsIObserver { public: @@ -49,7 +47,6 @@ public: NS_DECL_ISUPPORTS NS_DECL_NSISCRIPTSECURITYMANAGER - NS_DECL_NSICHANNELEVENTSINK NS_DECL_NSIOBSERVER static nsScriptSecurityManager* diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp index c74830d01dd..f4db0edd51f 100644 --- a/dom/security/nsContentSecurityManager.cpp +++ b/dom/security/nsContentSecurityManager.cpp @@ -8,7 +8,9 @@ #include "mozilla/dom/Element.h" -NS_IMPL_ISUPPORTS(nsContentSecurityManager, nsIContentSecurityManager) +NS_IMPL_ISUPPORTS(nsContentSecurityManager, + nsIContentSecurityManager, + nsIChannelEventSink) static nsresult ValidateSecurityFlags(nsILoadInfo* aLoadInfo) @@ -342,22 +344,17 @@ nsContentSecurityManager::doContentSecurityCheck(nsIChannel* aChannel, return NS_ERROR_UNEXPECTED; } + // if dealing with a redirected channel then we have already installed + // streamlistener and redirect proxies and so we are done. + if (loadInfo->GetInitialSecurityCheckDone()) { + return NS_OK; + } + // make sure that only one of the five security flags is set in the loadinfo // e.g. do not require same origin and allow cross origin at the same time nsresult rv = ValidateSecurityFlags(loadInfo); NS_ENSURE_SUCCESS(rv, rv); - // lets store the initialSecurityCheckDone flag which indicates whether the channel - // was initialy evaluated by the contentSecurityManager. Once the inital - // asyncOpen() of the channel went through the contentSecurityManager then - // redirects do not have perform all the security checks, e.g. no reason - // to setup CORS again. - bool initialSecurityCheckDone = loadInfo->GetInitialSecurityCheckDone(); - - // now lets set the initalSecurityFlag for subsequent calls - rv = loadInfo->SetInitialSecurityCheckDone(true); - NS_ENSURE_SUCCESS(rv, rv); - // since aChannel was openend using asyncOpen2() we have to make sure // that redirects of that channel also get openend using asyncOpen2() // please note that some implementations of ::AsyncOpen2 might already @@ -366,49 +363,116 @@ nsContentSecurityManager::doContentSecurityCheck(nsIChannel* aChannel, rv = loadInfo->SetEnforceSecurity(true); NS_ENSURE_SUCCESS(rv, rv); + if (loadInfo->GetSecurityMode() == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) { + rv = DoCORSChecks(aChannel, loadInfo, aInAndOutListener); + NS_ENSURE_SUCCESS(rv, rv); + } + else { + rv = CheckChannel(aChannel); + NS_ENSURE_SUCCESS(rv, rv); + } + nsCOMPtr finalChannelURI; rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(finalChannelURI)); NS_ENSURE_SUCCESS(rv, rv); + // Perform all ContentPolicy checks (MixedContent, CSP, ...) + rv = DoContentSecurityChecks(finalChannelURI, loadInfo); + NS_ENSURE_SUCCESS(rv, rv); + + // now lets set the initalSecurityFlag for subsequent calls + rv = loadInfo->SetInitialSecurityCheckDone(true); + NS_ENSURE_SUCCESS(rv, rv); + + // all security checks passed - lets allow the load + return NS_OK; +} + +NS_IMETHODIMP +nsContentSecurityManager::AsyncOnChannelRedirect(nsIChannel* aOldChannel, + nsIChannel* aNewChannel, + uint32_t aRedirFlags, + nsIAsyncVerifyRedirectCallback *aCb) +{ + nsCOMPtr loadInfo = aOldChannel->GetLoadInfo(); + // Are we enforcing security using LoadInfo? + if (loadInfo && loadInfo->GetEnforceSecurity()) { + nsresult rv = CheckChannel(aNewChannel); + if (NS_FAILED(rv)) { + aOldChannel->Cancel(rv); + return rv; + } + } + + // Also verify that the redirecting server is allowed to redirect to the + // given URI + nsCOMPtr oldPrincipal; + nsContentUtils::GetSecurityManager()-> + GetChannelResultPrincipal(aOldChannel, getter_AddRefs(oldPrincipal)); + + nsCOMPtr newURI; + aNewChannel->GetURI(getter_AddRefs(newURI)); + nsCOMPtr newOriginalURI; + aNewChannel->GetOriginalURI(getter_AddRefs(newOriginalURI)); + + NS_ENSURE_STATE(oldPrincipal && newURI && newOriginalURI); + + const uint32_t flags = + nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT | + nsIScriptSecurityManager::DISALLOW_SCRIPT; + nsresult rv = nsContentUtils::GetSecurityManager()-> + CheckLoadURIWithPrincipal(oldPrincipal, newURI, flags); + if (NS_SUCCEEDED(rv) && newOriginalURI != newURI) { + rv = nsContentUtils::GetSecurityManager()-> + CheckLoadURIWithPrincipal(oldPrincipal, newOriginalURI, flags); + } + NS_ENSURE_SUCCESS(rv, rv); + + aCb->OnRedirectVerifyCallback(NS_OK); + return NS_OK; +} + +/* + * Check that this channel passes all security checks. Returns an error code + * if this requesst should not be permitted. + */ +nsresult +nsContentSecurityManager::CheckChannel(nsIChannel* aChannel) +{ + nsCOMPtr loadInfo = aChannel->GetLoadInfo(); + MOZ_ASSERT(loadInfo); + + // CORS mode is handled by nsCORSListenerProxy nsSecurityFlags securityMode = loadInfo->GetSecurityMode(); + if (securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) { + return NS_OK; + } + + nsCOMPtr uri; + nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri)); + NS_ENSURE_SUCCESS(rv, rv); + // if none of the REQUIRE_SAME_ORIGIN flags are set, then SOP does not apply if ((securityMode == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS) || (securityMode == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED)) { - rv = DoSOPChecks(finalChannelURI, loadInfo); + rv = DoSOPChecks(uri, loadInfo); NS_ENSURE_SUCCESS(rv, rv); } - // if dealing with a redirected channel then we only enforce SOP - // and can return at this point. - if (initialSecurityCheckDone) { - return NS_OK; - } - if ((securityMode == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS) || (securityMode == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL)) { // Please note that DoCheckLoadURIChecks should only be enforced for // cross origin requests. If the flag SEC_REQUIRE_CORS_DATA_INHERITS is set // within the loadInfo, then then CheckLoadURIWithPrincipal is performed // within nsCorsListenerProxy - rv = DoCheckLoadURIChecks(finalChannelURI, loadInfo); + rv = DoCheckLoadURIChecks(uri, loadInfo); NS_ENSURE_SUCCESS(rv, rv); } - if (securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) { - rv = DoCORSChecks(aChannel, loadInfo, aInAndOutListener); - NS_ENSURE_SUCCESS(rv, rv); - } - - // Perform all ContentPolicy checks (MixedContent, CSP, ...) - rv = DoContentSecurityChecks(finalChannelURI, loadInfo); - NS_ENSURE_SUCCESS(rv, rv); - - // all security checks passed - lets allow the load return NS_OK; } - // ==== nsIContentSecurityManager implementation ===== NS_IMETHODIMP diff --git a/dom/security/nsContentSecurityManager.h b/dom/security/nsContentSecurityManager.h index 49fef2ac92a..912c0e89f11 100644 --- a/dom/security/nsContentSecurityManager.h +++ b/dom/security/nsContentSecurityManager.h @@ -9,6 +9,7 @@ #include "nsIContentSecurityManager.h" #include "nsIChannel.h" +#include "nsIChannelEventSink.h" class nsIStreamListener; @@ -19,10 +20,12 @@ class nsIStreamListener; { 0xa2, 0x94, 0xa6, 0x51, 0xfa, 0x35, 0x22, 0x7f } } class nsContentSecurityManager : public nsIContentSecurityManager + , public nsIChannelEventSink { public: NS_DECL_ISUPPORTS NS_DECL_NSICONTENTSECURITYMANAGER + NS_DECL_NSICHANNELEVENTSINK nsContentSecurityManager() {} @@ -30,6 +33,8 @@ public: nsCOMPtr& aInAndOutListener); private: + static nsresult CheckChannel(nsIChannel* aChannel); + virtual ~nsContentSecurityManager() {} }; diff --git a/dom/security/test/csp/file_redirects_main.html b/dom/security/test/csp/file_redirects_main.html index 5c9affea04b..d05af88fe80 100644 --- a/dom/security/test/csp/file_redirects_main.html +++ b/dom/security/test/csp/file_redirects_main.html @@ -18,7 +18,6 @@ var tests = { "font-src": thisSite+page+"?testid=font-src", "object-src": thisSite+page+"?testid=object-src", "script-src": thisSite+page+"?testid=script-src", "style-src": thisSite+page+"?testid=style-src", - "worker": thisSite+page+"?testid=worker", "xhr-src": thisSite+page+"?testid=xhr-src", "from-worker": thisSite+page+"?testid=from-worker", "from-blob-worker": thisSite+page+"?testid=from-blob-worker", diff --git a/dom/security/test/csp/file_redirects_page.sjs b/dom/security/test/csp/file_redirects_page.sjs index 9e3c0d0350d..ced2d0787b9 100644 --- a/dom/security/test/csp/file_redirects_page.sjs +++ b/dom/security/test/csp/file_redirects_page.sjs @@ -14,13 +14,8 @@ function handleRequest(request, response) var resource = "/tests/dom/security/test/csp/file_redirects_resource.sjs"; // CSP header value - var additional = "" - if (query['testid'] == "worker") { - additional = "; script-src 'self' 'unsafe-inline'"; - } response.setHeader("Content-Security-Policy", - "default-src 'self' blob: ; style-src 'self' 'unsafe-inline'" + additional, - false); + "default-src 'self' blob: ; style-src 'self' 'unsafe-inline'", false); // downloadable font that redirects to another site if (query["testid"] == "font-src") { @@ -69,12 +64,6 @@ function handleRequest(request, response) return; } - // worker script resource that redirects to another site - if (query["testid"] == "worker") { - response.write(''); - return; - } - // script that XHR's to a resource that redirects to another site if (query["testid"] == "xhr-src") { response.write(''); diff --git a/dom/security/test/csp/file_redirects_resource.sjs b/dom/security/test/csp/file_redirects_resource.sjs index d281f19d8ba..da138630fdc 100644 --- a/dom/security/test/csp/file_redirects_resource.sjs +++ b/dom/security/test/csp/file_redirects_resource.sjs @@ -85,13 +85,6 @@ function handleRequest(request, response) return; } - // web worker resource - if (query["res"] == "worker") { - response.setHeader("Content-Type", "application/javascript", false); - response.write("worker script data..."); - return; - } - // internal stylesheet that loads an image from an external site if (query["res"] == "cssLoader") { let bgURL = thisSite + resource + '?redir=other&res=image&id=' + query["id"]; diff --git a/dom/security/test/csp/test_redirects.html b/dom/security/test/csp/test_redirects.html index 4a588dcbe11..df01e3b4173 100644 --- a/dom/security/test/csp/test_redirects.html +++ b/dom/security/test/csp/test_redirects.html @@ -82,14 +82,12 @@ var testExpectedResults = { "font-src": true, "script-src-redir": false, "style-src": true, "style-src-redir": false, - "worker": true, - "worker-redir": false, "xhr-src": true, "xhr-src-redir": false, "from-worker": true, - "script-src-redir-from-worker": true, /* redir is allowed since policy isn't inherited */ - "xhr-src-redir-from-worker": true, /* redir is allowed since policy isn't inherited */ - "fetch-src-redir-from-worker": true, /* redir is allowed since policy isn't inherited */ + "script-src-redir-from-worker": true, // redir is allowed since policy isn't inherited + "xhr-src-redir-from-worker": true, // redir is allowed since policy isn't inherited + "fetch-src-redir-from-worker": true, // redir is allowed since policy isn't inherited "from-blob-worker": true, "script-src-redir-from-blob-worker": false, "xhr-src-redir-from-blob-worker": false, diff --git a/layout/build/nsLayoutModule.cpp b/layout/build/nsLayoutModule.cpp index 8310b1c5487..94d58fc7098 100644 --- a/layout/build/nsLayoutModule.cpp +++ b/layout/build/nsLayoutModule.cpp @@ -1285,7 +1285,6 @@ static const mozilla::Module::ContractIDEntry kLayoutContracts[] = { { NS_PARENTPROCESSMESSAGEMANAGER_CONTRACTID, &kNS_PARENTPROCESSMESSAGEMANAGER_CID }, { NS_CHILDPROCESSMESSAGEMANAGER_CONTRACTID, &kNS_CHILDPROCESSMESSAGEMANAGER_CID }, { NS_SCRIPTSECURITYMANAGER_CONTRACTID, &kNS_SCRIPTSECURITYMANAGER_CID }, - { NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID, &kNS_SCRIPTSECURITYMANAGER_CID }, { NS_PRINCIPAL_CONTRACTID, &kNS_PRINCIPAL_CID }, { NS_SYSTEMPRINCIPAL_CONTRACTID, &kNS_SYSTEMPRINCIPAL_CID }, { NS_NULLPRINCIPAL_CONTRACTID, &kNS_NULLPRINCIPAL_CID }, diff --git a/netwerk/base/nsIOService.cpp b/netwerk/base/nsIOService.cpp index cf87b01a8bb..ad86c9524dc 100644 --- a/netwerk/base/nsIOService.cpp +++ b/netwerk/base/nsIOService.cpp @@ -50,6 +50,7 @@ #include "CaptivePortalService.h" #include "ClosingService.h" #include "ReferrerPolicy.h" +#include "nsContentSecurityManager.h" #ifdef MOZ_WIDGET_GONK #include "nsINetworkManager.h" @@ -415,8 +416,11 @@ nsIOService::AsyncOnChannelRedirect(nsIChannel* oldChan, nsIChannel* newChan, // are in a captive portal, so we trigger a recheck. RecheckCaptivePortalIfLocalRedirect(newChan); + // This is silly. I wish there was a simpler way to get at the global + // reference of the contentSecurityManager. But it lives in the XPCOM + // service registry. nsCOMPtr sink = - do_GetService(NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID); + do_GetService(NS_CONTENTSECURITYMANAGER_CONTRACTID); if (sink) { nsresult rv = helper->DelegateOnChannelRedirect(sink, oldChan, newChan, flags); diff --git a/netwerk/build/nsNetCID.h b/netwerk/build/nsNetCID.h index 31d03726c0c..f2f96cd107b 100644 --- a/netwerk/build/nsNetCID.h +++ b/netwerk/build/nsNetCID.h @@ -1004,15 +1004,6 @@ * Contracts that can be implemented by necko users. */ -/** - * This contract ID will be gotten as a service and gets the opportunity to look - * at and veto all redirects that are processed by necko. - * - * Must implement nsIChannelEventSink - */ -#define NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID \ - "@mozilla.org/netwerk/global-channel-event-sink;1" - /** * This contract ID will be gotten as a service implementing nsINetworkLinkService * and monitored by IOService for automatic online/offline management.