From 3e884ce321dcc04e273f5bd0ae98be96c1100d3b Mon Sep 17 00:00:00 2001 From: Christoph Kerschbaumer Date: Fri, 20 Mar 2020 08:25:18 +0000 Subject: [PATCH] Bug 1188538: Ensure every protocol handler sets a valid security flag. r=bholley,mixedpuppy Differential Revision: https://phabricator.services.mozilla.com/D67496 --HG-- extra : moz-landing-system : lando --- .../browser/browser_ext_search_favicon.js | 2 +- caps/nsScriptSecurityManager.cpp | 46 ++++++---------- dom/security/nsContentSecurityManager.cpp | 54 +++++++++++++++++++ dom/security/nsContentSecurityManager.h | 1 + netwerk/base/nsIProtocolHandler.idl | 14 ++--- .../protocol/res/ExtensionProtocolHandler.cpp | 4 ++ 6 files changed, 83 insertions(+), 38 deletions(-) diff --git a/browser/components/extensions/test/browser/browser_ext_search_favicon.js b/browser/components/extensions/test/browser/browser_ext_search_favicon.js index 83d9cc1ea669..081cf62692f3 100644 --- a/browser/components/extensions/test/browser/browser_ext_search_favicon.js +++ b/browser/components/extensions/test/browser/browser_ext_search_favicon.js @@ -54,7 +54,7 @@ add_task(async function test_search_favicon() { search_provider: { name: "Bad Icon", search_url: "https://example.net/", - favicon_url: "moz-extension://invalid/iDoNotExist.png", + favicon_url: "iDoNotExist.png", }, }, }, diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp index 4da2b5354869..840e9c64c25d 100644 --- a/caps/nsScriptSecurityManager.cpp +++ b/caps/nsScriptSecurityManager.cpp @@ -961,38 +961,22 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags( return NS_ERROR_DOM_BAD_URI; } - // OK, everyone is allowed to load this, since unflagged handlers are - // deprecated but treated as URI_LOADABLE_BY_ANYONE. But check whether we - // need to warn. At some point we'll want to make this warning into an - // error and treat unflagged handlers as URI_DANGEROUS_TO_LOAD. - rv = NS_URIChainHasFlags( - aTargetBaseURI, nsIProtocolHandler::URI_LOADABLE_BY_ANYONE, &hasFlags); - NS_ENSURE_SUCCESS(rv, rv); - // NB: we also get here if the base URI is URI_LOADABLE_BY_SUBSUMERS, - // and none of the rest of the nested chain of URIs for aTargetURI - // prohibits the load, so avoid warning in that case: - bool hasSubsumersFlag = false; - rv = NS_URIChainHasFlags(aTargetBaseURI, - nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS, - &hasSubsumersFlag); - NS_ENSURE_SUCCESS(rv, rv); - if (!hasFlags && !hasSubsumersFlag) { - nsCOMPtr bundle = BundleHelper::GetOrCreate(); - if (bundle) { - nsAutoString message; - AutoTArray formatStrings; - CopyASCIItoUTF16(targetScheme, *formatStrings.AppendElement()); - rv = bundle->FormatStringFromName("ProtocolFlagError", formatStrings, - message); - if (NS_SUCCEEDED(rv)) { - nsCOMPtr console( - do_GetService("@mozilla.org/consoleservice;1")); - NS_ENSURE_TRUE(console, NS_ERROR_FAILURE); - - console->LogStringMessage(message.get()); - } - } +#ifdef DEBUG + { + // Everyone is allowed to load this. The case URI_LOADABLE_BY_SUBSUMERS + // is handled by the caller which is just delegating to us as a helper. + bool hasSubsumersFlag = false; + NS_URIChainHasFlags(aTargetBaseURI, + nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS, + &hasSubsumersFlag); + bool hasLoadableByAnyone = false; + NS_URIChainHasFlags(aTargetBaseURI, + nsIProtocolHandler::URI_LOADABLE_BY_ANYONE, + &hasLoadableByAnyone); + MOZ_ASSERT(hasLoadableByAnyone || hasSubsumersFlag, + "why do we get here and do not have any of the two flags set?"); } +#endif return NS_OK; } diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp index d79b75894861..e2ce32ac01f7 100644 --- a/dom/security/nsContentSecurityManager.cpp +++ b/dom/security/nsContentSecurityManager.cpp @@ -841,6 +841,57 @@ nsresult nsContentSecurityManager::CheckAllowLoadInSystemPrivilegedContext( return NS_ERROR_CONTENT_BLOCKED; } +/* + * Every protocol handler must set one of the five security flags + * defined in nsIProtocolHandler - if not - deny the load. + */ +nsresult nsContentSecurityManager::CheckChannelHasProtocolSecurityFlag( + nsIChannel* aChannel) { + nsCOMPtr uri; + nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri)); + NS_ENSURE_SUCCESS(rv, rv); + + nsAutoCString scheme; + rv = uri->GetScheme(scheme); + NS_ENSURE_SUCCESS(rv, rv); + + nsCOMPtr ios = do_GetIOService(&rv); + NS_ENSURE_SUCCESS(rv, rv); + + nsCOMPtr handler; + rv = ios->GetProtocolHandler(scheme.get(), getter_AddRefs(handler)); + NS_ENSURE_SUCCESS(rv, rv); + + uint32_t flags; + rv = handler->DoGetProtocolFlags(uri, &flags); + NS_ENSURE_SUCCESS(rv, rv); + + uint32_t securityFlagsSet = 0; + if (flags & nsIProtocolHandler::URI_LOADABLE_BY_ANYONE) { + securityFlagsSet += 1; + } + if (flags & nsIProtocolHandler::URI_DANGEROUS_TO_LOAD) { + securityFlagsSet += 1; + } + if (flags & nsIProtocolHandler::URI_IS_UI_RESOURCE) { + securityFlagsSet += 1; + } + if (flags & nsIProtocolHandler::URI_IS_LOCAL_FILE) { + securityFlagsSet += 1; + } + if (flags & nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS) { + securityFlagsSet += 1; + } + + // Ensure that only "1" valid security flags is set. + if (securityFlagsSet == 1) { + return NS_OK; + } + + MOZ_ASSERT(false, "protocol must use one valid security flag"); + return NS_ERROR_CONTENT_BLOCKED; +} + /* * Based on the security flags provided in the loadInfo of the channel, * doContentSecurityCheck() performs the following content security checks @@ -870,6 +921,9 @@ nsresult nsContentSecurityManager::doContentSecurityCheck( nsresult rv = CheckAllowLoadInSystemPrivilegedContext(aChannel); NS_ENSURE_SUCCESS(rv, rv); + rv = CheckChannelHasProtocolSecurityFlag(aChannel); + NS_ENSURE_SUCCESS(rv, rv); + // if dealing with a redirected channel then we have already installed // streamlistener and redirect proxies and so we are done. if (loadInfo->GetInitialSecurityCheckDone()) { diff --git a/dom/security/nsContentSecurityManager.h b/dom/security/nsContentSecurityManager.h index 2262821c0627..c2d51d4023c4 100644 --- a/dom/security/nsContentSecurityManager.h +++ b/dom/security/nsContentSecurityManager.h @@ -42,6 +42,7 @@ class nsContentSecurityManager : public nsIContentSecurityManager, static nsresult CheckChannel(nsIChannel* aChannel); static nsresult CheckFTPSubresourceLoad(nsIChannel* aChannel); static nsresult CheckAllowLoadInSystemPrivilegedContext(nsIChannel* aChannel); + static nsresult CheckChannelHasProtocolSecurityFlag(nsIChannel* aChannel); virtual ~nsContentSecurityManager() = default; }; diff --git a/netwerk/base/nsIProtocolHandler.idl b/netwerk/base/nsIProtocolHandler.idl index 5c1a21c4a776..a323603238dc 100644 --- a/netwerk/base/nsIProtocolHandler.idl +++ b/netwerk/base/nsIProtocolHandler.idl @@ -171,16 +171,18 @@ interface nsIProtocolHandler : nsISupports * | | * +-------------------------------------------------------------------+ * + * * URI_LOADABLE_BY_ANYONE + * * URI_DANGEROUS_TO_LOAD + * * URI_IS_UI_RESOURCE + * * URI_IS_LOCAL_FILE + * * URI_LOADABLE_BY_SUBSUMERS + * * These flags are used to determine who is allowed to load URIs for this * protocol. Note that if a URI is nested, only the flags for the * innermost URI matter. See nsINestedURI. * - * If none of these five flags are set, the URI must be treated as if it - * had the URI_LOADABLE_BY_ANYONE flag set, for compatibility with protocol - * handlers written against Gecko 1.8 or earlier. In this case, there may - * be run-time warning messages indicating that a "default insecure" - * assumption is being made. At some point in the futures (Mozilla 2.0, - * most likely), these warnings will become errors. + * If none of these five flags are set, the ContentSecurityManager will + * deny the load. */ /** diff --git a/netwerk/protocol/res/ExtensionProtocolHandler.cpp b/netwerk/protocol/res/ExtensionProtocolHandler.cpp index 7972e8e355a2..a7806f827a1b 100644 --- a/netwerk/protocol/res/ExtensionProtocolHandler.cpp +++ b/netwerk/protocol/res/ExtensionProtocolHandler.cpp @@ -390,6 +390,10 @@ nsresult ExtensionProtocolHandler::GetFlagsForURI(nsIURI* aURI, if (!policy->PrivateBrowsingAllowed()) { flags |= URI_DISALLOW_IN_PRIVATE_CONTEXT; } + } else { + // In case there is no policy, then default to treating moz-extension URIs + // as unsafe and generally only allow chrome: to load such. + flags |= URI_DANGEROUS_TO_LOAD; } *aFlags = flags;