From 7cdbee6cd7414b27b1b1b6fd1721fc4318b0f779 Mon Sep 17 00:00:00 2001 From: Christoph Kerschbaumer Date: Mon, 3 Jun 2019 06:04:25 +0000 Subject: [PATCH] Bug 1496418: Update Content Policy checks and allow CSP checks for system principal triggered loads. r=mccr8,baku Differential Revision: https://phabricator.services.mozilla.com/D32217 --HG-- extra : moz-landing-system : lando --- caps/BasePrincipal.h | 14 ++---- dom/base/Document.cpp | 7 --- dom/base/nsContentPolicyUtils.h | 77 +++++++++++++++++------------- dom/base/test/browser_bug593387.js | 4 ++ dom/security/nsCSPContext.cpp | 13 ----- dom/security/nsCSPService.cpp | 15 ++++-- dom/security/nsCSPService.h | 6 +++ dom/workers/WorkerPrivate.cpp | 14 +++--- 8 files changed, 76 insertions(+), 74 deletions(-) diff --git a/caps/BasePrincipal.h b/caps/BasePrincipal.h index 983a5143a073..792da5bfe5ea 100644 --- a/caps/BasePrincipal.h +++ b/caps/BasePrincipal.h @@ -187,18 +187,12 @@ class BasePrincipal : public nsJSPrincipals { // (or, if no URI is given, the last allowlist principal). nsIPrincipal* PrincipalToInherit(nsIURI* aRequestedURI = nullptr); - /** - * Returns true if this principal's CSP should override a document's CSP for - * loads that it triggers. Currently true for system principal, for expanded - * principals which subsume the document principal, and add-on codebase - * principals regardless of whether they subsume the document principal. + /* Returns true if this principal's CSP should override a document's CSP for + * loads that it triggers. Currently true for expanded principals which + * subsume the document principal, and add-on codebase principals regardless + * of whether they subsume the document principal. */ bool OverridesCSP(nsIPrincipal* aDocumentPrincipal) { - // SystemPrincipal can override the page's CSP by definition. - if (mKind == eSystemPrincipal) { - return true; - } - // Expanded principals override CSP if and only if they subsume the document // principal. if (mKind == eExpandedPrincipal) { diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index 2a61654c07b4..be33221fd6b5 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -2974,13 +2974,6 @@ nsresult Document::InitCSP(nsIChannel* aChannel) { return NS_OK; } - // If this is a system privileged document - do not apply a CSP. - // After Bug 1496418 we can remove the early return and apply - // a CSP even when loading using a SystemPrincipal. - if (nsContentUtils::IsSystemPrincipal(NodePrincipal())) { - return NS_OK; - } - MOZ_ASSERT(!mCSP, "where did mCSP get set if not here?"); // If there is a CSP that needs to be inherited either from the diff --git a/dom/base/nsContentPolicyUtils.h b/dom/base/nsContentPolicyUtils.h index 9af71cb85b3f..28746a92796c 100644 --- a/dom/base/nsContentPolicyUtils.h +++ b/dom/base/nsContentPolicyUtils.h @@ -23,6 +23,7 @@ #include "nsIURI.h" #include "nsServiceManagerUtils.h" #include "nsStringFwd.h" +#include "mozilla/dom/nsCSPService.h" // XXXtw sadly, this makes consumers of nsContentPolicyUtils depend on widget #include "mozilla/dom/Document.h" @@ -166,38 +167,46 @@ inline const char* NS_CP_ContentTypeName(uint32_t contentType) { * * Note: requestOrigin is scoped outside the PR_BEGIN_MACRO/PR_END_MACRO on * purpose */ -#define CHECK_PRINCIPAL_AND_DATA(action) \ - nsCOMPtr requestOrigin; \ - PR_BEGIN_MACRO \ - if (loadingPrincipal) { \ - /* We exempt most loads into any document with the system principal \ - * from content policy checks, mostly as an optimization. Which means \ - * that we need to apply this check to the loading principal, not the \ - * principal that triggered the load. */ \ - bool isSystem = loadingPrincipal->IsSystemPrincipal(); \ - if (isSystem && contentType != nsIContentPolicy::TYPE_DOCUMENT) { \ - *decision = nsIContentPolicy::ACCEPT; \ - nsCOMPtr n = do_QueryInterface(context); \ - if (!n) { \ - nsCOMPtr win = do_QueryInterface(context); \ - n = win ? win->GetExtantDoc() : nullptr; \ - } \ - if (n) { \ - mozilla::dom::Document* d = n->OwnerDoc(); \ - if (d->IsLoadedAsData() || d->IsBeingUsedAsImage() || \ - d->IsResourceDoc()) { \ - nsCOMPtr dataPolicy = \ - do_GetService("@mozilla.org/data-document-content-policy;1"); \ - if (dataPolicy) { \ - dataPolicy->action(contentLocation, loadInfo, mimeType, decision); \ - } \ - } \ - } \ - return NS_OK; \ - } \ - nsresult rv = loadingPrincipal->GetURI(getter_AddRefs(requestOrigin)); \ - NS_ENSURE_SUCCESS(rv, rv); \ - } \ +#define CHECK_PRINCIPAL_CSP_AND_DATA(action) \ + nsCOMPtr requestOrigin; \ + PR_BEGIN_MACRO \ + if (loadingPrincipal) { \ + /* We exempt most loads into any document with the system principal \ + * from content policy (escept CSP) checks, mostly as an optimization. \ + * Which means that we need to apply this check to the loading principal, \ + * not the principal that triggered the load. */ \ + bool isSystem = loadingPrincipal->IsSystemPrincipal(); \ + if (isSystem) { \ + /* Check CSP for System Privileged pages */ \ + CSPService::ConsultCSP(contentLocation, loadInfo, mimeType, decision); \ + if (NS_CP_REJECTED(*decision)) { \ + return NS_OK; \ + } \ + if (contentType != nsIContentPolicy::TYPE_DOCUMENT) { \ + *decision = nsIContentPolicy::ACCEPT; \ + nsCOMPtr n = do_QueryInterface(context); \ + if (!n) { \ + nsCOMPtr win = do_QueryInterface(context); \ + n = win ? win->GetExtantDoc() : nullptr; \ + } \ + if (n) { \ + mozilla::dom::Document* d = n->OwnerDoc(); \ + if (d->IsLoadedAsData() || d->IsBeingUsedAsImage() || \ + d->IsResourceDoc()) { \ + nsCOMPtr dataPolicy = \ + do_GetService("@mozilla.org/data-document-content-policy;1"); \ + if (dataPolicy) { \ + dataPolicy->action(contentLocation, loadInfo, mimeType, \ + decision); \ + } \ + } \ + } \ + } \ + return NS_OK; \ + } \ + nsresult rv = loadingPrincipal->GetURI(getter_AddRefs(requestOrigin)); \ + NS_ENSURE_SUCCESS(rv, rv); \ + } \ PR_END_MACRO /** @@ -216,7 +225,7 @@ inline nsresult NS_CheckContentLoadPolicy( nsIPrincipal* loadingPrincipal = loadInfo->LoadingPrincipal(); nsCOMPtr context = loadInfo->GetLoadingContext(); nsContentPolicyType contentType = loadInfo->InternalContentPolicyType(); - CHECK_PRINCIPAL_AND_DATA(ShouldLoad); + CHECK_PRINCIPAL_CSP_AND_DATA(ShouldLoad); if (policyService) { CHECK_CONTENT_POLICY_WITH_SERVICE(ShouldLoad, policyService); } @@ -232,7 +241,7 @@ inline nsresult NS_CheckContentProcessPolicy( nsIPrincipal* loadingPrincipal = loadInfo->LoadingPrincipal(); nsCOMPtr context = loadInfo->GetLoadingContext(); nsContentPolicyType contentType = loadInfo->InternalContentPolicyType(); - CHECK_PRINCIPAL_AND_DATA(ShouldProcess); + CHECK_PRINCIPAL_CSP_AND_DATA(ShouldProcess); if (policyService) { CHECK_CONTENT_POLICY_WITH_SERVICE(ShouldProcess, policyService); } diff --git a/dom/base/test/browser_bug593387.js b/dom/base/test/browser_bug593387.js index 8fd4f5dee73a..0c118eb92278 100644 --- a/dom/base/test/browser_bug593387.js +++ b/dom/base/test/browser_bug593387.js @@ -7,6 +7,10 @@ */ add_task(async function test() { + // We have to disable CSP for this test otherwise the CSP of about:mozilla will + // block the dynamic frame creation. + await SpecialPowers.pushPrefEnv({"set": [["security.csp.enable", false],]}); + await BrowserTestUtils.withNewTab({ gBrowser, url: "chrome://global/content/mozilla.xhtml" }, async function(newBrowser) { diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp index 2b0a9d7bac69..517325f7756b 100644 --- a/dom/security/nsCSPContext.cpp +++ b/dom/security/nsCSPContext.cpp @@ -406,11 +406,6 @@ nsCSPContext::AppendPolicy(const nsAString& aPolicyString, bool aReportOnly, MOZ_ASSERT( mSelfURI, "did you forget to call setRequestContextWith{Document,Principal}?"); - // After Bug 1496418 we can remove that assertion because we will allow - // CSP on system privileged documents. - MOZ_ASSERT(!nsContentUtils::IsSystemPrincipal(mLoadingPrincipal), - "Do not call setRequestContextWith{Document,Principal} using " - "SystemPrincipal"); NS_ENSURE_TRUE(mLoadingPrincipal, NS_ERROR_UNEXPECTED); NS_ENSURE_TRUE(mSelfURI, NS_ERROR_UNEXPECTED); @@ -725,10 +720,6 @@ nsCSPContext::SetRequestContextWithDocument(Document* aDocument) { MOZ_ASSERT(mLoadingPrincipal, "need a valid requestPrincipal"); MOZ_ASSERT(mSelfURI, "need mSelfURI to translate 'self' into actual URI"); - // After Bug 1496418 we can remove that assertion because we will allow - // CSP on system privileged documents. - MOZ_ASSERT(!nsContentUtils::IsSystemPrincipal(mLoadingPrincipal), - "do not apply CSP to system privileged documents"); return NS_OK; } @@ -752,10 +743,6 @@ nsCSPContext::SetRequestContextWithPrincipal(nsIPrincipal* aRequestPrincipal, MOZ_ASSERT(mLoadingPrincipal, "need a valid requestPrincipal"); MOZ_ASSERT(mSelfURI, "need mSelfURI to translate 'self' into actual URI"); - // After Bug 1496418 we can remove that assertion because we will allow - // CSP on system privileged documents. - MOZ_ASSERT(!nsContentUtils::IsSystemPrincipal(mLoadingPrincipal), - "do not apply CSP to system privileged documents"); return NS_OK; } diff --git a/dom/security/nsCSPService.cpp b/dom/security/nsCSPService.cpp index 5c849ab11230..a64eef3a6480 100644 --- a/dom/security/nsCSPService.cpp +++ b/dom/security/nsCSPService.cpp @@ -113,10 +113,10 @@ bool subjectToCSP(nsIURI* aURI, nsContentPolicyType aContentType) { return true; } -/* nsIContentPolicy implementation */ -NS_IMETHODIMP -CSPService::ShouldLoad(nsIURI* aContentLocation, nsILoadInfo* aLoadInfo, - const nsACString& aMimeTypeGuess, int16_t* aDecision) { +/* static */ nsresult CSPService::ConsultCSP(nsIURI* aContentLocation, + nsILoadInfo* aLoadInfo, + const nsACString& aMimeTypeGuess, + int16_t* aDecision) { if (!aContentLocation) { return NS_ERROR_FAILURE; } @@ -206,6 +206,13 @@ CSPService::ShouldLoad(nsIURI* aContentLocation, nsILoadInfo* aLoadInfo, return NS_OK; } +/* nsIContentPolicy implementation */ +NS_IMETHODIMP +CSPService::ShouldLoad(nsIURI* aContentLocation, nsILoadInfo* aLoadInfo, + const nsACString& aMimeTypeGuess, int16_t* aDecision) { + return ConsultCSP(aContentLocation, aLoadInfo, aMimeTypeGuess, aDecision); +} + NS_IMETHODIMP CSPService::ShouldProcess(nsIURI* aContentLocation, nsILoadInfo* aLoadInfo, const nsACString& aMimeTypeGuess, diff --git a/dom/security/nsCSPService.h b/dom/security/nsCSPService.h index c81f881111b2..4056ecf50503 100644 --- a/dom/security/nsCSPService.h +++ b/dom/security/nsCSPService.h @@ -28,6 +28,12 @@ class CSPService : public nsIContentPolicy, public nsIChannelEventSink { CSPService(); + // helper function to avoid creating a new instance of the + // cspservice everytime we call content policies. + static nsresult ConsultCSP(nsIURI* aContentLocation, nsILoadInfo* aLoadInfo, + const nsACString& aMimeTypeGuess, + int16_t* aDecision); + protected: virtual ~CSPService(); }; diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index a5c9bf998f1e..3f8cf62e0a77 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/dom/workers/WorkerPrivate.cpp @@ -1272,20 +1272,22 @@ nsresult WorkerPrivate::SetCSPFromHeaderValues( AssertIsOnMainThread(); MOZ_DIAGNOSTIC_ASSERT(!mLoadInfo.mCSP); - if (nsContentUtils::IsSystemPrincipal(mLoadInfo.mPrincipal)) { - // After Bug 1496418 we can remove the early return and apply - // a CSP even when loading using a SystemPrincipal. - return NS_OK; - } - NS_ConvertASCIItoUTF16 cspHeaderValue(aCSPHeaderValue); NS_ConvertASCIItoUTF16 cspROHeaderValue(aCSPReportOnlyHeaderValue); nsresult rv; nsCOMPtr csp = new nsCSPContext(); + // First, we try to query the URI from the Principal, but + // in case selfURI remains empty (e.g in case the Principal + // is a SystemPrincipal) then we fall back and use the + // base URI as selfURI for CSP. nsCOMPtr selfURI; mLoadInfo.mPrincipal->GetURI(getter_AddRefs(selfURI)); + if (!selfURI) { + selfURI = mLoadInfo.mBaseURI; + } + MOZ_ASSERT(selfURI, "need a self URI for CSP"); rv = csp->SetRequestContextWithPrincipal(mLoadInfo.mPrincipal, selfURI, EmptyString(), 0);