From 9ad680cfc48c4ea041697176fddaa016268e630d Mon Sep 17 00:00:00 2001 From: Moonchild Date: Sat, 26 Apr 2025 20:07:16 +0200 Subject: [PATCH] Issue #2736 - Part 1: Provide more consistent principals to CSP. We're currently fairly vague and inconsistent about the values we provide to content policy implementations for requestOrigin and requestPrincipal. In some cases they're the triggering principal, sometimes the loading principal, sometimes the channel principal. Our existing content policy implementations which require or expect a loading principal currently retrieve it from the context node. Since no current callers require the principal to be the loading principal, and some already expect it to be the triggering principal (which there's currently no other way to retrieve), a choice was made to pass the triggering principal whenever possible, but use the loading principal to determine the origin URL. --- docshell/base/nsDocShell.cpp | 3 ++ dom/base/WebSocket.cpp | 3 +- dom/base/nsContentPolicyUtils.h | 48 +++++++++++-------- dom/base/nsContentUtils.cpp | 1 + dom/base/nsIContentPolicy.idl | 15 ++++-- dom/base/nsObjectLoadingContent.cpp | 6 ++- dom/html/ImageDocument.cpp | 3 ++ .../base/nsPluginStreamListenerPeer.cpp | 3 +- dom/script/ScriptLoader.cpp | 3 +- dom/security/nsContentSecurityManager.cpp | 11 +---- dom/workers/ServiceWorkerEvents.cpp | 10 ++-- dom/workers/ServiceWorkerManager.cpp | 3 +- dom/xml/nsXMLContentSink.cpp | 3 +- image/imgLoader.cpp | 3 +- layout/style/FontFaceSet.cpp | 3 +- layout/style/Loader.cpp | 44 +++++++++++------ layout/style/Loader.h | 3 +- toolkit/modules/addons/WebRequestContent.js | 5 +- 18 files changed, 109 insertions(+), 61 deletions(-) diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 770eb43119..459ed7523a 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -9951,6 +9951,9 @@ nsDocShell::InternalLoad(nsIURI* aURI, int16_t shouldLoad = nsIContentPolicy::ACCEPT; rv = NS_CheckContentLoadPolicy(contentType, aURI, + // This is a top-level load, so the loading + // principal is null. + nullptr, aTriggeringPrincipal, requestingContext, EmptyCString(), // mime guess diff --git a/dom/base/WebSocket.cpp b/dom/base/WebSocket.cpp index 075697dd71..f91de98773 100644 --- a/dom/base/WebSocket.cpp +++ b/dom/base/WebSocket.cpp @@ -1556,7 +1556,8 @@ WebSocketImpl::Init(JSContext* aCx, int16_t shouldLoad = nsIContentPolicy::ACCEPT; aRv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_WEBSOCKET, uri, - aPrincipal, + aPrincipal, // loading principal + aPrincipal, // triggering principal originDoc, EmptyCString(), nullptr, diff --git a/dom/base/nsContentPolicyUtils.h b/dom/base/nsContentPolicyUtils.h index 3984ede544..9b4de24276 100644 --- a/dom/base/nsContentPolicyUtils.h +++ b/dom/base/nsContentPolicyUtils.h @@ -152,7 +152,7 @@ NS_CP_ContentTypeName(uint32_t contentType) return NS_ERROR_FAILURE; \ \ return policy-> action (contentType, contentLocation, requestOrigin, \ - context, mimeType, extra, originPrincipal, \ + context, mimeType, extra, triggeringPrincipal, \ decision); \ PR_END_MACRO @@ -160,7 +160,7 @@ NS_CP_ContentTypeName(uint32_t contentType) #define CHECK_CONTENT_POLICY_WITH_SERVICE(action, _policy) \ PR_BEGIN_MACRO \ return _policy-> action (contentType, contentLocation, requestOrigin, \ - context, mimeType, extra, originPrincipal, \ + context, mimeType, extra, triggeringPrincipal, \ decision); \ PR_END_MACRO @@ -173,14 +173,18 @@ NS_CP_ContentTypeName(uint32_t contentType) #define CHECK_PRINCIPAL_AND_DATA(action) \ nsCOMPtr requestOrigin; \ PR_BEGIN_MACRO \ - if (originPrincipal) { \ + if (loadingPrincipal) { \ nsCOMPtr secMan = aSecMan; \ if (!secMan) { \ secMan = do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID); \ } \ if (secMan) { \ bool isSystem; \ - nsresult rv = secMan->IsSystemPrincipal(originPrincipal, \ + /* 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. */ \ + nsresult rv = secMan->IsSystemPrincipal(loadingPrincipal, \ &isSystem); \ NS_ENSURE_SUCCESS(rv, rv); \ if (isSystem && contentType != nsIContentPolicy::TYPE_DOCUMENT) { \ @@ -203,31 +207,33 @@ NS_CP_ContentTypeName(uint32_t contentType) dataPolicy-> action (externalType, contentLocation, \ requestOrigin, context, \ mimeType, extra, \ - originPrincipal, decision); \ + triggeringPrincipal, decision);\ } \ } \ } \ return NS_OK; \ } \ } \ - nsresult rv = originPrincipal->GetURI(getter_AddRefs(requestOrigin)); \ + nsresult rv = loadingPrincipal->GetURI(getter_AddRefs(requestOrigin)); \ NS_ENSURE_SUCCESS(rv, rv); \ } \ PR_END_MACRO /** * Alias for calling ShouldLoad on the content policy service. Parameters are - * the same as nsIContentPolicy::shouldLoad, except for the originPrincipal - * parameter, which should be non-null if possible, and the last two - * parameters, which can be used to pass in pointer to some useful services if - * the caller already has them. The origin URI to pass to shouldLoad will be - * the URI of originPrincipal, unless originPrincipal is null (in which case a - * null origin URI will be passed). + * the same as nsIContentPolicy::shouldLoad, except for the loadingPrincipal + * and triggeringPrincipal parameters (which should be non-null if possible, + * and have the same semantics as in LoadInfo), and the last two parameters, + * which can be used to pass in pointer to some useful services if the caller + * already has them. The origin URI to pass to shouldLoad will be the URI of + * loadingPrincipal, unless loadingPrincipal is null (in which case a null + * origin URI will be passed). */ inline nsresult NS_CheckContentLoadPolicy(uint32_t contentType, nsIURI *contentLocation, - nsIPrincipal *originPrincipal, + nsIPrincipal *loadingPrincipal, + nsIPrincipal *triggeringPrincipal, nsISupports *context, const nsACString &mimeType, nsISupports *extra, @@ -244,17 +250,19 @@ NS_CheckContentLoadPolicy(uint32_t contentType, /** * Alias for calling ShouldProcess on the content policy service. Parameters - * are the same as nsIContentPolicy::shouldLoad, except for the originPrincipal - * parameter, which should be non-null if possible, and the last two - * parameters, which can be used to pass in pointer to some useful services if - * the caller already has them. The origin URI to pass to shouldLoad will be - * the URI of originPrincipal, unless originPrincipal is null (in which case a - * null origin URI will be passed). + * are the same as nsIContentPolicy::shouldLoad, except for the and + * triggeringPrincipal parameters (which should be non-null if possible, and + * have the same semantics as in nsLoadInfo), and the last parameter, which + * can be used to pass in a pointer to a useful service if the caller already + * has it. The origin URI to pass to shouldLoad will be the URI of + * loadingPrincipal, unless loadingPrincipal is null (in which case a null + * origin URI will be passed). */ inline nsresult NS_CheckContentProcessPolicy(uint32_t contentType, nsIURI *contentLocation, - nsIPrincipal *originPrincipal, + nsIPrincipal *loadingPrincipal, + nsIPrincipal *triggeringPrincipal, nsISupports *context, const nsACString &mimeType, nsISupports *extra, diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index 155d55ac29..1d3086c512 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -3289,6 +3289,7 @@ nsContentUtils::CanLoadImage(nsIURI* aURI, nsISupports* aContext, rv = NS_CheckContentLoadPolicy(aContentType, aURI, aLoadingPrincipal, + aLoadingPrincipal, // triggering principal aContext, EmptyCString(), //mime guess nullptr, //extra diff --git a/dom/base/nsIContentPolicy.idl b/dom/base/nsIContentPolicy.idl index 870ed6f739..31044e46f0 100644 --- a/dom/base/nsIContentPolicy.idl +++ b/dom/base/nsIContentPolicy.idl @@ -34,8 +34,11 @@ interface nsIContentPolicy : nsIContentPolicyBase * not be null * * @param aRequestOrigin OPTIONAL. the location of the resource that - * initiated this load request; can be null if - * inapplicable + * that is loading the request. This will generally + * be the URI of the loading principal for the + * resulting request (as determined by its + * LoadInfo), but may vary depending on the + * caller. Can be null if inapplicable. * * @param aContext OPTIONAL. the nsIDOMNode or nsIDOMWindow that * initiated the request, or something that can QI @@ -55,8 +58,12 @@ interface nsIContentPolicy : nsIContentPolicyBase * @param aRequestPrincipal an OPTIONAL argument, defines the principal that * caused the load. This is optional only for * non-gecko code: all gecko code should set this - * argument. For navigation events, this is - * the principal of the page that caused this load. + * argument. This should generally be the same as + * the triggering principal for the resulting + * request (as determined by its LoadInfo), but may + * vary depending on the caller. Sometimes it will + * be the loading principal or final channel + * principal instead. * * @return ACCEPT or REJECT_* * diff --git a/dom/base/nsObjectLoadingContent.cpp b/dom/base/nsObjectLoadingContent.cpp index 0f0c9106eb..e6ff926906 100644 --- a/dom/base/nsObjectLoadingContent.cpp +++ b/dom/base/nsObjectLoadingContent.cpp @@ -1633,7 +1633,8 @@ nsObjectLoadingContent::CheckLoadPolicy(int16_t *aContentPolicy) *aContentPolicy = nsIContentPolicy::ACCEPT; nsresult rv = NS_CheckContentLoadPolicy(contentPolicyType, mURI, - doc->NodePrincipal(), + doc->NodePrincipal(), // loading principal + doc->NodePrincipal(), // triggering principal thisContent, mContentType, nullptr, //extra @@ -1684,7 +1685,8 @@ nsObjectLoadingContent::CheckProcessPolicy(int16_t *aContentPolicy) nsresult rv = NS_CheckContentProcessPolicy(objectType, mURI ? mURI : mBaseURI, - doc->NodePrincipal(), + doc->NodePrincipal(), // loading principal + doc->NodePrincipal(), // triggering principal static_cast(this), mContentType, nullptr, //extra diff --git a/dom/html/ImageDocument.cpp b/dom/html/ImageDocument.cpp index 87bf3ee99f..efa95f36c5 100644 --- a/dom/html/ImageDocument.cpp +++ b/dom/html/ImageDocument.cpp @@ -97,11 +97,14 @@ ImageListener::OnStartRequest(nsIRequest* request, nsISupports *ctxt) if (secMan) { secMan->GetChannelResultPrincipal(channel, getter_AddRefs(channelPrincipal)); } + + nsCOMPtr loadInfo = channel->GetLoadInfo(); int16_t decision = nsIContentPolicy::ACCEPT; nsresult rv = NS_CheckContentProcessPolicy(nsIContentPolicy::TYPE_INTERNAL_IMAGE, channelURI, channelPrincipal, + loadInfo ? loadInfo->TriggeringPrincipal() : nullptr, domWindow->GetFrameElementInternal(), mimeType, nullptr, diff --git a/dom/plugins/base/nsPluginStreamListenerPeer.cpp b/dom/plugins/base/nsPluginStreamListenerPeer.cpp index 603f2408c8..3cde93725c 100644 --- a/dom/plugins/base/nsPluginStreamListenerPeer.cpp +++ b/dom/plugins/base/nsPluginStreamListenerPeer.cpp @@ -505,7 +505,8 @@ nsPluginStreamListenerPeer::OnStartRequest(nsIRequest *request, int16_t shouldLoad = nsIContentPolicy::ACCEPT; rv = NS_CheckContentProcessPolicy(nsIContentPolicy::TYPE_OBJECT_SUBREQUEST, mURL, - principal, + principal, // loading principal + principal, // triggering principal element, contentType, nullptr, diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index b6462a0cee..dc07d47781 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -448,7 +448,8 @@ ScriptLoader::CheckContentPolicy(nsIDocument* aDocument, int16_t shouldLoad = nsIContentPolicy::ACCEPT; nsresult rv = NS_CheckContentLoadPolicy(contentPolicyType, aURI, - aDocument->NodePrincipal(), + aDocument->NodePrincipal(), // loading principal + aDocument->NodePrincipal(), // triggering principal aContext, NS_LossyConvertUTF16toASCII(aType), nullptr, //extra diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp index d33f962ecd..1539e263d5 100644 --- a/dom/security/nsContentSecurityManager.cpp +++ b/dom/security/nsContentSecurityManager.cpp @@ -573,18 +573,11 @@ DoContentSecurityChecks(nsIChannel* aChannel, nsILoadInfo* aLoadInfo) MOZ_ASSERT(false, "can not perform security check without a valid contentType"); } - // For document loads we use the triggeringPrincipal as the originPrincipal. - // Note the the loadingPrincipal for loads of TYPE_DOCUMENT is a nullptr. - nsCOMPtr principal = - (contentPolicyType == nsIContentPolicy::TYPE_DOCUMENT || - contentPolicyType == nsIContentPolicy::TYPE_SUBDOCUMENT) - ? aLoadInfo->TriggeringPrincipal() - : aLoadInfo->LoadingPrincipal(); - int16_t shouldLoad = nsIContentPolicy::ACCEPT; rv = NS_CheckContentLoadPolicy(internalContentPolicyType, uri, - principal, + aLoadInfo->LoadingPrincipal(), + aLoadInfo->TriggeringPrincipal(), requestingContext, mimeTypeGuess, nullptr, //extra, diff --git a/dom/workers/ServiceWorkerEvents.cpp b/dom/workers/ServiceWorkerEvents.cpp index 34b307caf4..ec22a68d63 100644 --- a/dom/workers/ServiceWorkerEvents.cpp +++ b/dom/workers/ServiceWorkerEvents.cpp @@ -261,10 +261,14 @@ public: rv = NS_NewURI(getter_AddRefs(uri), url, nullptr, nullptr); NS_ENSURE_SUCCESS(rv, false); int16_t decision = nsIContentPolicy::ACCEPT; - rv = NS_CheckContentLoadPolicy(aLoadInfo->InternalContentPolicyType(), uri, + rv = NS_CheckContentLoadPolicy(aLoadInfo->InternalContentPolicyType(), + uri, aLoadInfo->LoadingPrincipal(), - aLoadInfo->LoadingNode(), EmptyCString(), - nullptr, &decision); + aLoadInfo->TriggeringPrincipal(), + aLoadInfo->LoadingNode(), + EmptyCString(), + nullptr, + &decision); NS_ENSURE_SUCCESS(rv, false); return decision == nsIContentPolicy::ACCEPT; } diff --git a/dom/workers/ServiceWorkerManager.cpp b/dom/workers/ServiceWorkerManager.cpp index 7f70c9ee10..6c0e8250b8 100644 --- a/dom/workers/ServiceWorkerManager.cpp +++ b/dom/workers/ServiceWorkerManager.cpp @@ -592,7 +592,8 @@ ServiceWorkerManager::Register(mozIDOMWindow* aWindow, int16_t decision = nsIContentPolicy::ACCEPT; rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER, aScriptURI, - documentPrincipal, + documentPrincipal, // loading principal + documentPrincipal, // triggering principal doc, EmptyCString(), nullptr, diff --git a/dom/xml/nsXMLContentSink.cpp b/dom/xml/nsXMLContentSink.cpp index 2c55265193..44eff4cc32 100644 --- a/dom/xml/nsXMLContentSink.cpp +++ b/dom/xml/nsXMLContentSink.cpp @@ -696,7 +696,8 @@ nsXMLContentSink::ProcessStyleLink(nsIContent* aElement, int16_t decision = nsIContentPolicy::ACCEPT; rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_XSLT, url, - mDocument->NodePrincipal(), + mDocument->NodePrincipal(), // loading principal + mDocument->NodePrincipal(), // triggering principal aElement, type, nullptr, diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp index 1bf6ece905..503bc5ed11 100644 --- a/image/imgLoader.cpp +++ b/image/imgLoader.cpp @@ -567,7 +567,8 @@ ShouldLoadCachedImage(imgRequest* aImgRequest, int16_t decision = nsIContentPolicy::REJECT_REQUEST; rv = NS_CheckContentLoadPolicy(aPolicyType, contentLocation, - aLoadingPrincipal, + aLoadingPrincipal, // loading principal + aLoadingPrincipal, // triggering principal aLoadingContext, EmptyCString(), //mime guess nullptr, //aExtra diff --git a/layout/style/FontFaceSet.cpp b/layout/style/FontFaceSet.cpp index 402e7c05a3..5b390d769e 100644 --- a/layout/style/FontFaceSet.cpp +++ b/layout/style/FontFaceSet.cpp @@ -1366,7 +1366,8 @@ FontFaceSet::IsFontLoadAllowed(nsIURI* aFontLocation, nsIPrincipal* aPrincipal) int16_t shouldLoad = nsIContentPolicy::ACCEPT; nsresult rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_FONT, aFontLocation, - aPrincipal, + aPrincipal, // loading principal + aPrincipal, // triggering principal mDocument, EmptyCString(), // mime type nullptr, // aExtra diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp index 3b445eb567..41ed9a1af5 100644 --- a/layout/style/Loader.cpp +++ b/layout/style/Loader.cpp @@ -1028,14 +1028,15 @@ Loader::ObsoleteSheet(nsIURI* aURI) } nsresult -Loader::CheckContentPolicy(nsIPrincipal* aSourcePrincipal, - nsIURI* aTargetURI, - nsISupports* aContext, - bool aIsPreload) +Loader::CheckContentPolicy(nsIPrincipal* aLoadingPrincipal, + nsIPrincipal* aTriggeringPrincipal, + nsIURI* aTargetURI, + nsISupports* aContext, + bool aIsPreload) { // When performing a system load (e.g. aUseSystemPrincipal = true) - // then aSourcePrincipal == null; don't consult content policies. - if (!aSourcePrincipal) { + // then aLoadingPrincipal == null; don't consult content policies. + if (!aLoadingPrincipal) { return NS_OK; } @@ -1046,7 +1047,8 @@ Loader::CheckContentPolicy(nsIPrincipal* aSourcePrincipal, int16_t shouldLoad = nsIContentPolicy::ACCEPT; nsresult rv = NS_CheckContentLoadPolicy(contentPolicyType, aTargetURI, - aSourcePrincipal, + aLoadingPrincipal, + aTriggeringPrincipal, aContext, NS_LITERAL_CSTRING("text/css"), nullptr, //extra param @@ -2039,15 +2041,20 @@ Loader::LoadStyleLink(nsIContent* aElement, NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_INITIALIZED); - nsIPrincipal* principal = - aElement ? aElement->NodePrincipal() : mDocument->NodePrincipal(); + nsIPrincipal* loadingPrincipal = aElement ? aElement->NodePrincipal() + : mDocument->NodePrincipal(); + //SHOULD BE: + //nsIPrincipal* principal = aTriggeringPrincipal ? aTriggeringPrincipal + // : loadingPrincipal; + nsIPrincipal* principal = loadingPrincipal; + nsISupports* context = aElement; if (!context) { context = mDocument; } - nsresult rv = CheckContentPolicy(principal, aURL, context, false); + nsresult rv = CheckContentPolicy(loadingPrincipal, principal, aURL, context, false); if (NS_WARN_IF(NS_FAILED(rv))) { // Don't fire the error event if our document is loaded as data. We're // supposed to not even try to do loads in that case... Unfortunately, we @@ -2188,13 +2195,18 @@ Loader::LoadChildSheet(StyleSheet* aParentSheet, owningNode = topSheet->GetOwnerNode(); } - nsISupports* context = owningNode; - if (!context) { + nsISupports* context = nullptr; + nsIPrincipal* loadingPrincipal = nullptr; + if (owningNode) { + context = owningNode; + loadingPrincipal = owningNode->NodePrincipal(); + } else if (mDocument) { context = mDocument; + loadingPrincipal = mDocument->NodePrincipal(); } nsIPrincipal* principal = aParentSheet->Principal(); - nsresult rv = CheckContentPolicy(principal, aURL, context, false); + nsresult rv = CheckContentPolicy(loadingPrincipal, principal, aURL, context, false); NS_ENSURE_SUCCESS(rv, rv); SheetLoadData* parentData = nullptr; @@ -2351,7 +2363,11 @@ Loader::InternalLoadNonDocumentSheet(nsIURI* aURL, return NS_ERROR_NOT_AVAILABLE; } - nsresult rv = CheckContentPolicy(aOriginPrincipal, aURL, mDocument, aIsPreload); + nsCOMPtr loadingPrincipal = (aOriginPrincipal && mDocument + ? mDocument->NodePrincipal() + : nullptr); + nsresult rv = CheckContentPolicy(loadingPrincipal, aOriginPrincipal, + aURL, mDocument, aIsPreload); NS_ENSURE_SUCCESS(rv, rv); StyleSheetState state; diff --git a/layout/style/Loader.h b/layout/style/Loader.h index 2319164a2f..830099b3ce 100644 --- a/layout/style/Loader.h +++ b/layout/style/Loader.h @@ -452,7 +452,8 @@ public: private: friend class SheetLoadData; - nsresult CheckContentPolicy(nsIPrincipal* aSourcePrincipal, + nsresult CheckContentPolicy(nsIPrincipal* aLoadingPrincipal, + nsIPrincipal* aTriggeringPrincipal, nsIURI* aTargetURI, nsISupports* aContext, bool aIsPreload); diff --git a/toolkit/modules/addons/WebRequestContent.js b/toolkit/modules/addons/WebRequestContent.js index f044a1cd4e..d3f52bc6a5 100644 --- a/toolkit/modules/addons/WebRequestContent.js +++ b/toolkit/modules/addons/WebRequestContent.js @@ -173,7 +173,10 @@ var ContentPolicy = { windowId, parentWindowId}; if (requestOrigin) { - data.originUrl = requestOrigin.spec; + data.documentUrl = requestOrigin.spec; + } + if (requestPrincipal && requestPrincipal.URI) { + data.originUrl = requestPrincipal.URI.spec; } if (block) { let rval = mm.sendSyncMessage("WebRequest:ShouldLoad", data);