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..1df8bb1b6871eb66019fd0cecafc10f73faaf6f4 100644
--- a/Source/core/frame/ContentSecurityPolicy.cpp
+++ b/Source/core/frame/ContentSecurityPolicy.cpp
@@ -49,6 +49,8 @@
 #include "weborigin/SchemeRegistry.h"
 #include "weborigin/SecurityOrigin.h"
 #include "wtf/HashSet.h"
+#include "wtf/SHA1.h"
+#include "wtf/text/Base64.h"
 #include "wtf/text/TextPosition.h"
 #include "wtf/text/WTFString.h"
 
@@ -74,6 +76,11 @@ bool isNonceCharacter(UChar c)
     return isASCIIAlphanumeric(c) || c == '+' || c == '/';
 }
 
+bool isHashCharacter(UChar c)
+{
+    return isASCIIAlphanumeric(c) || c == '+' || c == '/' || c == '=';

[Mike West, 2013/10/16 14:53:02]

It seems that we should be able to get away with combining isNonceCharacter and
isHashCharacter via an isBase64EncodingCharacter implementation.

[jww, 2013/10/16 21:49:21]

Good point, and it actually brings up a more interesting point that the current
check for hashes is too permissive; it allows '=' anywhere in the hash.

+}
+
 bool isSourceCharacter(UChar c)
 {
     return !isASCIISpace(c);
@@ -279,6 +286,7 @@ 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); }
 
 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);
 
     void addSourceSelf();
     void addSourceStar();
     void addSourceUnsafeInline();
     void addSourceUnsafeEval();
     void addSourceNonce(const String& nonce);
+    void addSourceHash(const String& hash);
 
     ContentSecurityPolicy* m_policy;
     Vector<CSPSource> m_list;
@@ -301,6 +311,7 @@ private:
     bool m_allowInline;
     bool m_allowEval;
     HashSet<String> m_nonces;
+    HashSet<String> m_hashes;
 };
 
 CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String& directiveName)
@@ -409,6 +420,15 @@ bool CSPSourceList::parseSource(const UChar* begin, const UChar* end,
             addSourceNonce(nonce);
             return true;
         }
+
+        String hash;
+        if (!parseHash(begin, end, hash))
+            return false;
+
+        if (!hash.isNull()) {
+            addSourceHash(hash);
+            return true;
+        }
     }
 
     const UChar* position = begin;
@@ -508,6 +528,35 @@ 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)
+{
+    DEFINE_STATIC_LOCAL(const String, sha1Prefix, ("'sha1-"));
+    DEFINE_STATIC_LOCAL(const String, sha256Prefix, ("'sha256-"));
+
+    bool isSha1 = equalIgnoringCase(sha1Prefix.characters8(), begin, sha1Prefix.length());
+    bool isSha256 = equalIgnoringCase(sha256Prefix.characters8(), begin, sha256Prefix.length());
+
+    if (!isSha1 && !isSha256)
+        return true;

[Mike West, 2013/10/16 14:53:02]

We might want to be more clever about this to warn folks about unsupported hash
functions. Probably something for a future patch, but keep it in mind.

[jww, 2013/10/16 21:49:21]

Yeah, I'll put in a todo to make a warning message.

+
+    const String& prefix = (isSha1 ? sha1Prefix : sha256Prefix);

[Mike West, 2013/10/16 14:53:02]

You're only using this to determine the initial position, right? Perhaps 'const
UChar* position = begin + (isSha1 ? sha1Prefix.length() :
sha256Prefix.length());" would be more straightforward.

[jww, 2013/10/16 21:49:21]

Ah, yes, good call. Remnants from an earlier version :-)

+    const UChar* position = begin + prefix.length();
+    const UChar* hashBegin = position;
+
+    skipWhile<UChar, isHashCharacter>(position, end);
+    ASSERT(hashBegin <= position);
+
+    if (((position + 1) != end  && *position != '\'') || !(position - hashBegin))
+        return false;
+
+    hash = String(begin + 1, position - begin - 1);

[Mike West, 2013/10/16 14:53:02]

You need to pass the hash type back to the caller too, right? Otherwise you have
the hash, but don't know what function to call to reproduce it.

[jww, 2013/10/16 21:49:21]

This actually leaves the prefix on the beginning of the hash. That is, the
return string will be on the form: 'sha1-thehashofthescript'. It seemed like
this would be the easiest way to store and look up the hashes.

+    return true;
+}
+
 //                     ; <scheme> production from RFC 3986
 // scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
 //
@@ -650,6 +699,11 @@ void CSPSourceList::addSourceNonce(const String& nonce)
     m_nonces.add(nonce);
 }
 
+void CSPSourceList::addSourceHash(const String& hash)
+{
+    m_hashes.add(hash);

[Mike West, 2013/10/16 14:53:02]

Again, I think you'll need the type as well.

[jww, 2013/10/16 21:49:21]

See above reply. The hash string actually includes the prefix.

+}
+
 class CSPDirective {
 public:
     CSPDirective(const String& name, const String& value, ContentSecurityPolicy* policy)
@@ -766,6 +820,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()); }
+    bool allowHash(const String& hash) const { return m_sourceList.allowHash(hash); }
 
 private:
     CSPSourceList m_sourceList;
@@ -800,6 +855,8 @@ public:
     bool allowBaseURI(const KURL&, ContentSecurityPolicy::ReportingStatus) const;
     bool allowScriptNonce(const String&) const;
     bool allowStyleNonce(const String&) const;
+    bool allowScriptHash(const String&) const;
+    bool allowStyleHash(const String&) const;
 
     void gatherReportURIs(DOMStringList&) const;
     const String& evalDisabledErrorMessage() const { return m_evalDisabledErrorMessage; }
@@ -828,6 +885,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 +988,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 +1238,16 @@ 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);
+}
+
+bool CSPDirectiveList::allowStyleHash(const String& hash) const
+{
+    return checkHash(operativeDirective(m_styleSrc.get()), hash);
+}
+
 // policy            = directive-list
 // directive-list    = [ directive *( ";" [ directive ] ) ]
 //
@@ -1524,6 +1597,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 +1681,27 @@ 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-"));

[Mike West, 2013/10/16 14:53:02]

If you're using this string in two places, extract it somewhere. Top of the
document with the other constants seems reasonable.

[jww, 2013/10/16 21:49:21]

Done.

+    DEFINE_STATIC_LOCAL(const String, sha256Prefix, ("sha256-"));
+
+    // 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.
+    SHA1 sourceSha1;
+    sourceSha1.addBytes(source.utf8());
+    String sha1Hash(sourceSha1.computeHexDigest().data());
+
+    String lowerHash = sha1Hash.lower();
+    String hash = sha1Prefix + base64Encode(lowerHash.utf8());
+    return isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash);

[Mike West, 2013/10/16 14:53:02]

Ah, I see. You're storing the whole string.

I think it would be more efficient to determine which hash functions you
actually _need_ to run based on the hashes provided in the policy. If it only
provides sha1 hash, running sha256 is a waste of time, and vice-versa.

[jww, 2013/10/16 21:49:21]

Good idea. What about just storing some bits that say which hash functions are
in the policy, but leaving the prefixes on? That is, during parsing, if we come
across a SHA1 and a SHA256 hash, we keep that information around, and we
calculate hashes of scripts only for the hash functions used.

That means we can still do quick look ups on sets of strings without worrying
about collisions, but still only calculate hashes for algorithms that are
present in the policy.

+}
+
+bool ContentSecurityPolicy::allowStyleHash(const String& hash) const
+{
+    return isAllowedByAllWithHash<&CSPDirectiveList::allowStyleHash>(m_policies, hash);
+}
+
 bool ContentSecurityPolicy::allowObjectFromSource(const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     return isAllowedByAllWithURL<&CSPDirectiveList::allowObjectFromSource>(m_policies, url, reportingStatus);
@@ -1857,6 +1962,12 @@ void ContentSecurityPolicy::reportInvalidNonce(const String& nonce) const
     logToConsole(message);
 }
 
+void ContentSecurityPolicy::reportInvalidHash(const String& hash) const
+{
+    String message = "Ignoring invalid Content Security Policy script hash: '" + hash + "'.\n";

[Mike West, 2013/10/16 14:53:02]

I'd suggest changing both this and reportInvalidNonce to something similar to
reportInvalidSourceExpression directly below. "The source list for CSP directive
XXX contains an invalid [nonce|hash]: YYY. It will be ignored."

[jww, 2013/10/16 21:49:21]

Hmmm, not that you point this out, I believe this is dead code anyway :-) Both
nonces and hashes are source expressions, and these functions never get called
because the error message comes from reportInvalidSourceExperssion.

+    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.";