From b680625ab08a81a9b936363f33a4557c4cf58afb Mon Sep 17 00:00:00 2001 From: Tom Schuster Date: Mon, 7 Nov 2022 17:56:23 +0000 Subject: [PATCH] Bug 1797070 - CSP: Add a basic implementation of unsafe-hashes behind a flag. r=freddyb Differential Revision: https://phabricator.services.mozilla.com/D160046 --- dom/events/EventListenerManager.cpp | 1 + .../security/nsIContentSecurityPolicy.idl | 6 ++- dom/jsurl/nsJSProtocolHandler.cpp | 43 ++++++++++++------- dom/script/ScriptLoader.cpp | 5 ++- dom/security/nsCSPContext.cpp | 19 +++++--- dom/security/nsCSPParser.cpp | 12 +++++- dom/security/nsCSPUtils.cpp | 2 +- dom/security/nsCSPUtils.h | 1 + dom/security/test/unit/test_csp_reports.js | 2 + layout/style/nsStyleUtil.cpp | 17 ++++---- modules/libpref/init/StaticPrefList.yaml | 6 +++ .../unsafe-hashes/__dir__.ini | 2 +- .../javascript_src_allowed-href.html.ini | 3 -- ...llowed-href_blank-script-src-elem.html.ini | 5 --- ...javascript_src_allowed-href_blank.html.ini | 5 --- ...cript_src_allowed-window_location.html.ini | 5 --- ...avascript_src_allowed-window_open.html.ini | 5 --- ...lers_denied_missing_unsafe_hashes.html.ini | 5 --- ...bute_denied_missing_unsafe_hashes.html.ini | 6 --- .../string-compilation-nonce-classic.html.ini | 1 + .../string-compilation-nonce-module.html.ini | 1 + 21 files changed, 83 insertions(+), 69 deletions(-) delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank-script-src-elem.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_location.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_open.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/script_event_handlers_denied_missing_unsafe_hashes.html.ini delete mode 100644 testing/web-platform/meta/content-security-policy/unsafe-hashes/style_attribute_denied_missing_unsafe_hashes.html.ini diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp index cedbab9d5709..bd11e9477b68 100644 --- a/dom/events/EventListenerManager.cpp +++ b/dom/events/EventListenerManager.cpp @@ -1044,6 +1044,7 @@ nsresult EventListenerManager::SetEventHandler(nsAtom* aName, bool allowsInlineScript = true; rv = csp->GetAllowsInline( nsIContentSecurityPolicy::SCRIPT_SRC_ATTR_DIRECTIVE, + true, // aHasUnsafeHash u""_ns, // aNonce true, // aParserCreated (true because attribute event handler) aElement, diff --git a/dom/interfaces/security/nsIContentSecurityPolicy.idl b/dom/interfaces/security/nsIContentSecurityPolicy.idl index 4b2de2ca37b5..bf691ed215c6 100644 --- a/dom/interfaces/security/nsIContentSecurityPolicy.idl +++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl @@ -125,7 +125,10 @@ interface nsIContentSecurityPolicy : nsISerializable /* * Whether this policy allows inline script or style. - * @param aContentPolicyType Either TYPE_SCRIPT or TYPE_STYLESHEET + * @param aContentPolicyType Either SCRIPT_SRC_(ELEM|ATTR)_DIRECTIVE or + * STYLE_SRC_(ELEM|ATTR)_DIRECTIVE. + * @param aHasUnsafeHash Only hash this when the 'unsafe-hashes' directive is + * also specified. * @param aNonce The nonce string to check against the policy * @param aParserCreated If the script element was created by the HTML Parser * @param aTriggeringElement The script element of the inline resource to @@ -142,6 +145,7 @@ interface nsIContentSecurityPolicy : nsISerializable * (block the rules if false). */ boolean getAllowsInline(in nsIContentSecurityPolicy_CSPDirective aDirective, + in bool aHasUnsafeHash, in AString aNonce, in boolean aParserCreated, in Element aTriggeringElement, diff --git a/dom/jsurl/nsJSProtocolHandler.cpp b/dom/jsurl/nsJSProtocolHandler.cpp index f36d44838780..06347fdfaff9 100644 --- a/dom/jsurl/nsJSProtocolHandler.cpp +++ b/dom/jsurl/nsJSProtocolHandler.cpp @@ -132,23 +132,36 @@ static nsIScriptGlobalObject* GetGlobalObject(nsIChannel* aChannel) { } static bool AllowedByCSP(nsIContentSecurityPolicy* aCSP, - const nsAString& aContentOfPseudoScript) { + const nsACString& aJavaScriptURL) { if (!aCSP) { return true; } - // javascript: is a "navigation" type, so script-src-elem applies. + // https://w3c.github.io/webappsec-csp/#should-block-navigation-request + // Step 3. If result is "Allowed", and if navigation request’s current URL’s + // scheme is javascript: + // + // Step 3.1.1.2 If directive’s inline check returns "Allowed" when executed + // upon null, "navigation" and navigation request’s current URL, skip to the + // next directive. + // + // This means /type/ is "navigation" and /source string/ is the + // "navigation request’s current URL". + // + // Per // https://w3c.github.io/webappsec-csp/#effective-directive-for-inline-check + // type "navigation" maps to the effective directive script-src-elem. bool allowsInlineScript = true; nsresult rv = aCSP->GetAllowsInline(nsIContentSecurityPolicy::SCRIPT_SRC_ELEM_DIRECTIVE, - u""_ns, // aNonce - true, // aParserCreated - nullptr, // aElement, - nullptr, // nsICSPEventListener - aContentOfPseudoScript, // aContent - 0, // aLineNumber - 0, // aColumnNumber + true, // aHasUnsafeHash + u""_ns, // aNonce + true, // aParserCreated + nullptr, // aElement, + nullptr, // nsICSPEventListener + NS_ConvertASCIItoUTF16(aJavaScriptURL), // aContent + 0, // aLineNumber + 0, // aColumnNumber &allowsInlineScript); return (NS_SUCCEEDED(rv) && allowsInlineScript); @@ -210,11 +223,7 @@ nsresult nsJSThunk::EvaluateScript( // once we have determined the target document. nsCOMPtr csp = loadInfo->GetCspToInherit(); - nsAutoCString script(mScript); - // Unescape the script - NS_UnescapeURL(script); - - if (!AllowedByCSP(csp, NS_ConvertASCIItoUTF16(script))) { + if (!AllowedByCSP(csp, mURL)) { return NS_ERROR_DOM_RETVAL_UNDEFINED; } @@ -264,7 +273,7 @@ nsresult nsJSThunk::EvaluateScript( // against if the triggering principal is system. if (targetDoc->NodePrincipal()->Subsumes(loadInfo->TriggeringPrincipal())) { nsCOMPtr targetCSP = targetDoc->GetCsp(); - if (!AllowedByCSP(targetCSP, NS_ConvertASCIItoUTF16(script))) { + if (!AllowedByCSP(targetCSP, mURL)) { return NS_ERROR_DOM_RETVAL_UNDEFINED; } } @@ -305,6 +314,10 @@ nsresult nsJSThunk::EvaluateScript( return NS_ERROR_DOM_SECURITY_ERR; } + nsAutoCString script(mScript); + // Unescape the script + NS_UnescapeURL(script); + JS::Rooted v(cx, JS::UndefinedValue()); // Finally, we have everything needed to evaluate the expression. JS::CompileOptions options(cx); diff --git a/dom/script/ScriptLoader.cpp b/dom/script/ScriptLoader.cpp index 2637f2f32e06..b966391396fc 100644 --- a/dom/script/ScriptLoader.cpp +++ b/dom/script/ScriptLoader.cpp @@ -801,8 +801,9 @@ static bool CSPAllowsInlineScript(nsIScriptElement* aElement, bool allowInlineScript = false; nsresult rv = csp->GetAllowsInline( - nsIContentSecurityPolicy::SCRIPT_SRC_ELEM_DIRECTIVE, nonce, parserCreated, - scriptContent, nullptr /* nsICSPEventListener */, u""_ns, + nsIContentSecurityPolicy::SCRIPT_SRC_ELEM_DIRECTIVE, + false /* aHasUnsafeHash */, nonce, parserCreated, scriptContent, + nullptr /* nsICSPEventListener */, u""_ns, aElement->GetScriptLineNumber(), aElement->GetScriptColumnNumber(), &allowInlineScript); return NS_SUCCEEDED(rv) && allowInlineScript; diff --git a/dom/security/nsCSPContext.cpp b/dom/security/nsCSPContext.cpp index b76721f766ce..7b2a5c00ce41 100644 --- a/dom/security/nsCSPContext.cpp +++ b/dom/security/nsCSPContext.cpp @@ -572,8 +572,9 @@ void nsCSPContext::reportInlineViolation( } NS_IMETHODIMP -nsCSPContext::GetAllowsInline(CSPDirective aDirective, const nsAString& aNonce, - bool aParserCreated, Element* aTriggeringElement, +nsCSPContext::GetAllowsInline(CSPDirective aDirective, bool aHasUnsafeHash, + const nsAString& aNonce, bool aParserCreated, + Element* aTriggeringElement, nsICSPEventListener* aCSPEventListener, const nsAString& aContentOfPseudoScript, uint32_t aLineNumber, uint32_t aColumnNumber, @@ -616,12 +617,20 @@ nsCSPContext::GetAllowsInline(CSPDirective aDirective, const nsAString& aNonce, element->GetScriptText(content); } } - if (content.IsEmpty()) { content = aContentOfPseudoScript; } - allowed = - mPolicies[i]->allows(aDirective, CSP_HASH, content, aParserCreated); + + // Per https://w3c.github.io/webappsec-csp/#match-element-to-source-list + // Step 5. If type is "script" or "style", or unsafe-hashes flag is true: + // + // aHasUnsafeHash is true for event handlers (type "script attribute"), + // style= attributes (type "style attribute") and the javascript: protocol. + if (!aHasUnsafeHash || mPolicies[i]->allows(aDirective, CSP_UNSAFE_HASHES, + u""_ns, aParserCreated)) { + allowed = + mPolicies[i]->allows(aDirective, CSP_HASH, content, aParserCreated); + } if (!allowed) { // policy is violoated: deny the load unless policy is report only and diff --git a/dom/security/nsCSPParser.cpp b/dom/security/nsCSPParser.cpp index 3e3ac2619bc0..e71f7de6dd00 100644 --- a/dom/security/nsCSPParser.cpp +++ b/dom/security/nsCSPParser.cpp @@ -454,6 +454,11 @@ nsCSPBaseSrc* nsCSPParser::keywordSource() { return new nsCSPKeywordSrc(CSP_UTF16KeywordToEnum(mCurToken)); } + if (StaticPrefs::security_csp_unsafe_hashes_enabled() && + CSP_IsKeyword(mCurToken, CSP_UNSAFE_HASHES)) { + return new nsCSPKeywordSrc(CSP_UTF16KeywordToEnum(mCurToken)); + } + if (CSP_IsKeyword(mCurToken, CSP_UNSAFE_ALLOW_REDIRECTS)) { if (!CSP_IsDirective(mCurDir[0], nsIContentSecurityPolicy::NAVIGATE_TO_DIRECTIVE)) { @@ -1070,10 +1075,13 @@ void nsCSPParser::directive() { nsAutoString srcStr; srcs[i]->toString(srcStr); // Even though we invalidate all of the srcs internally, we don't want to - // log messages for the srcs: (1) strict-dynamic, (2) unsafe-inline, (3) - // nonces, and (4) hashes + // log messages for the srcs: 'strict-dynamic', 'unsafe-inline', + // 'unsafe-hashes', nonces, and hashes, because those still apply even + // with 'strict-dynamic'. + // TODO the comment seems wrong 'unsafe-eval' vs 'unsafe-inline'. if (!srcStr.EqualsASCII(CSP_EnumToUTF8Keyword(CSP_STRICT_DYNAMIC)) && !srcStr.EqualsASCII(CSP_EnumToUTF8Keyword(CSP_UNSAFE_EVAL)) && + !srcStr.EqualsASCII(CSP_EnumToUTF8Keyword(CSP_UNSAFE_HASHES)) && !StringBeginsWith( srcStr, nsDependentString(CSP_EnumToUTF16Keyword(CSP_NONCE))) && !StringBeginsWith(srcStr, u"'sha"_ns)) { diff --git a/dom/security/nsCSPUtils.cpp b/dom/security/nsCSPUtils.cpp index 2d3fbf6d2488..11aa1d8b6232 100644 --- a/dom/security/nsCSPUtils.cpp +++ b/dom/security/nsCSPUtils.cpp @@ -875,7 +875,7 @@ bool nsCSPKeywordSrc::allows(enum CSPKeyword aKeyword, "%s", CSP_EnumToUTF8Keyword(aKeyword), NS_ConvertUTF16toUTF8(aHashOrNonce).get(), - mInvalidated ? "yes" : "false")); + mInvalidated ? "true" : "false")); if (mInvalidated) { // only 'self', 'report-sample' and 'unsafe-inline' are keywords that can be diff --git a/dom/security/nsCSPUtils.h b/dom/security/nsCSPUtils.h index 1dc0cdbfd6f5..dd3ea9a1c493 100644 --- a/dom/security/nsCSPUtils.h +++ b/dom/security/nsCSPUtils.h @@ -116,6 +116,7 @@ inline CSPDirective CSP_StringToCSPDirective(const nsAString& aDir) { MACRO(CSP_SELF, "'self'") \ MACRO(CSP_UNSAFE_INLINE, "'unsafe-inline'") \ MACRO(CSP_UNSAFE_EVAL, "'unsafe-eval'") \ + MACRO(CSP_UNSAFE_HASHES, "'unsafe-hashes'") \ MACRO(CSP_NONE, "'none'") \ MACRO(CSP_NONCE, "'nonce-") \ MACRO(CSP_REPORT_SAMPLE, "'report-sample'") \ diff --git a/dom/security/test/unit/test_csp_reports.js b/dom/security/test/unit/test_csp_reports.js index b30002b77d21..3df7a001d5c7 100644 --- a/dom/security/test/unit/test_csp_reports.js +++ b/dom/security/test/unit/test_csp_reports.js @@ -129,6 +129,7 @@ function run_test() { let inlineOK = true; inlineOK = csp.getAllowsInline( Ci.nsIContentSecurityPolicy.SCRIPT_SRC_ELEM_DIRECTIVE, + false, // aHasUnsafeHash "", // aNonce false, // aParserCreated null, // aTriggeringElement @@ -206,6 +207,7 @@ function run_test() { let inlineOK = true; inlineOK = csp.getAllowsInline( Ci.nsIContentSecurityPolicy.SCRIPT_SRC_ELEM_DIRECTIVE, + false, // aHasUnsafeHash "", // aNonce false, // aParserCreated null, // aTriggeringElement diff --git a/layout/style/nsStyleUtil.cpp b/layout/style/nsStyleUtil.cpp index 305d9917ac33..8faa926acd9b 100644 --- a/layout/style/nsStyleUtil.cpp +++ b/layout/style/nsStyleUtil.cpp @@ -306,12 +306,11 @@ bool nsStyleUtil::CSPAllowsInlineStyle( return true; } - nsIContentSecurityPolicy::CSPDirective directive = - nsIContentSecurityPolicy::STYLE_SRC_ATTR_DIRECTIVE; + bool isStyleElement = false; // query the nonce nsAutoString nonce; if (aElement && aElement->NodeInfo()->NameAtom() == nsGkAtoms::style) { - directive = nsIContentSecurityPolicy::STYLE_SRC_ELEM_DIRECTIVE; + isStyleElement = true; nsString* cspNonce = static_cast(aElement->GetProperty(nsGkAtoms::nonce)); if (cspNonce) { @@ -320,11 +319,13 @@ bool nsStyleUtil::CSPAllowsInlineStyle( } bool allowInlineStyle = true; - rv = csp->GetAllowsInline(directive, nonce, - false, // aParserCreated only applies to scripts - aElement, nullptr, // nsICSPEventListener - aStyleText, aLineNumber, aColumnNumber, - &allowInlineStyle); + rv = csp->GetAllowsInline( + isStyleElement ? nsIContentSecurityPolicy::STYLE_SRC_ELEM_DIRECTIVE + : nsIContentSecurityPolicy::STYLE_SRC_ATTR_DIRECTIVE, + !isStyleElement /* aHasUnsafeHash */, nonce, + false, // aParserCreated only applies to scripts + aElement, nullptr, // nsICSPEventListener + aStyleText, aLineNumber, aColumnNumber, &allowInlineStyle); NS_ENSURE_SUCCESS(rv, false); return allowInlineStyle; diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index b9c5d8a0aa32..31183c28a867 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -12859,6 +12859,12 @@ value: true mirror: always +# unsafe-hashes source keyword +- name: security.csp.unsafe-hashes.enabled + type: bool + value: false + mirror: always + # The script-src-attr and script-src-elem directive - name: security.csp.script-src-attr-elem.enabled type: bool diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/__dir__.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/__dir__.ini index 18dd33687ade..ecd25ae52a58 100644 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/__dir__.ini +++ b/testing/web-platform/meta/content-security-policy/unsafe-hashes/__dir__.ini @@ -1 +1 @@ -prefs: [dom.targetBlankNoOpener.enabled:false] +prefs: [dom.targetBlankNoOpener.enabled:false, security.csp.unsafe-hashes.enabled:true] diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href.html.ini deleted file mode 100644 index 20b5ed315ddb..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[javascript_src_allowed-href.html] - [javascript: navigation using should be allowed] - expected: FAIL diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank-script-src-elem.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank-script-src-elem.html.ini deleted file mode 100644 index 53c529cd5073..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank-script-src-elem.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[javascript_src_allowed-href_blank-script-src-elem.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [javascript: navigation using should be allowed] - expected: FAIL diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank.html.ini deleted file mode 100644 index ee6757f0080d..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-href_blank.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[javascript_src_allowed-href_blank.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [javascript: navigation using should be allowed] - expected: FAIL diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_location.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_location.html.ini deleted file mode 100644 index 258635b5bec4..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_location.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[javascript_src_allowed-window_location.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [Test that the javascript: src is allowed to run] - expected: FAIL diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_open.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_open.html.ini deleted file mode 100644 index 98f511fee3eb..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/javascript_src_allowed-window_open.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[javascript_src_allowed-window_open.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [Test that the javascript: src is allowed to run] - expected: FAIL diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/script_event_handlers_denied_missing_unsafe_hashes.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/script_event_handlers_denied_missing_unsafe_hashes.html.ini deleted file mode 100644 index f692bd56f56f..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/script_event_handlers_denied_missing_unsafe_hashes.html.ini +++ /dev/null @@ -1,5 +0,0 @@ -[script_event_handlers_denied_missing_unsafe_hashes.html] - expected: TIMEOUT - [Test that the inline event handler is not allowed to run] - expected: NOTRUN - diff --git a/testing/web-platform/meta/content-security-policy/unsafe-hashes/style_attribute_denied_missing_unsafe_hashes.html.ini b/testing/web-platform/meta/content-security-policy/unsafe-hashes/style_attribute_denied_missing_unsafe_hashes.html.ini deleted file mode 100644 index abf6f0304f89..000000000000 --- a/testing/web-platform/meta/content-security-policy/unsafe-hashes/style_attribute_denied_missing_unsafe_hashes.html.ini +++ /dev/null @@ -1,6 +0,0 @@ -implementation-status: backlog -[style_attribute_denied_missing_unsafe_hashes.html] - expected: TIMEOUT - [Test that the inline style attribute is blocked] - expected: NOTRUN - diff --git a/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-classic.html.ini b/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-classic.html.ini index ae4070464424..a864045cbb71 100644 --- a/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-classic.html.ini +++ b/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-classic.html.ini @@ -1,3 +1,4 @@ [string-compilation-nonce-classic.html] + prefs: [security.csp.unsafe-hashes.enabled:true] expected: if (os == "android") and fission: [OK, TIMEOUT] diff --git a/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-module.html.ini b/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-module.html.ini index 3364d46d57c7..6746b0e36319 100644 --- a/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-module.html.ini +++ b/testing/web-platform/meta/html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-nonce-module.html.ini @@ -1,3 +1,4 @@ [string-compilation-nonce-module.html] + prefs: [security.csp.unsafe-hashes.enabled:true] expected: if (os == "android") and fission: [OK, TIMEOUT]