Implementation of script hashes for CSP.

Source/core/frame/ContentSecurityPolicy.cpp

Index: Source/core/frame/ContentSecurityPolicy.cpp
diff --git a/Source/core/frame/ContentSecurityPolicy.cpp b/Source/core/frame/ContentSecurityPolicy.cpp
index e141aa91cb904e121d2d03d7abf32e53fb9c6ee0..879ea4f62706e51b8a04b3224a8e5448d4aa7281 100644
--- a/Source/core/frame/ContentSecurityPolicy.cpp
+++ b/Source/core/frame/ContentSecurityPolicy.cpp
@@ -44,11 +44,15 @@
 #include "core/platform/network/FormData.h"
 #include "core/platform/network/ResourceResponse.h"
 #include "platform/JSONValues.h"
+#include "platform/NotImplemented.h"
 #include "weborigin/KURL.h"
 #include "weborigin/KnownPorts.h"
 #include "weborigin/SchemeRegistry.h"
 #include "weborigin/SecurityOrigin.h"
 #include "wtf/HashSet.h"
+#include "wtf/SHA1.h"
+#include "wtf/text/Base64.h"
+#include "wtf/text/StringBuilder.h"
 #include "wtf/text/TextPosition.h"
 #include "wtf/text/WTFString.h"
 
@@ -69,7 +73,9 @@ bool isDirectiveValueCharacter(UChar c)
     return isASCIISpace(c) || (c >= 0x21 && c <= 0x7e); // Whitespace + VCHAR
 }
 
-bool isNonceCharacter(UChar c)
+// Only checks for general Base64 encoded chars, not '=' chars since '=' is
+// positional and may only appear at the end of a Base64 encoded string.
+bool isBase64EncodedCharacter(UChar c)
 {
     return isASCIIAlphanumeric(c) || c == '+' || c == '/';
 }
@@ -279,6 +285,8 @@ public:
     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); }
+    bool allowHash(const String& hash) const { return !hash.isNull() && m_hashes.contains(hash); }
+    uint8_t getHashFunctionsUsed() const { return m_hashFunctionsUsed; }

[abarth-chromium, 2013/10/22 17:46:49]

getHashFunctionsUsed -> hashFunctionsUsed

(The "get" prefix doesn't add anything so we usually drop it.)

[jww, 2013/10/28 19:36:23]

Done.

 
 private:
     bool parseSource(const UChar* begin, const UChar* end, String& scheme, String& host, int& port, String& path, bool& hostHasWildcard, bool& portHasWildcard);
@@ -287,12 +295,14 @@ private:
     bool parsePort(const UChar* begin, const UChar* end, int& port, bool& portHasWildcard);
     bool parsePath(const UChar* begin, const UChar* end, String& path);
     bool parseNonce(const UChar* begin, const UChar* end, String& nonce);
+    bool parseHash(const UChar* begin, const UChar* end, String& hash, ContentSecurityPolicy::HashFunctions&);
 
     void addSourceSelf();
     void addSourceStar();
     void addSourceUnsafeInline();
     void addSourceUnsafeEval();
     void addSourceNonce(const String& nonce);
+    void addSourceHash(const String& hash, const ContentSecurityPolicy::HashFunctions&);
 
     ContentSecurityPolicy* m_policy;
     Vector<CSPSource> m_list;
@@ -301,6 +311,8 @@ private:
     bool m_allowInline;
     bool m_allowEval;
     HashSet<String> m_nonces;
+    HashSet<String> m_hashes;
+    uint8_t m_hashFunctionsUsed;

[abarth-chromium, 2013/10/22 17:46:49]

Don't we need to keep track of which hash functions were used on a per-hash
basis?  I was expecting this to be a HashSet of a struct that contained a hash
function and a hash value.

[jww, 2013/10/28 19:36:23]

Fixed by other changes.

 };
 
 CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String& directiveName)
@@ -309,6 +321,7 @@ CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String& direct
     , m_allowStar(false)
     , m_allowInline(false)
     , m_allowEval(false)
+    , m_hashFunctionsUsed(0)
 {
 }
 
@@ -409,6 +422,16 @@ bool CSPSourceList::parseSource(const UChar* begin, const UChar* end,
             addSourceNonce(nonce);
             return true;
         }
+
+        String hash;
+        ContentSecurityPolicy::HashFunctions hashFunction = ContentSecurityPolicy::HashFunctionsNone;
+        if (!parseHash(begin, end, hash, hashFunction))
+            return false;
+
+        if (!hash.isNull()) {
+            addSourceHash(hash, hashFunction);
+            return true;
+        }
     }
 
     const UChar* position = begin;
@@ -498,7 +521,7 @@ bool CSPSourceList::parseNonce(const UChar* begin, const UChar* end, String& non
     const UChar* position = begin + noncePrefix.length();
     const UChar* nonceBegin = position;
 
-    skipWhile<UChar, isNonceCharacter>(position, end);
+    skipWhile<UChar, isBase64EncodedCharacter>(position, end);
     ASSERT(nonceBegin <= position);
 
     if (((position + 1) != end  && *position != '\'') || !(position - nonceBegin))
@@ -508,6 +531,42 @@ bool CSPSourceList::parseNonce(const UChar* begin, const UChar* end, String& non
     return true;
 }
 
+// hash-source       = "'" hash-algorithm "-" hash-value "'"
+// hash-algorithm    = "sha1" / "sha256"
+// hash-value        = 1*( ALPHA / DIGIT / "+" / "/" / "=" )
+//
+bool CSPSourceList::parseHash(const UChar* begin, const UChar* end, String& hash, ContentSecurityPolicy::HashFunctions& hashFunction)
+{
+    DEFINE_STATIC_LOCAL(const String, sha1Prefix, ("'sha1-"));
+    DEFINE_STATIC_LOCAL(const String, sha256Prefix, ("'sha256-"));
+
+    String prefix;
+    if (equalIgnoringCase(sha1Prefix.characters8(), begin, sha1Prefix.length())) {
+        prefix = sha1Prefix;
+        hashFunction = ContentSecurityPolicy::HashFunctionsSha1;
+    } else if (equalIgnoringCase(sha256Prefix.characters8(), begin, sha256Prefix.length())) {
+        notImplemented();
+    } else {
+        return true;
+    }
+
+    const UChar* position = begin + prefix.length();
+    const UChar* hashBegin = position;
+
+    skipWhile<UChar, isBase64EncodedCharacter>(position, end);
+    ASSERT(hashBegin <= position);
+
+    // Base64 encodings may end with exactly one or two '=' characters
+    skipExactly<UChar>(position, position + 1, '=');
+    skipExactly<UChar>(position, position + 1, '=');
+
+    if (((position + 1) != end  && *position != '\'') || !(position - hashBegin))
+        return false;

[abarth-chromium, 2013/10/22 17:46:49]

Is this logic right?  I would have expected:

if ((position + 1 != end) || *position != '\'' || !(position - hashBegin))

In particular, we want to return false if position + 1 != end, right?

[jww, 2013/10/28 19:36:23]

Done.

+
+    hash = String(begin + 1, position - begin - 1);

[abarth-chromium, 2013/10/22 17:46:49]

Why begin + 1 rather than hashBegin?  I would have thought that |hash| would be
the base64 encoded hash value with the prefix stripped off.  Here you're
including the prefix.

I see, you're including the tag in the hash.  Maybe we should use a different
name for the variable?  Perhaps tagAndHash?

Also, there's something odd that happens if the site supplies something like
'SHA1-oisnf3n30we498...'.  You're using a case-insensitive comparison on like
544, but then on line 1707 you always use a lower case tag.  That means
uppercase tags will never match.

Rather than trying to encode the hash function in-band in the string, I'd
recommending using a data structure.  Instead of storing a string in the
HashSet, we can store a struct that contains the enum value for the hash
function and a string corresponding to the hash value.  We can even use a
Vector<uint8_t> to store the hash value decoded so we don't have any trouble
with canonicalizing the base64 encoding.  That will give us a much more directly
internal representation.

[jww, 2013/10/28 19:36:23]

Hashes are now stored as a struct that contain the hash function used and a bit
vector of the hash value.

+    return true;
+}
+
 //                     ; <scheme> production from RFC 3986
 // scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
 //
@@ -650,6 +709,12 @@ void CSPSourceList::addSourceNonce(const String& nonce)
     m_nonces.add(nonce);
 }
 
+void CSPSourceList::addSourceHash(const String& hash, const ContentSecurityPolicy::HashFunctions& hashFunction)
+{
+    m_hashes.add(hash);
+    m_hashFunctionsUsed |= hashFunction;
+}
+
 class CSPDirective {
 public:
     CSPDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
@@ -766,6 +831,9 @@ 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()); }
+    bool allowHash(const String& hash) const { return m_sourceList.allowHash(hash); }
+
+    uint8_t getHashFunctionsUsed() const { return m_sourceList.getHashFunctionsUsed(); }
 
 private:
     CSPSourceList m_sourceList;
@@ -800,6 +868,7 @@ public:
     bool allowBaseURI(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
     bool allowScriptNonce(const String&) const;
     bool allowStyleNonce(const String&) const;
+    bool allowScriptHash(const String&) const;
 
     void gatherReportURIs(DOMStringList&) const;
     const String& evalDisabledErrorMessage() const { return m_evalDisabledErrorMessage; }
@@ -828,6 +897,7 @@ private:
     bool checkEval(SourceListDirective*) const;
     bool checkInline(SourceListDirective*) const;
     bool checkNonce(SourceListDirective*, const String&) const;
+    bool checkHash(SourceListDirective*, const String&) const;
     bool checkSource(SourceListDirective*, const KURL&) const;
     bool checkMediaType(MediaListDirective*, const String& type, const String& typeAttribute) const;
 
@@ -930,6 +1000,11 @@ bool CSPDirectiveList::checkNonce(SourceListDirective* directive, const String&
     return !directive || directive->allowNonce(nonce);
 }
 
+bool CSPDirectiveList::checkHash(SourceListDirective* directive, const String& hash) const
+{
+    return !directive || directive->allowHash(hash);
+}
+
 bool CSPDirectiveList::checkSource(SourceListDirective* directive, const KURL& url) const
 {
     return !directive || directive->allows(url);
@@ -1175,6 +1250,11 @@ bool CSPDirectiveList::allowStyleNonce(const String& nonce) const
     return checkNonce(operativeDirective(m_styleSrc.get()), nonce);
 }
 
+bool CSPDirectiveList::allowScriptHash(const String& hash) const
+{
+    return checkHash(operativeDirective(m_scriptSrc.get()), hash);
+}
+
 // policy            = directive-list
 // directive-list    = [ directive *( ";" [ directive ] ) ]
 //
@@ -1358,29 +1438,30 @@ void CSPDirectiveList::addDirective(const String& name, const String& value)
 {
     ASSERT(!name.isEmpty());
 
-    if (equalIgnoringCase(name, defaultSrc))
+    if (equalIgnoringCase(name, defaultSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_defaultSrc);
-    else if (equalIgnoringCase(name, scriptSrc))
+    } else if (equalIgnoringCase(name, scriptSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_scriptSrc);
-    else if (equalIgnoringCase(name, objectSrc))
+        m_policy->usesScriptHashFunctions(m_scriptSrc->getHashFunctionsUsed());
+    } else if (equalIgnoringCase(name, objectSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_objectSrc);
-    else if (equalIgnoringCase(name, frameSrc))
+    } else if (equalIgnoringCase(name, frameSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_frameSrc);
-    else if (equalIgnoringCase(name, imgSrc))
+    } else if (equalIgnoringCase(name, imgSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_imgSrc);
-    else if (equalIgnoringCase(name, styleSrc))
+    } else if (equalIgnoringCase(name, styleSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_styleSrc);
-    else if (equalIgnoringCase(name, fontSrc))
+    } else if (equalIgnoringCase(name, fontSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_fontSrc);
-    else if (equalIgnoringCase(name, mediaSrc))
+    } else if (equalIgnoringCase(name, mediaSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_mediaSrc);
-    else if (equalIgnoringCase(name, connectSrc))
+    } else if (equalIgnoringCase(name, connectSrc)) {
         setCSPDirective<SourceListDirective>(name, value, m_connectSrc);
-    else if (equalIgnoringCase(name, sandbox))
+    } else if (equalIgnoringCase(name, sandbox)) {
         applySandboxPolicy(name, value);
-    else if (equalIgnoringCase(name, reportURI))
+    } else if (equalIgnoringCase(name, reportURI)) {
         parseReportURI(name, value);
-    else if (m_policy->experimentalFeaturesEnabled()) {
+    } else if (m_policy->experimentalFeaturesEnabled()) {
         if (equalIgnoringCase(name, baseURI))
             setCSPDirective<SourceListDirective>(name, value, m_baseURI);
         else if (equalIgnoringCase(name, formAction))
@@ -1391,14 +1472,15 @@ void CSPDirectiveList::addDirective(const String& name, const String& value)
             parseReflectedXSS(name, value);
         else
             m_policy->reportUnsupportedDirective(name);
-    }
-    else
+    } else {
         m_policy->reportUnsupportedDirective(name);
+    }
 }
 
 ContentSecurityPolicy::ContentSecurityPolicy(ExecutionContext* executionContext)
     : m_executionContext(executionContext)
     , m_overrideInlineStyleAllowed(false)
+    , m_sourceHashFunctionsUsed(HashFunctionsNone)
 {
 }
 
@@ -1524,6 +1606,17 @@ bool isAllowedByAllWithNonce(const CSPDirectiveListVector& policies, const Strin
     }
     return true;
 }
+
+template<bool (CSPDirectiveList::*allowed)(const String&) const>
+bool isAllowedByAllWithHash(const CSPDirectiveListVector& policies, const String& hash)
+{
+    for (size_t i = 0; i < policies.size(); ++i) {
+        if (!(policies[i].get()->*allowed)(hash))
+            return false;
+    }
+    return true;
+}
+
 template<bool (CSPDirectiveList::*allowFromURL)(const KURL&, ContentSecurityPolicy::ReportingStatus) const>
 bool isAllowedByAllWithURL(const CSPDirectiveListVector& policies, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus)
 {
@@ -1597,6 +1690,34 @@ bool ContentSecurityPolicy::allowStyleNonce(const String& nonce) const
     return isAllowedByAllWithNonce<&CSPDirectiveList::allowStyleNonce>(m_policies, nonce);
 }
 
+bool ContentSecurityPolicy::allowScriptHash(const String& source) const
+{
+    DEFINE_STATIC_LOCAL(const String, sha1Prefix, ("sha1-"));
+
+    // TODO(jww) We don't currently have a WTF SHA256 implementation. Once we
+    // have that, we should implement a proper check for sha256 hash values here.
+    if (HashFunctionsSha1 & m_sourceHashFunctionsUsed) {
+        SHA1 sourceSha1;
+        sourceSha1.addBytes(UTF8Encoding().normalizeAndEncode(source, WTF::EntitiesForUnencodables));
+        Vector<uint8_t, 20> digest;
+        sourceSha1.computeHash(digest);
+
+        StringBuilder hash;
+        hash.reserveCapacity(sha1Prefix.length() + digest.size());
+        hash.append(sha1Prefix);
+        hash.append(base64Encode(reinterpret_cast<char*>(digest.data()), digest.size()));

[abarth-chromium, 2013/10/22 17:46:49]

What about the trailing = characters?  Do we need to canonicalize the base64
encoding at all?  For example, maybe we should strip off the trailing =?  Maybe
we should require them to be present?

[jww, 2013/10/28 19:36:23]

Addressed by other changes (now storing the hash as a bit vector).

+        if (isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash.toString()))
+            return true;
+    }
+
+    return false;
+}
+
+void ContentSecurityPolicy::usesScriptHashFunctions(uint8_t hashFunctions)
+{
+    m_sourceHashFunctionsUsed |= hashFunctions;
+}
+
 bool ContentSecurityPolicy::allowObjectFromSource(const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     return isAllowedByAllWithURL<&CSPDirectiveList::allowObjectFromSource>(m_policies, url, reportingStatus);
@@ -1851,12 +1972,6 @@ void ContentSecurityPolicy::reportInvalidPathCharacter(const String& directiveNa
     logToConsole(message);
 }
 
-void ContentSecurityPolicy::reportInvalidNonce(const String& nonce) const
-{
-    String message = "Ignoring invalid Content Security Policy script nonce: '" + nonce + "'.\n";
-    logToConsole(message);
-}
-
 void ContentSecurityPolicy::reportInvalidSourceExpression(const String& directiveName, const String& source) const
 {
     String message = "The source list for Content Security Policy directive '" + directiveName + "' contains an invalid source: '" + source + "'. It will be ignored.";