Implementation of W3C compliant CSP script-src nonce.

Source/core/page/ContentSecurityPolicy.cpp

Index: Source/core/page/ContentSecurityPolicy.cpp
diff --git a/Source/core/page/ContentSecurityPolicy.cpp b/Source/core/page/ContentSecurityPolicy.cpp
index 7aea7903bb3f7f7e2889577aa5ee2624d6ceeace..94bf785f9717098a6bb18f29a4d3faa3320168ea 100644
--- a/Source/core/page/ContentSecurityPolicy.cpp
+++ b/Source/core/page/ContentSecurityPolicy.cpp
@@ -69,7 +69,7 @@ bool isDirectiveValueCharacter(UChar c)
 
 bool isNonceCharacter(UChar c)
 {
-    return (c >= 0x21 && c <= 0x7e) && c != ',' && c != ';'; // VCHAR - ',' - ';'
+    return isASCIIAlphanumeric(c);
 }
 
 bool isSourceCharacter(UChar c)
@@ -320,6 +320,7 @@ public:
     bool matches(const KURL&);
     bool allowInline() const { return m_allowInline; }
     bool allowEval() const { return m_allowEval; }
+    bool allowNonce(const String& nonce) const { return !nonce.isNull() && m_nonces.contains(nonce); }
 
 private:
     void parse(const UChar* begin, const UChar* end);
@@ -334,6 +335,7 @@ private:
     void addSourceStar();
     void addSourceUnsafeInline();
     void addSourceUnsafeEval();
+    void addSourceNonce(String& nonce);

[abarth-chromium, 2013/05/14 05:58:16]

const String&   <-- we generally use const references unless the parameter is an
out parameter.

[jww, 2013/05/14 20:49:30]

Done.

 
     ContentSecurityPolicy* m_policy;
     Vector<CSPSource> m_list;
@@ -341,6 +343,8 @@ private:
     bool m_allowStar;
     bool m_allowInline;
     bool m_allowEval;
+    // Set of nonces that indicate whitelisted scripts

[abarth-chromium, 2013/05/14 05:58:16]

I'd skip this comment.  CSPSourceList isn't specific to scripts, so this comment
is going to be wrong once we add nonce support to style-src.

[jww, 2013/05/14 20:49:30]

Done.

+    HashSet<String> m_nonces;
 };
 
 CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String& directiveName)
@@ -445,7 +449,41 @@ bool CSPSourceList::parseSource(const UChar* begin, const UChar* end,
         return true;
     }
 
-    const UChar* position = begin;
+    const UChar* position;

[abarth-chromium, 2013/05/14 05:58:16]

There's no reason to have this variable declared here.  We should move it down
to line 486 so that you can declare and initialize it at the same time.

[jww, 2013/05/14 20:49:30]

I declared it up here because I actually use it in the 'if' statement directly
below it. Is that okay, or do you think I should use a new variable instead
within the 'if' statement?

EDIT: I like Mike's suggestion of factoring all this code into its own
parseNonce function, so I believe my question above is now irrelevant.
On 2013/05/14 05:58:16, abarth wrote:

+
+    const char noncePrefix[] = "'nonce-";
+    const int noncePrefixLen = strlen(noncePrefix);

[abarth-chromium, 2013/05/14 05:58:16]

It's better if you use a String that's an ASCIILiteral.  Then the compiler will
compute the length for you.

[jww, 2013/05/14 20:49:30]

I'm unclear on how to get the length in a compiler-friendly way if I declare an
ASCIILiteral. However, I noticed that there's a common use of
DEFINE_STATIC_LOCAL to make String objects from ASCIILiterals, so I changed it
to use that. Let me know if that's acceptable or unacceptable.
On 2013/05/14 05:58:16, abarth wrote:

+
+    if (equalIgnoringCase(noncePrefix, begin, noncePrefixLen) && (*(end - 1) == '\'')) {

[abarth-chromium, 2013/05/14 05:58:16]

There's no reason to check the end - 1 condition.  If the ' isn't in the right
place, we want to error out on line 479 rather than try to consume the the
directive another way.

[Mike West, 2013/05/14 08:07:38]

Given the length, it might also be reasonable to split this block out into
'parseNonce()', similar to the 'parseHost()', etc. we use in other places in
this function.

[jww, 2013/05/14 20:49:30]

Done.

[jww, 2013/05/14 20:49:30]

Done.

+        String nonce;
+        const UChar* nonceBegin = position = begin + noncePrefixLen;

[abarth-chromium, 2013/05/14 05:58:16]

We generally don't use compound statements like this one.  It's more readable if
you use two lines.

[jww, 2013/05/14 20:49:30]

Done.

+        skipWhile<isNonceCharacter>(position, end);
+
+        // According to the W3C spec, we need to accept the empty string as a
+        // valid nonce (that is, "script-src 'nonce-'" is valid and is a nonce
+        // of the empty string ""). Hence why we do a <= comparrion rather than
+        // just a < comparrison.
+        if (nonceBegin <= position) {

[abarth-chromium, 2013/05/14 05:58:16]

There's no way this can fail to be true.  I'd just make this an ASSERT and not
worry about handling the case where it's not true.

[jww, 2013/05/14 20:49:30]

Done.

+            nonce = String(nonceBegin, position - nonceBegin);
+        } else {
+            // This is a really odd error condition. Maybe this should be an
+            // ASSERT or do-not-reach?
+            return false;
+        }
+
+        if ((position + 1) != end  && *position != '\'') {
+            // There is an invalid nonce character here. We should report the
+            // invalid character, but also eat up any characters until the
+            // single quote.
+            position = end;

[abarth-chromium, 2013/05/14 05:58:16]

Position is a local variable.  THere's no reason to do this assignment.

[jww, 2013/05/14 20:49:30]

Whoops. Remnants of an old misunderstanding :-)
On 2013/05/14 05:58:16, abarth wrote:

+            return false;
+        }
+
+        addSourceNonce(nonce);
+        return true;
+    }
+
+    position = begin;
     const UChar* beginHost = begin;
     const UChar* beginPath = end;
     const UChar* beginPort = 0;
@@ -661,6 +699,11 @@ void CSPSourceList::addSourceUnsafeEval()
     m_allowEval = true;
 }
 
+void CSPSourceList::addSourceNonce(String& nonce)
+{
+    m_nonces.add(String(nonce));

[abarth-chromium, 2013/05/14 05:58:16]

There's no reason to call the String constructor again here.

[jww, 2013/05/14 20:49:30]

Done.

+}
+
 class CSPDirective {
 public:
     CSPDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
@@ -681,50 +724,6 @@ private:
     ContentSecurityPolicy* m_policy;
 };
 
-class NonceDirective : public CSPDirective {
-public:
-    NonceDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
-        : CSPDirective(name, value, policy)
-    {
-        parse(value);
-    }
-
-    bool allows(const String& nonce) const
-    {
-        return (!m_scriptNonce.isEmpty() && nonce.stripWhiteSpace() == m_scriptNonce);
-    }
-
-private:
-    void parse(const String& value)
-    {
-        String nonce;
-        const UChar* position = value.characters();
-        const UChar* end = position + value.length();
-
-        skipWhile<isASCIISpace>(position, end);
-        const UChar* nonceBegin = position;
-        if (position == end) {
-            policy()->reportInvalidNonce(String());
-            m_scriptNonce = "";
-            return;
-        }
-        skipWhile<isNonceCharacter>(position, end);
-        if (nonceBegin < position)
-            nonce = String(nonceBegin, position - nonceBegin);
-
-        // Trim off trailing whitespace: If we're not at the end of the string, log
-        // an error.
-        skipWhile<isASCIISpace>(position, end);
-        if (position < end) {
-            policy()->reportInvalidNonce(value);
-            m_scriptNonce = "";
-        } else
-            m_scriptNonce = nonce;
-    }
-
-    String m_scriptNonce;
-};
-
 class MediaListDirective : public CSPDirective {
 public:
     MediaListDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
@@ -817,6 +816,7 @@ public:
 
     bool allowInline() const { return m_sourceList.allowInline(); }
     bool allowEval() const { return m_sourceList.allowEval(); }
+    bool allowNonce(const String& nonce) const { return m_sourceList.allowNonce(nonce.stripWhiteSpace()); }
 
 private:
     CSPSourceList m_sourceList;
@@ -835,7 +835,6 @@ public:
     bool allowInlineScript(const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus) const;
     bool allowInlineStyle(const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus) const;
     bool allowEval(ScriptState*, ContentSecurityPolicy::ReportingStatus) const;
-    bool allowScriptNonce(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine, const KURL&) const;
     bool allowPluginType(const String& type, const String& typeAttribute, const KURL&, ContentSecurityPolicy::ReportingStatus) const;
 
     bool allowScriptFromSource(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
@@ -848,6 +847,7 @@ public:
     bool allowConnectToSource(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
     bool allowFormAction(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
     bool allowBaseURI(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
+    bool allowNonce(const String&) const;
 
     void gatherReportURIs(DOMStringList&) const;
     const String& evalDisabledErrorMessage() { return m_evalDisabledErrorMessage; }
@@ -862,7 +862,6 @@ private:
 
     bool parseDirective(const UChar* begin, const UChar* end, String& name, String& value);
     void parseReportURI(const String& name, const String& value);
-    void parseScriptNonce(const String& name, const String& value);
     void parsePluginTypes(const String& name, const String& value);
     void parseReflectedXSS(const String& name, const String& value);
     void addDirective(const String& name, const String& value);
@@ -876,7 +875,7 @@ private:
 
     bool checkEval(SourceListDirective*) const;
     bool checkInline(SourceListDirective*) const;
-    bool checkNonce(NonceDirective*, const String&) const;
+    bool checkNonce(SourceListDirective*, const String&) const;
     bool checkSource(SourceListDirective*, const KURL&) const;
     bool checkMediaType(MediaListDirective*, const String& type, const String& typeAttribute) const;
 
@@ -884,7 +883,6 @@ private:
 
     bool checkEvalAndReportViolation(SourceListDirective*, const String& consoleMessage, const String& contextURL = String(), const WTF::OrdinalNumber& contextLine = WTF::OrdinalNumber::beforeFirst(), ScriptState* = 0) const;
     bool checkInlineAndReportViolation(SourceListDirective*, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine, bool isScript) const;
-    bool checkNonceAndReportViolation(NonceDirective*, const String& nonce, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine) const;
 
     bool checkSourceAndReportViolation(SourceListDirective*, const KURL&, const String& effectiveDirective) const;
     bool checkMediaTypeAndReportViolation(MediaListDirective*, const String& type, const String& typeAttribute, const String& consoleMessage) const;
@@ -901,7 +899,6 @@ private:
     ContentSecurityPolicy::ReflectedXSSDisposition m_reflectedXSSDisposition;
 
     OwnPtr<MediaListDirective> m_pluginTypes;
-    OwnPtr<NonceDirective> m_scriptNonce;
     OwnPtr<SourceListDirective> m_baseURI;
     OwnPtr<SourceListDirective> m_connectSrc;
     OwnPtr<SourceListDirective> m_defaultSrc;
@@ -961,9 +958,9 @@ bool CSPDirectiveList::checkInline(SourceListDirective* directive) const
     return !directive || directive->allowInline();
 }
 
-bool CSPDirectiveList::checkNonce(NonceDirective* directive, const String& nonce) const
+bool CSPDirectiveList::checkNonce(SourceListDirective* directive, const String& nonce) const
 {
-    return !directive || directive->allows(nonce);
+    return !directive || directive->allowNonce(nonce);
 }
 
 bool CSPDirectiveList::checkSource(SourceListDirective* directive, const KURL& url) const
@@ -1002,14 +999,6 @@ bool CSPDirectiveList::checkEvalAndReportViolation(SourceListDirective* directiv
     return true;
 }
 
-bool CSPDirectiveList::checkNonceAndReportViolation(NonceDirective* directive, const String& nonce, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine) const
-{
-    if (checkNonce(directive, nonce))
-        return true;
-    reportViolation(directive->text(), scriptNonce, consoleMessage + "\"" + directive->text() + "\".\n", KURL(), contextURL, contextLine);
-    return denyIfEnforcingPolicy();
-}
-
 bool CSPDirectiveList::checkMediaTypeAndReportViolation(MediaListDirective* directive, const String& type, const String& typeAttribute, const String& consoleMessage) const
 {
     if (checkMediaType(directive, type, typeAttribute))
@@ -1081,11 +1070,9 @@ bool CSPDirectiveList::allowJavaScriptURLs(const String& contextURL, const WTF::
 {
     DEFINE_STATIC_LOCAL(String, consoleMessage, (ASCIILiteral("Refused to execute JavaScript URL because it violates the following Content Security Policy directive: ")));
     if (reportingStatus == ContentSecurityPolicy::SendReport) {
-        return (checkInlineAndReportViolation(operativeDirective(m_scriptSrc.get()), consoleMessage, contextURL, contextLine, true)
-                && checkNonceAndReportViolation(m_scriptNonce.get(), String(), consoleMessage, contextURL, contextLine));
+        return checkInlineAndReportViolation(operativeDirective(m_scriptSrc.get()), consoleMessage, contextURL, contextLine, true);
     } else {
-        return (checkInline(operativeDirective(m_scriptSrc.get()))
-                && checkNonce(m_scriptNonce.get(), String()));
+        return checkInline(operativeDirective(m_scriptSrc.get()));
     }
 }
 
@@ -1093,11 +1080,9 @@ bool CSPDirectiveList::allowInlineEventHandlers(const String& contextURL, const
 {
     DEFINE_STATIC_LOCAL(String, consoleMessage, (ASCIILiteral("Refused to execute inline event handler because it violates the following Content Security Policy directive: ")));
     if (reportingStatus == ContentSecurityPolicy::SendReport) {
-        return (checkInlineAndReportViolation(operativeDirective(m_scriptSrc.get()), consoleMessage, contextURL, contextLine, true)
-                && checkNonceAndReportViolation(m_scriptNonce.get(), String(), consoleMessage, contextURL, contextLine));
+        return checkInlineAndReportViolation(operativeDirective(m_scriptSrc.get()), consoleMessage, contextURL, contextLine, true);
     } else {
-        return (checkInline(operativeDirective(m_scriptSrc.get()))
-                && checkNonce(m_scriptNonce.get(), String()));
+        return checkInline(operativeDirective(m_scriptSrc.get()));
     }
 }
 
@@ -1125,14 +1110,6 @@ bool CSPDirectiveList::allowEval(ScriptState* state, ContentSecurityPolicy::Repo
         checkEval(operativeDirective(m_scriptSrc.get()));
 }
 
-bool CSPDirectiveList::allowScriptNonce(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine, const KURL& url) const
-{
-    DEFINE_STATIC_LOCAL(String, consoleMessage, (ASCIILiteral("Refused to execute script because it violates the following Content Security Policy directive: ")));
-    if (url.isEmpty())
-        return checkNonceAndReportViolation(m_scriptNonce.get(), nonce, consoleMessage, contextURL, contextLine);
-    return checkNonceAndReportViolation(m_scriptNonce.get(), nonce, "Refused to load '" + url.elidedString() + "' because it violates the following Content Security Policy directive: ", contextURL, contextLine);
-}
-
 bool CSPDirectiveList::allowPluginType(const String& type, const String& typeAttribute, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     return reportingStatus == ContentSecurityPolicy::SendReport ?
@@ -1220,6 +1197,11 @@ bool CSPDirectiveList::allowBaseURI(const KURL& url, ContentSecurityPolicy::Repo
         checkSource(m_baseURI.get(), url);
 }
 
+bool CSPDirectiveList::allowNonce(const String& nonce) const
+{
+    return checkNonce(operativeDirective(m_scriptSrc.get()), nonce);
+}
+
 // policy            = directive-list
 // directive-list    = [ directive *( ";" [ directive ] ) ]
 //
@@ -1426,8 +1408,6 @@ void CSPDirectiveList::addDirective(const String& name, const String& value)
             setCSPDirective<SourceListDirective>(name, value, m_formAction);
         else if (equalIgnoringCase(name, pluginTypes))
             setCSPDirective<MediaListDirective>(name, value, m_pluginTypes);
-        else if (equalIgnoringCase(name, scriptNonce))
-            setCSPDirective<NonceDirective>(name, value, m_scriptNonce);
         else if (equalIgnoringCase(name, reflectedXSS))
             parseReflectedXSS(name, value);
         else
@@ -1533,16 +1513,15 @@ bool isAllowedByAllWithContext(const CSPDirectiveListVector& policies, const Str
     return true;
 }
 
-template<bool (CSPDirectiveList::*allowed)(const String&, const String&, const WTF::OrdinalNumber&, const KURL&) const>
-bool isAllowedByAllWithNonce(const CSPDirectiveListVector& policies, const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine, const KURL& url)
+template<bool (CSPDirectiveList::*allowed)(const String&) const>
+bool isAllowedByAllWithNonce(const CSPDirectiveListVector& policies, const String& nonce)
 {
     for (size_t i = 0; i < policies.size(); ++i) {
-        if (!(policies[i].get()->*allowed)(nonce, contextURL, contextLine, url))
+        if (!(policies[i].get()->*allowed)(nonce))
             return false;
     }
     return true;
 }
-
 template<bool (CSPDirectiveList::*allowFromURL)(const KURL&, ContentSecurityPolicy::ReportingStatus) const>
 bool isAllowedByAllWithURL(const CSPDirectiveListVector& policies, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus)
 {
@@ -1566,9 +1545,10 @@ bool ContentSecurityPolicy::allowInlineEventHandlers(const String& contextURL, c
     return isAllowedByAllWithContext<&CSPDirectiveList::allowInlineEventHandlers>(m_policies, contextURL, contextLine, reportingStatus);
 }
 
-bool ContentSecurityPolicy::allowInlineScript(const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowInlineScript(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
-    return isAllowedByAllWithContext<&CSPDirectiveList::allowInlineScript>(m_policies, contextURL, contextLine, reportingStatus);
+    bool nonceAllowed = isAllowedByAllWithNonce<&CSPDirectiveList::allowNonce>(m_policies, nonce);
+    return nonceAllowed || isAllowedByAllWithContext<&CSPDirectiveList::allowInlineScript>(m_policies, contextURL, contextLine, reportingStatus);
 }
 
 bool ContentSecurityPolicy::allowInlineStyle(const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus reportingStatus) const
@@ -1592,11 +1572,6 @@ String ContentSecurityPolicy::evalDisabledErrorMessage() const
     return String();
 }
 
-bool ContentSecurityPolicy::allowScriptNonce(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine, const KURL& url) const
-{
-    return isAllowedByAllWithNonce<&CSPDirectiveList::allowScriptNonce>(m_policies, nonce, contextURL, contextLine, url);
-}
-
 bool ContentSecurityPolicy::allowPluginType(const String& type, const String& typeAttribute, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     for (size_t i = 0; i < m_policies.size(); ++i) {
@@ -1606,9 +1581,10 @@ bool ContentSecurityPolicy::allowPluginType(const String& type, const String& ty
     return true;
 }
 
-bool ContentSecurityPolicy::allowScriptFromSource(const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowScriptFromSource(const KURL& url, const String& nonce, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
-    return isAllowedByAllWithURL<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, reportingStatus);
+    bool nonceAllowed = isAllowedByAllWithNonce<&CSPDirectiveList::allowNonce>(m_policies, nonce);
+    return nonceAllowed || isAllowedByAllWithURL<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, reportingStatus);
 }
 
 bool ContentSecurityPolicy::allowObjectFromSource(const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const