From ee1cc488f28398fc1cb7a0b743f1e94404293941 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 12 Dec 2019 16:41:19 +0000 Subject: [PATCH] Bug 1602483 part 2. Add a window id argument to CheckLoadURIWithPrincipal. r=ckerschb Differential Revision: https://phabricator.services.mozilla.com/D56428 --HG-- rename : devtools/client/webconsole/test/browser/browser_webconsole_same_origin_errors.js => devtools/client/webconsole/test/browser/browser_webconsole_checkloaduri_errors.js rename : devtools/client/webconsole/test/browser/test-same-origin-required-load.html => devtools/client/webconsole/test/browser/test-checkloaduri-failure.html extra : moz-landing-system : lando --- caps/nsIScriptSecurityManager.idl | 7 ++- caps/nsScriptSecurityManager.cpp | 44 ++++++++++++------- caps/nsScriptSecurityManager.h | 3 +- .../webconsole/test/browser/browser.ini | 2 + .../browser_webconsole_checkloaduri_errors.js | 29 ++++++++++++ .../browser/test-checkloaduri-failure.html | 23 ++++++++++ docshell/base/nsDocShell.cpp | 3 +- dom/base/LocationBase.cpp | 7 ++- dom/base/nsContentUtils.cpp | 6 ++- dom/base/nsFrameLoader.cpp | 3 +- dom/base/nsTreeSanitizer.cpp | 5 ++- dom/html/HTMLFormElement.cpp | 3 +- dom/security/nsContentSecurityManager.cpp | 5 ++- dom/xml/nsXMLContentSink.cpp | 5 ++- dom/xul/nsXULContentSink.cpp | 2 +- netwerk/protocol/http/nsCORSListenerProxy.cpp | 4 +- 16 files changed, 118 insertions(+), 33 deletions(-) create mode 100644 devtools/client/webconsole/test/browser/browser_webconsole_checkloaduri_errors.js create mode 100644 devtools/client/webconsole/test/browser/test-checkloaduri-failure.html diff --git a/caps/nsIScriptSecurityManager.idl b/caps/nsIScriptSecurityManager.idl index ff4dc48071f6..ad482859df80 100644 --- a/caps/nsIScriptSecurityManager.idl +++ b/caps/nsIScriptSecurityManager.idl @@ -108,10 +108,15 @@ interface nsIScriptSecurityManager : nsISupports * @param aPrincipal the principal identifying the actor causing the load * @param uri the URI that is being loaded * @param flags the permission set, see above + * @param innerWindowID the window ID for error reporting. If this is 0 + * (which happens automatically if it's not passed from JS), errors + * will only appear in the browser console, not window-associated + * consoles like the web console. */ void checkLoadURIWithPrincipal(in nsIPrincipal aPrincipal, in nsIURI uri, - in unsigned long flags); + in unsigned long flags, + [optional] in unsigned long long innerWindowID); /** * Similar to checkLoadURIWithPrincipal but there are two differences: diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp index 725a99e575c9..2f4166df1c51 100644 --- a/caps/nsScriptSecurityManager.cpp +++ b/caps/nsScriptSecurityManager.cpp @@ -518,8 +518,10 @@ nsScriptSecurityManager::CheckLoadURIFromScript(JSContext* cx, nsIURI* aURI) { // Get principal of currently executing script. MOZ_ASSERT(cx == nsContentUtils::GetCurrentJSContext()); nsIPrincipal* principal = nsContentUtils::SubjectPrincipal(); - nsresult rv = CheckLoadURIWithPrincipal(principal, aURI, - nsIScriptSecurityManager::STANDARD); + nsresult rv = CheckLoadURIWithPrincipal( + // Passing 0 for the window ID here is OK, because we will report a + // script-visible exception anyway. + principal, aURI, nsIScriptSecurityManager::STANDARD, 0); if (NS_SUCCEEDED(rv)) { // OK to load return NS_OK; @@ -584,7 +586,8 @@ static bool EqualOrSubdomain(nsIURI* aProbeArg, nsIURI* aBase) { NS_IMETHODIMP nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal, nsIURI* aTargetURI, - uint32_t aFlags) { + uint32_t aFlags, + uint64_t aInnerWindowID) { MOZ_ASSERT(aPrincipal, "CheckLoadURIWithPrincipal must have a principal"); // If someone passes a flag that we don't understand, we should @@ -622,7 +625,8 @@ nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal, if (basePrin->Is()) { auto expanded = basePrin->As(); for (auto& prin : expanded->AllowList()) { - nsresult rv = CheckLoadURIWithPrincipal(prin, aTargetURI, aFlags); + nsresult rv = + CheckLoadURIWithPrincipal(prin, aTargetURI, aFlags, aInnerWindowID); if (NS_SUCCEEDED(rv)) { // Allow access if it succeeded with one of the allowlisted principals return NS_OK; @@ -673,11 +677,15 @@ nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal, // access: rv = CheckLoadURIFlags( sourceURI, aTargetURI, sourceBaseURI, targetBaseURI, aFlags, - aPrincipal->OriginAttributesRef().mPrivateBrowsingId > 0); + aPrincipal->OriginAttributesRef().mPrivateBrowsingId > 0, + aInnerWindowID); NS_ENSURE_SUCCESS(rv, rv); // Check the principal is allowed to load the target. - // Unfortunately we don't have a window id to work with here... - return aPrincipal->CheckMayLoadWithReporting(targetBaseURI, false, 0); + if (aFlags & nsIScriptSecurityManager::DONT_REPORT_ERRORS) { + return aPrincipal->CheckMayLoad(targetBaseURI, false); + } + return aPrincipal->CheckMayLoadWithReporting(targetBaseURI, false, + aInnerWindowID); } //-- get the source scheme @@ -794,7 +802,8 @@ nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal, if (!schemesMatch || (denySameSchemeLinks && !isSamePage)) { return CheckLoadURIFlags( currentURI, currentOtherURI, sourceBaseURI, targetBaseURI, aFlags, - aPrincipal->OriginAttributesRef().mPrivateBrowsingId > 0); + aPrincipal->OriginAttributesRef().mPrivateBrowsingId > 0, + aInnerWindowID); } // Otherwise... check if we can nest another level: nsCOMPtr nestedURI = do_QueryInterface(currentURI); @@ -828,7 +837,8 @@ nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal* aPrincipal, */ nsresult nsScriptSecurityManager::CheckLoadURIFlags( nsIURI* aSourceURI, nsIURI* aTargetURI, nsIURI* aSourceBaseURI, - nsIURI* aTargetBaseURI, uint32_t aFlags, bool aFromPrivateWindow) { + nsIURI* aTargetBaseURI, uint32_t aFlags, bool aFromPrivateWindow, + uint64_t aInnerWindowID) { // Note that the order of policy checks here is very important! // We start from most restrictive and work our way down. bool reportErrors = !(aFlags & nsIScriptSecurityManager::DONT_REPORT_ERRORS); @@ -844,7 +854,8 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags( if (NS_FAILED(rv)) { // Deny access, since the origin principal is not system if (reportErrors) { - ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow); + ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow, + aInnerWindowID); } return rv; } @@ -856,7 +867,8 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags( aTargetURI, nsIProtocolHandler::URI_DISALLOW_IN_PRIVATE_CONTEXT); if (NS_FAILED(rv)) { if (reportErrors) { - ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow); + ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow, + aInnerWindowID); } return rv; } @@ -923,7 +935,8 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags( } if (reportErrors) { - ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow); + ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow, + aInnerWindowID); } return NS_ERROR_DOM_BAD_URI; } @@ -948,7 +961,8 @@ nsresult nsScriptSecurityManager::CheckLoadURIFlags( // Nothing else. if (reportErrors) { - ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow); + ReportError(errorTag, aSourceURI, aTargetURI, aFromPrivateWindow, + aInnerWindowID); } return NS_ERROR_DOM_BAD_URI; } @@ -1050,7 +1064,7 @@ nsScriptSecurityManager::CheckLoadURIStrWithPrincipal( rv = NS_NewURI(getter_AddRefs(target), aTargetURIStr); NS_ENSURE_SUCCESS(rv, rv); - rv = CheckLoadURIWithPrincipal(aPrincipal, target, aFlags); + rv = CheckLoadURIWithPrincipal(aPrincipal, target, aFlags, 0); if (rv == NS_ERROR_DOM_BAD_URI) { // Don't warn because NS_ERROR_DOM_BAD_URI is one of the expected // return values. @@ -1082,7 +1096,7 @@ nsScriptSecurityManager::CheckLoadURIStrWithPrincipal( getter_AddRefs(target)); NS_ENSURE_SUCCESS(rv, rv); - rv = CheckLoadURIWithPrincipal(aPrincipal, target, aFlags); + rv = CheckLoadURIWithPrincipal(aPrincipal, target, aFlags, 0); if (rv == NS_ERROR_DOM_BAD_URI) { // Don't warn because NS_ERROR_DOM_BAD_URI is one of the expected // return values. diff --git a/caps/nsScriptSecurityManager.h b/caps/nsScriptSecurityManager.h index 01954679bef4..5b0b9af29a0b 100644 --- a/caps/nsScriptSecurityManager.h +++ b/caps/nsScriptSecurityManager.h @@ -102,7 +102,8 @@ class nsScriptSecurityManager final : public nsIScriptSecurityManager { nsresult CheckLoadURIFlags(nsIURI* aSourceURI, nsIURI* aTargetURI, nsIURI* aSourceBaseURI, nsIURI* aTargetBaseURI, - uint32_t aFlags, bool aFromPrivateWindow); + uint32_t aFlags, bool aFromPrivateWindow, + uint64_t aInnerWindowID); // Returns the file URI allowlist, initializing it if it has not been // initialized. diff --git a/devtools/client/webconsole/test/browser/browser.ini b/devtools/client/webconsole/test/browser/browser.ini index a789714f02f8..576cccd2081a 100644 --- a/devtools/client/webconsole/test/browser/browser.ini +++ b/devtools/client/webconsole/test/browser/browser.ini @@ -40,6 +40,7 @@ support-files = test-iframe-child.html test-iframe-parent.html test-certificate-messages.html + test-checkloaduri-failure.html test-click-function-to-source.html test-click-function-to-source.js test-click-function-to-mapped-source.html @@ -309,6 +310,7 @@ tags = mcb [browser_webconsole_cd_iframe.js] [browser_webconsole_certificate_messages.js] skip-if = fission +[browser_webconsole_checkloaduri_errors.js] [browser_webconsole_clear_cache.js] [browser_webconsole_click_function_to_source.js] [browser_webconsole_click_function_to_mapped_source.js] diff --git a/devtools/client/webconsole/test/browser/browser_webconsole_checkloaduri_errors.js b/devtools/client/webconsole/test/browser/browser_webconsole_checkloaduri_errors.js new file mode 100644 index 000000000000..cf9d9dccf2a9 --- /dev/null +++ b/devtools/client/webconsole/test/browser/browser_webconsole_checkloaduri_errors.js @@ -0,0 +1,29 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +// Ensure that same-origin errors are logged to the console. + +"use strict"; + +const TEST_URI = + "http://example.com/browser/devtools/client/webconsole/test/browser/test-checkloaduri-failure.html"; + +add_task(async function() { + const hud = await openNewTabAndConsole(TEST_URI); + + const targetURL = "file:///something-weird"; + const onErrorMessage = waitForMessage(hud, "may not load or link"); + SpecialPowers.spawn(gBrowser.selectedBrowser, [targetURL], url => { + XPCNativeWrapper.unwrap(content).testImage(url); + }); + const message = await onErrorMessage; + const node = message.node; + ok( + node.classList.contains("error"), + "The message has the expected classname" + ); + ok( + node.textContent.includes(targetURL), + "The message is about the thing we were expecting" + ); +}); diff --git a/devtools/client/webconsole/test/browser/test-checkloaduri-failure.html b/devtools/client/webconsole/test/browser/test-checkloaduri-failure.html new file mode 100644 index 000000000000..a242c549da0f --- /dev/null +++ b/devtools/client/webconsole/test/browser/test-checkloaduri-failure.html @@ -0,0 +1,23 @@ + + + + + + Test loads that fail checkLoadURI + + + + + diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 437eab281013..5bebd54817fd 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -5676,7 +5676,8 @@ nsresult nsDocShell::SetupRefreshURIFromHeader(nsIURI* aBaseURI, if (NS_SUCCEEDED(rv)) { rv = securityManager->CheckLoadURIWithPrincipal( aPrincipal, uri, - nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT); + nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT, + aInnerWindowID); if (NS_SUCCEEDED(rv)) { bool isjs = true; diff --git a/dom/base/LocationBase.cpp b/dom/base/LocationBase.cpp index 055a52bdea2a..d7e8346987ef 100644 --- a/dom/base/LocationBase.cpp +++ b/dom/base/LocationBase.cpp @@ -39,9 +39,12 @@ already_AddRefed LocationBase::CheckURL( return nullptr; } - // Check to see if URI is allowed. + // Check to see if URI is allowed. We're not going to worry about a + // window ID here because it's not 100% clear which window's id we + // would want, and we're throwing a content-visible exception + // anyway. nsresult rv = ssm->CheckLoadURIWithPrincipal( - &aSubjectPrincipal, aURI, nsIScriptSecurityManager::STANDARD); + &aSubjectPrincipal, aURI, nsIScriptSecurityManager::STANDARD, 0); if (NS_WARN_IF(NS_FAILED(rv))) { nsAutoCString spec; aURI->GetSpec(spec); diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index c1b9982d9ddb..339d3fd2e921 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -3160,7 +3160,8 @@ bool nsContentUtils::CanLoadImage(nsIURI* aURI, nsINode* aNode, // from anywhere. This allows editor to insert images from file:// // into documents that are being edited. rv = sSecurityManager->CheckLoadURIWithPrincipal( - aLoadingPrincipal, aURI, nsIScriptSecurityManager::ALLOW_CHROME); + aLoadingPrincipal, aURI, nsIScriptSecurityManager::ALLOW_CHROME, + aLoadingDocument->InnerWindowID()); if (NS_FAILED(rv)) { return false; } @@ -5100,7 +5101,8 @@ void nsContentUtils::TriggerLink(nsIContent* aContent, nsIURI* aLinkURI, if (sSecurityManager) { uint32_t flag = static_cast(nsIScriptSecurityManager::STANDARD); proceed = sSecurityManager->CheckLoadURIWithPrincipal( - aContent->NodePrincipal(), aLinkURI, flag); + aContent->NodePrincipal(), aLinkURI, flag, + aContent->OwnerDoc()->InnerWindowID()); } // Only pass off the click event if the script security manager says it's ok. diff --git a/dom/base/nsFrameLoader.cpp b/dom/base/nsFrameLoader.cpp index 5922c21b8399..c09c395c61d8 100644 --- a/dom/base/nsFrameLoader.cpp +++ b/dom/base/nsFrameLoader.cpp @@ -693,7 +693,8 @@ nsresult nsFrameLoader::CheckURILoad(nsIURI* aURI, // Check if we are allowed to load absURL nsresult rv = secMan->CheckLoadURIWithPrincipal( - principal, aURI, nsIScriptSecurityManager::STANDARD); + principal, aURI, nsIScriptSecurityManager::STANDARD, + mOwnerContent->OwnerDoc()->InnerWindowID()); if (NS_FAILED(rv)) { return rv; // We're not } diff --git a/dom/base/nsTreeSanitizer.cpp b/dom/base/nsTreeSanitizer.cpp index 483b9589503b..7ff11ef0be8e 100644 --- a/dom/base/nsTreeSanitizer.cpp +++ b/dom/base/nsTreeSanitizer.cpp @@ -1251,10 +1251,11 @@ bool nsTreeSanitizer::SanitizeURL(mozilla::dom::Element* aElement, // in case someone goofs with these in the future, let's drop them. rv = NS_ERROR_FAILURE; } else { - rv = secMan->CheckLoadURIWithPrincipal(sNullPrincipal, attrURI, flags); + rv = secMan->CheckLoadURIWithPrincipal(sNullPrincipal, attrURI, flags, + 0); } } else { - rv = secMan->CheckLoadURIWithPrincipal(sNullPrincipal, attrURI, flags); + rv = secMan->CheckLoadURIWithPrincipal(sNullPrincipal, attrURI, flags, 0); } } if (NS_FAILED(rv)) { diff --git a/dom/html/HTMLFormElement.cpp b/dom/html/HTMLFormElement.cpp index 2985c364fda8..327b0b816894 100644 --- a/dom/html/HTMLFormElement.cpp +++ b/dom/html/HTMLFormElement.cpp @@ -1659,7 +1659,8 @@ nsresult HTMLFormElement::GetActionURL(nsIURI** aActionURL, nsIScriptSecurityManager* securityManager = nsContentUtils::GetSecurityManager(); rv = securityManager->CheckLoadURIWithPrincipal( - NodePrincipal(), actionURL, nsIScriptSecurityManager::STANDARD); + NodePrincipal(), actionURL, nsIScriptSecurityManager::STANDARD, + OwnerDoc()->InnerWindowID()); NS_ENSURE_SUCCESS(rv, rv); // Potentially the page uses the CSP directive 'upgrade-insecure-requests'. In diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp index daa7f8519cb8..a0e0686b98b0 100644 --- a/dom/security/nsContentSecurityManager.cpp +++ b/dom/security/nsContentSecurityManager.cpp @@ -309,7 +309,8 @@ static nsresult DoCheckLoadURIChecks(nsIURI* aURI, nsILoadInfo* aLoadInfo) { // the LoadingPrincipal when SEC_ALLOW_CROSS_ORIGIN_* security flags are set, // to allow, e.g. user stylesheets to load chrome:// URIs. return nsContentUtils::GetSecurityManager()->CheckLoadURIWithPrincipal( - aLoadInfo->TriggeringPrincipal(), aURI, aLoadInfo->CheckLoadURIFlags()); + aLoadInfo->TriggeringPrincipal(), aURI, aLoadInfo->CheckLoadURIFlags(), + aLoadInfo->GetInnerWindowID()); } static bool URIHasFlags(nsIURI* aURI, uint32_t aURIFlags) { @@ -955,7 +956,7 @@ nsContentSecurityManager::AsyncOnChannelRedirect( nsIScriptSecurityManager::LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT | nsIScriptSecurityManager::DISALLOW_SCRIPT; rv = nsContentUtils::GetSecurityManager()->CheckLoadURIWithPrincipal( - oldPrincipal, newURI, flags); + oldPrincipal, newURI, flags, loadInfo->GetInnerWindowID()); NS_ENSURE_SUCCESS(rv, rv); aCb->OnRedirectVerifyCallback(NS_OK); diff --git a/dom/xml/nsXMLContentSink.cpp b/dom/xml/nsXMLContentSink.cpp index 759744704bfd..ffb4f95f64e2 100644 --- a/dom/xml/nsXMLContentSink.cpp +++ b/dom/xml/nsXMLContentSink.cpp @@ -685,8 +685,9 @@ nsresult nsXMLContentSink::MaybeProcessXSLTLink( // Do security check nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager(); - rv = secMan->CheckLoadURIWithPrincipal( - mDocument->NodePrincipal(), url, nsIScriptSecurityManager::ALLOW_CHROME); + rv = secMan->CheckLoadURIWithPrincipal(mDocument->NodePrincipal(), url, + nsIScriptSecurityManager::ALLOW_CHROME, + mDocument->InnerWindowID()); NS_ENSURE_SUCCESS(rv, NS_OK); nsCOMPtr secCheckLoadInfo = diff --git a/dom/xul/nsXULContentSink.cpp b/dom/xul/nsXULContentSink.cpp index 360cdf214c92..838ff5cd4793 100644 --- a/dom/xul/nsXULContentSink.cpp +++ b/dom/xul/nsXULContentSink.cpp @@ -759,7 +759,7 @@ nsresult XULContentSinkImpl::OpenScript(const char16_t** aAttributes, if (NS_SUCCEEDED(rv)) { rv = mSecMan->CheckLoadURIWithPrincipal( doc->NodePrincipal(), script->mSrcURI, - nsIScriptSecurityManager::ALLOW_CHROME); + nsIScriptSecurityManager::ALLOW_CHROME, doc->InnerWindowID()); } } } diff --git a/netwerk/protocol/http/nsCORSListenerProxy.cpp b/netwerk/protocol/http/nsCORSListenerProxy.cpp index 7ea193162a61..be816d76507e 100644 --- a/netwerk/protocol/http/nsCORSListenerProxy.cpp +++ b/netwerk/protocol/http/nsCORSListenerProxy.cpp @@ -904,12 +904,12 @@ nsresult nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel, // Check that the uri is ok to load uint32_t flags = loadInfo->CheckLoadURIFlags(); rv = nsContentUtils::GetSecurityManager()->CheckLoadURIWithPrincipal( - mRequestingPrincipal, uri, flags); + mRequestingPrincipal, uri, flags, loadInfo->GetInnerWindowID()); NS_ENSURE_SUCCESS(rv, rv); if (originalURI != uri) { rv = nsContentUtils::GetSecurityManager()->CheckLoadURIWithPrincipal( - mRequestingPrincipal, originalURI, flags); + mRequestingPrincipal, originalURI, flags, loadInfo->GetInnerWindowID()); NS_ENSURE_SUCCESS(rv, rv); }