Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26481005:
Implementation of script hashes for CSP. (Closed)
|
Created:
7 years, 2 months ago by jww Modified:
7 years, 1 month ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplementation of script hashes for CSP.
BUG=277905
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160866
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fixes and improvements. #Patch Set 3 : Added tests #
Total comments: 18
Patch Set 4 : Fixed abarth's comments, added tests, and added short circuit for unused hashes. #
Total comments: 16
Patch Set 5 : Updated with nits from mkwst. #Patch Set 6 : More fixes from mkwst comments #
Total comments: 14
Patch Set 7 : Changes from abarth, including splitting hash into struct #
Total comments: 6
Patch Set 8 : Fixed nits from abarth #Patch Set 9 : Rebase on lkgr #Patch Set 10 : Rebase on tip of tree #Messages
Total messages: 23 (0 generated)
This is my initial implementation of script hashes. It still needs some work (tests, SHA256 implementation, and notably I still need to figure out how to get the binary representation of the computed hash), but I'd love your thoughts so far.
This is a great start, Joel! Thanks for putting it together... I look forward to tests! :) I'd suggest cleaning this up for landing with just sha1 support, and adding sha256 functionality in a second CL. I suspect that will be a little more work if it's not already cleanly exposed in WTF. https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.... Source/core/dom/ScriptLoader.cpp:311: bool shouldBypassMainWorldContentSecurityPolicy = (frame && frame->script()->shouldBypassMainWorldContentSecurityPolicy()) || elementDocument->contentSecurityPolicy()->allowScriptNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr)) || elementDocument->contentSecurityPolicy()->allowScriptHash(sourceCode.source()); This is getting pretty ugly. I shouldn't have added the nonce bits here in the first place, but adding hash really makes it visibly wrong. Ideally, we'd refactor this in a separate CL so that the 'shouldBypassMainWorldContentSecurityPolicy' variable did what it said. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:81: return isASCIIAlphanumeric(c) || c == '+' || c == '/' || c == '='; It seems that we should be able to get away with combining isNonceCharacter and isHashCharacter via an isBase64EncodingCharacter implementation. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:544: return true; 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:546: const String& prefix = (isSha1 ? sha1Prefix : sha256Prefix); 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:556: hash = String(begin + 1, position - begin - 1); 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:704: m_hashes.add(hash); Again, I think you'll need the type as well. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1686: DEFINE_STATIC_LOCAL(const String, sha1Prefix, ("sha1-")); If you're using this string in two places, extract it somewhere. Top of the document with the other constants seems reasonable. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1697: return isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash); 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1967: String message = "Ignoring invalid Content Security Policy script hash: '" + hash + "'.\n"; 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." https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.h:106: bool allowStyleHash(const String& hash) const; source vs hash? I haven't looked at the implementations yet, but I imagine these should be the same name.
On 2013/10/16 14:53:02, Mike West wrote: > This is a great start, Joel! Thanks for putting it together... I look forward to > tests! :) > > I'd suggest cleaning this up for landing with just sha1 support, and adding > sha256 functionality in a second CL. I suspect that will be a little more work > if it's not already cleanly exposed in WTF. > > https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.cpp > File Source/core/dom/ScriptLoader.cpp (right): > > https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.... > Source/core/dom/ScriptLoader.cpp:311: bool > shouldBypassMainWorldContentSecurityPolicy = (frame && > frame->script()->shouldBypassMainWorldContentSecurityPolicy()) || > elementDocument->contentSecurityPolicy()->allowScriptNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr)) > || > elementDocument->contentSecurityPolicy()->allowScriptHash(sourceCode.source()); > This is getting pretty ugly. I shouldn't have added the nonce bits here in the > first place, but adding hash really makes it visibly wrong. Ideally, we'd > refactor this in a separate CL so that the > 'shouldBypassMainWorldContentSecurityPolicy' variable did what it said. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > File Source/core/frame/ContentSecurityPolicy.cpp (right): > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:81: return isASCIIAlphanumeric(c) || > c == '+' || c == '/' || c == '='; > It seems that we should be able to get away with combining isNonceCharacter and > isHashCharacter via an isBase64EncodingCharacter implementation. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:544: return true; > 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. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:546: const String& prefix = (isSha1 > ? sha1Prefix : sha256Prefix); > 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. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:556: hash = String(begin + 1, > position - begin - 1); > 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. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:704: m_hashes.add(hash); > Again, I think you'll need the type as well. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:1686: DEFINE_STATIC_LOCAL(const > String, sha1Prefix, ("sha1-")); > If you're using this string in two places, extract it somewhere. Top of the > document with the other constants seems reasonable. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:1697: return > isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash); > 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. > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.cpp:1967: String message = "Ignoring > invalid Content Security Policy script hash: '" + hash + "'.\n"; > 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." > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > File Source/core/frame/ContentSecurityPolicy.h (right): > > https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... > Source/core/frame/ContentSecurityPolicy.h:106: bool allowStyleHash(const String& > hash) const; > source vs hash? I haven't looked at the implementations yet, but I imagine these > should be the same name. Awesome, and thanks for the comments. I'll upload the fixes mentioned in my response, and I'll work on getting a more complete CL up tonight.
https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.... Source/core/dom/ScriptLoader.cpp:311: bool shouldBypassMainWorldContentSecurityPolicy = (frame && frame->script()->shouldBypassMainWorldContentSecurityPolicy()) || elementDocument->contentSecurityPolicy()->allowScriptNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr)) || elementDocument->contentSecurityPolicy()->allowScriptHash(sourceCode.source()); On 2013/10/16 14:53:02, Mike West wrote: > This is getting pretty ugly. I shouldn't have added the nonce bits here in the > first place, but adding hash really makes it visibly wrong. Ideally, we'd > refactor this in a separate CL so that the > 'shouldBypassMainWorldContentSecurityPolicy' variable did what it said. I'm a little confused about what you'd like to see here. How about I break up all of these calls into separate booleans and we can just || them all together. That should make it quite a bit more readable, yes? I suppose the only problem is short circuiting extraneous checks, but that's what compilers are for, I suppose :-) https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:81: return isASCIIAlphanumeric(c) || c == '+' || c == '/' || c == '='; On 2013/10/16 14:53:02, Mike West wrote: > It seems that we should be able to get away with combining isNonceCharacter and > isHashCharacter via an isBase64EncodingCharacter implementation. 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:544: return true; On 2013/10/16 14:53:02, Mike West wrote: > 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. Yeah, I'll put in a todo to make a warning message. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:546: const String& prefix = (isSha1 ? sha1Prefix : sha256Prefix); On 2013/10/16 14:53:02, Mike West wrote: > 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. Ah, yes, good call. Remnants from an earlier version :-) https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:556: hash = String(begin + 1, position - begin - 1); On 2013/10/16 14:53:02, Mike West wrote: > 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. 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:704: m_hashes.add(hash); On 2013/10/16 14:53:02, Mike West wrote: > Again, I think you'll need the type as well. See above reply. The hash string actually includes the prefix. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1686: DEFINE_STATIC_LOCAL(const String, sha1Prefix, ("sha1-")); On 2013/10/16 14:53:02, Mike West wrote: > If you're using this string in two places, extract it somewhere. Top of the > document with the other constants seems reasonable. Done. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1697: return isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash); On 2013/10/16 14:53:02, Mike West wrote: > 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. 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.cpp:1967: String message = "Ignoring invalid Content Security Policy script hash: '" + hash + "'.\n"; On 2013/10/16 14:53:02, Mike West wrote: > 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." 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. https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/1/Source/core/frame/ContentSecu... Source/core/frame/ContentSecurityPolicy.h:106: bool allowStyleHash(const String& hash) const; On 2013/10/16 14:53:02, Mike West wrote: > source vs hash? I haven't looked at the implementations yet, but I imagine these > should be the same name. Done.
Okay, I've made the changes I mentioned in my reply to Mike, I've added some tests, and I fixed the sha1 hash check to actually be correct. I do have one clarification, however. Neither Nicholas's or Neil's emails fully clarify 1 point, which is should script hashes apply to src'd scripts as well as inline? One of Neil's comments implies only to inlined, which is my preference. The problem is if they apply to src'd scripts as well, we have a chicken and egg problem which is that we check CSP to see if the resource URL is acceptable according to the policy. If not, we do not make the request. However, that means that if the URL is not acceptable *but* the hash of the script *is* acceptable, we never see the script, and thus can't override the URL decision. Obviously this isn't a fundamental limitation, but I'm not sure of the true value of applying script hash to src'd content, and it would take a heck of a lot of refactoring. Thoughts?
https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoa... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoa... Source/core/dom/ScriptLoader.cpp:313: bool allowScriptHash = elementDocument->contentSecurityPolicy()->allowHash(sourceCode.source()); We shouldn't call either of these functions if frameContentSecurityPolicyBypass is true. They'll generator bogus violation reports. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:544: sha1Prefix.append("-"); There's no reason to use a StringBuilder here. You can just pass the constant "'sha1-" to equalIgnoringCase. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:550: bool isSha256 = equalIgnoringCase(sha256Prefix.toString().characters8(), begin, sha256Prefix.length()); Ditto https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:553: return true; You should call notImplemented() for the isSha256 case. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1257: return checkHash(operativeDirective(m_scriptSrc.get()), hash); Notice the m_scriptSrc here. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1698: sourceSha1.addBytes(source.utf8()); Woah there cowboy. There are a couple of important details to worry about here: 1) What do we do with unencodable code units? 2) Do we want NFC, NFD, NFKC, or NFKD normalization? Basically, we need to call a lower-level unicode API that makes these things explicit. Specifically, you want to call UTF8Encoding()'s functions. Given that this is the web, we probably want NFC normalization and EntitiesForUnencodables, which means calling: UTF8Encoding().normalizeAndEncode(..., EntitiesForUnencodables); Please make sure to include these details in the spec and to test these tricky cases. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1702: StringBuilder hash; Please call |hash.reserveCapacity(...)| with the exact number of characters you'll need. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1705: hash.append(base64Encode(reinterpret_cast<char*>(digest.data()), 20)); 20 -> digest.size() https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:105: bool allowHash(const String& source) const; allowHash -> allowScriptHash ? Presumably this function will look at the script-src directive. We'll need a separate function when we support hash sources in style-src.
I've made the fixes abarth asked for. Also, could someone address my previous comment about hashes for src'd scripts? Namely, I don't believe we should support that. https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoa... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoa... Source/core/dom/ScriptLoader.cpp:313: bool allowScriptHash = elementDocument->contentSecurityPolicy()->allowHash(sourceCode.source()); On 2013/10/17 02:28:09, abarth wrote: > We shouldn't call either of these functions if frameContentSecurityPolicyBypass > is true. They'll generator bogus violation reports. I don't believe that's true. These functions don't generate reports themselves. IIRC, we changed that with my script nonce implementation because we need to do all the other CSP source checks later, so all we want to know here is if it has a valid nonce or hash and thus we can bypass those checks later, and thus don't want to generate any reports here. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:544: sha1Prefix.append("-"); On 2013/10/17 02:28:09, abarth wrote: > There's no reason to use a StringBuilder here. You can just pass the constant > "'sha1-" to equalIgnoringCase. Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:550: bool isSha256 = equalIgnoringCase(sha256Prefix.toString().characters8(), begin, sha256Prefix.length()); On 2013/10/17 02:28:09, abarth wrote: > Ditto Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:553: return true; On 2013/10/17 02:28:09, abarth wrote: > You should call notImplemented() for the isSha256 case. Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1257: return checkHash(operativeDirective(m_scriptSrc.get()), hash); On 2013/10/17 02:28:09, abarth wrote: > Notice the m_scriptSrc here. Ah, right, that's why we have separate script and style functions... https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1698: sourceSha1.addBytes(source.utf8()); On 2013/10/17 02:28:09, abarth wrote: > Woah there cowboy. There are a couple of important details to worry about here: > > 1) What do we do with unencodable code units? > 2) Do we want NFC, NFD, NFKC, or NFKD normalization? > > Basically, we need to call a lower-level unicode API that makes these things > explicit. Specifically, you want to call UTF8Encoding()'s functions. > > Given that this is the web, we probably want NFC normalization and > EntitiesForUnencodables, which means calling: > > UTF8Encoding().normalizeAndEncode(..., EntitiesForUnencodables); > > Please make sure to include these details in the spec and to test these tricky > cases. Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1702: StringBuilder hash; On 2013/10/17 02:28:09, abarth wrote: > Please call |hash.reserveCapacity(...)| with the exact number of characters > you'll need. Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1705: hash.append(base64Encode(reinterpret_cast<char*>(digest.data()), 20)); On 2013/10/17 02:28:09, abarth wrote: > 20 -> digest.size() Done. https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:105: bool allowHash(const String& source) const; On 2013/10/17 02:28:09, abarth wrote: > allowHash -> allowScriptHash ? Presumably this function will look at the > script-src directive. We'll need a separate function when we support hash > sources in style-src. Done.
On 2013/10/18 22:58:04, jww wrote: > I've made the fixes abarth asked for. Also, could someone address my previous > comment about hashes for src'd scripts? Namely, I don't believe we should > support that. Sounds like a good question for the mailing list, but I think I agree with you.
This looks great. I just have a few small comments, but otherwise LGTM. That said, please wait for Adam to sign off on the encoding bits. He knows what he's talking about there; I do not. :) Also, with regard to inline vs src'd scripts: I agree with you that hashes should be limited to inline. I'll comment to that effect on the thread you've started. Thanks! https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoa... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoa... Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; I still think we should rename this, but a) not in this patch, b) I don't have a brilliant idea. :) https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:313: HashSet<String> m_hashes; I don't think we need two sets of hashes here (though we will need two calling functions on ContentSecurityPolicy). This object represents the set of source expressions associated with a given directive. Since we're explicitly scoped to "script-src" or "style-src", I think we can safely just "addSourceHash" to "m_hashes". https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:533: // hash-value = 1*( ALPHA / DIGIT / "+" / "/" / "=" ) Spec: If we want to be explicit about this being base64 encoded, we'll need to change the BNF to note that "=" is a) optional, and b) only valid at the end. I have to believe someone has already defined that somewhere. Adam? :) https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:556: // Base64 encodings may end with exactly one or two '=' characters Might be clearer if you "skipExactly" twice (ignoring the result) rather than looping. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1698: sourceSha1.addBytes(UTF8Encoding().normalizeAndEncode(source, WTF::EntitiesForUnencodables)); This all looks quite sane to me, but I'd like Adam to sign off on it, since he actually knows things about encoding. :) https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1706: if (isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash.toString())) Ok, this makes sense. You put the hash types on the policy object so that you only need to calculate the value once, then you distribute it to all the CSPDirectiveLists for validation. Can you add a comment to that effect in the .h file? https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:113: void usesScriptHashFunction(HashFunctions); This can probably be private. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:163: uint8_t m_sourceHashFunctionsUsed; It surprises me that this is on the policy object, and not on CSPSourceExpression or something else specifically related to a single set of directives. I'll keep reading. :)
https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoa... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoa... Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; On 2013/10/21 07:11:55, Mike West wrote: > I still think we should rename this, but a) not in this patch, b) I don't have a > brilliant idea. :) Makes sense. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:313: HashSet<String> m_hashes; On 2013/10/21 07:11:55, Mike West wrote: > I don't think we need two sets of hashes here (though we will need two calling > functions on ContentSecurityPolicy). This object represents the set of source > expressions associated with a given directive. Since we're explicitly scoped to > "script-src" or "style-src", I think we can safely just "addSourceHash" to > "m_hashes". My only objection is that, in theory, there are valid sets of scripts and styles that might be the same hash, but you only want to authorize as a script *or* a style (but not both). That having been said, I can't think of any attacks you could make by using these scripts, but it seems to me, better safe than sorry and we ought to keep them separate. What do you think? https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:556: // Base64 encodings may end with exactly one or two '=' characters On 2013/10/21 07:11:55, Mike West wrote: > Might be clearer if you "skipExactly" twice (ignoring the result) rather than > looping. Done. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1698: sourceSha1.addBytes(UTF8Encoding().normalizeAndEncode(source, WTF::EntitiesForUnencodables)); On 2013/10/21 07:11:55, Mike West wrote: > This all looks quite sane to me, but I'd like Adam to sign off on it, since he > actually knows things about encoding. :) sgtm https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:1706: if (isAllowedByAllWithHash<&CSPDirectiveList::allowScriptHash>(m_policies, hash.toString())) On 2013/10/21 07:11:55, Mike West wrote: > Ok, this makes sense. You put the hash types on the policy object so that you > only need to calculate the value once, then you distribute it to all the > CSPDirectiveLists for validation. Can you add a comment to that effect in the .h > file? Done. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:113: void usesScriptHashFunction(HashFunctions); On 2013/10/21 07:11:55, Mike West wrote: > This can probably be private. See responses below. https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.h:163: uint8_t m_sourceHashFunctionsUsed; On 2013/10/21 07:11:55, Mike West wrote: > It surprises me that this is on the policy object, and not on > CSPSourceExpression or something else specifically related to a single set of > directives. I'll keep reading. :) I believe you answered this in your later comments, but I've added a comment to explain.
https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... Source/core/frame/ContentSecurityPolicy.cpp:313: HashSet<String> m_hashes; On 2013/10/21 19:18:04, jww wrote: > On 2013/10/21 07:11:55, Mike West wrote: > > I don't think we need two sets of hashes here (though we will need two calling > > functions on ContentSecurityPolicy). This object represents the set of source > > expressions associated with a given directive. Since we're explicitly scoped > to > > "script-src" or "style-src", I think we can safely just "addSourceHash" to > > "m_hashes". > My only objection is that, in theory, there are valid sets of scripts and styles > that might be the same hash, but you only want to authorize as a script *or* a > style (but not both). That having been said, I can't think of any attacks you > could make by using these scripts, but it seems to me, better safe than sorry > and we ought to keep them separate. What do you think? You'd have a policy like `script-src '<hash goes here>'`, right? That creates a CSPSourceList for the 'script-src' directive, containing a hash value. It does nothing for 'style-src'. When you check 'allowScriptHash', you find the 'script-src' CSPDirective, and ask it if the hash matches. Likewise for 'allowStyleHash'. I think that means that we don't need to have multiple hashes for each CSPSourceList, because each source list is tied directly to one directive, and not the other.
On 2013/10/21 19:28:47, Mike West wrote: > https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... > File Source/core/frame/ContentSecurityPolicy.cpp (right): > > https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/Content... > Source/core/frame/ContentSecurityPolicy.cpp:313: HashSet<String> m_hashes; > On 2013/10/21 19:18:04, jww wrote: > > On 2013/10/21 07:11:55, Mike West wrote: > > > I don't think we need two sets of hashes here (though we will need two > calling > > > functions on ContentSecurityPolicy). This object represents the set of > source > > > expressions associated with a given directive. Since we're explicitly scoped > > to > > > "script-src" or "style-src", I think we can safely just "addSourceHash" to > > > "m_hashes". > > My only objection is that, in theory, there are valid sets of scripts and > styles > > that might be the same hash, but you only want to authorize as a script *or* a > > style (but not both). That having been said, I can't think of any attacks you > > could make by using these scripts, but it seems to me, better safe than sorry > > and we ought to keep them separate. What do you think? > > You'd have a policy like `script-src '<hash goes here>'`, right? That creates a > CSPSourceList for the 'script-src' directive, containing a hash value. It does > nothing for 'style-src'. > > When you check 'allowScriptHash', you find the 'script-src' CSPDirective, and > ask it if the hash matches. Likewise for 'allowStyleHash'. I think that means > that we don't need to have multiple hashes for each CSPSourceList, because each > source list is tied directly to one directive, and not the other. Ah, great point! I'll change this.
https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLo... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLo... Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; This has the same problem of eagerly evaluating all these "allow" calls. Given that they can have side effects (like sending violation reports), we don't want to evaluate them eagerly. Instead, we want to do them in a specific order and bail out when we've gotten an answer. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:289: uint8_t getHashFunctionsUsed() const { return m_hashFunctionsUsed; } getHashFunctionsUsed -> hashFunctionsUsed (The "get" prefix doesn't add anything so we usually drop it.) https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:315: uint8_t m_hashFunctionsUsed; 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. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:564: return false; 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? https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:566: hash = String(begin + 1, position - begin - 1); 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. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:1708: hash.append(base64Encode(reinterpret_cast<char*>(digest.data()), digest.size())); 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? https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.h:81: HashFunctionsSha256 = 0x2 It looks like you're using this as a bit field. The style we usually use for that is: HashFunctionsNone = 0, HashFunctionsSha1 = 1 << 1, HashFunctionsSha256 = 1 << 2,
I think I've addressed all of abarth's comments and issues. The biggest change in this CL is that we are now storing the hashes as a struct containing the hash function used and the bit vector form of the hash. I've included the hashing function for the bit vector in ContentSecurityPolicy.cpp, but it is a more general hash function, so I'm tempted to put it in a usable space. If you agree, I'd take suggestions on where it should live. The most obivious place, it seems to me, is in StringHasher.h, since it reuses some of those functions, but I'm not sure. https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLo... File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLo... Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; On 2013/10/22 17:46:49, abarth wrote: > This has the same problem of eagerly evaluating all these "allow" calls. Given > that they can have side effects (like sending violation reports), we don't want > to evaluate them eagerly. Instead, we want to do them in a specific order and > bail out when we've gotten an answer. Done. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:289: uint8_t getHashFunctionsUsed() const { return m_hashFunctionsUsed; } On 2013/10/22 17:46:49, abarth wrote: > getHashFunctionsUsed -> hashFunctionsUsed > > (The "get" prefix doesn't add anything so we usually drop it.) Done. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:315: uint8_t m_hashFunctionsUsed; On 2013/10/22 17:46:49, abarth wrote: > 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. Fixed by other changes. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:564: return false; On 2013/10/22 17:46:49, abarth wrote: > 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? Done. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:566: hash = String(begin + 1, position - begin - 1); On 2013/10/22 17:46:49, abarth wrote: > 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. Hashes are now stored as a struct that contain the hash function used and a bit vector of the hash value. https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:1708: hash.append(base64Encode(reinterpret_cast<char*>(digest.data()), digest.size())); On 2013/10/22 17:46:49, abarth wrote: > 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? Addressed by other changes (now storing the hash as a bit vector). https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.h (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.h:81: HashFunctionsSha256 = 0x2 On 2013/10/22 17:46:49, abarth wrote: > It looks like you're using this as a bit field. The style we usually use for > that is: > > HashFunctionsNone = 0, > HashFunctionsSha1 = 1 << 1, > HashFunctionsSha256 = 1 << 2, Done.
It doesn't look like you've uploaded a new patchset to this CL.
Indeed, my mistake. New CL should be uploaded now.
LGTM. Thanks for iterating on this CL. Below are just some aesthetic nits. https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:443: ContentSecurityPolicy::HashFunctions hashFunction = ContentSecurityPolicy::HashFunctionsNone; Maybe HashAlgorithms instead of HashFunctions? That way you can name this variable |algorithm| instead of |hashFunction|. It's not a big deal either way. It just might read slightly better., https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:583: base64Decode(String(hashBegin, position - hashBegin), hashVector); It's lame that we need to copy these characters into a string just to base64 decode them. I doubt its a big deal. It's just lame. https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:732: m_hashes.add(SourceHashValue(hashFunction, hash)); It's slightly ugly that you've reversed the order of these parameters here. Can you make the order of the parameters consistent?
https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:443: ContentSecurityPolicy::HashFunctions hashFunction = ContentSecurityPolicy::HashFunctionsNone; On 2013/10/29 05:56:26, abarth wrote: > Maybe HashAlgorithms instead of HashFunctions? That way you can name this > variable |algorithm| instead of |hashFunction|. It's not a big deal either way. > It just might read slightly better., Done. https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:583: base64Decode(String(hashBegin, position - hashBegin), hashVector); On 2013/10/29 05:56:26, abarth wrote: > It's lame that we need to copy these characters into a string just to base64 > decode them. I doubt its a big deal. It's just lame. Okay, I changed this by adding a new base64Decode function for UChars. Turns out it could already basically support it, we just needed to create the API to do so. https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/Conten... Source/core/frame/ContentSecurityPolicy.cpp:732: m_hashes.add(SourceHashValue(hashFunction, hash)); On 2013/10/29 05:56:26, abarth wrote: > It's slightly ugly that you've reversed the order of these parameters here. Can > you make the order of the parameters consistent? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/26481005/328001
Failed to apply patch for Source/core/frame/ContentSecurityPolicy.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/frame/ContentSecurityPolicy.cpp
Hunk #1 FAILED at 42.
Hunk #2 succeeded at 50 (offset 1 line).
Hunk #3 succeeded at 89 (offset 1 line).
Hunk #4 succeeded at 297 (offset 1 line).
Hunk #5 succeeded at 307 (offset 1 line).
Hunk #6 succeeded at 323 (offset 1 line).
Hunk #7 succeeded at 333 (offset 1 line).
Hunk #8 succeeded at 432 (offset 1 line).
Hunk #9 succeeded at 531 (offset 1 line).
Hunk #10 succeeded at 721 (offset 1 line).
Hunk #11 succeeded at 843 (offset 1 line).
Hunk #12 succeeded at 880 (offset 1 line).
Hunk #13 succeeded at 909 (offset 1 line).
Hunk #14 succeeded at 1012 (offset 1 line).
Hunk #15 succeeded at 1259 (offset 1 line).
Hunk #16 succeeded at 1451 (offset 1 line).
Hunk #17 succeeded at 1489 (offset 1 line).
Hunk #18 succeeded at 1617 (offset 1 line).
Hunk #19 succeeded at 1701 (offset 1 line).
Hunk #20 succeeded at 1978 (offset 1 line).
1 out of 20 hunks FAILED -- saving rejects to file
Source/core/frame/ContentSecurityPolicy.cpp.rej
Patch: 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
714cf474c180da5b6341c009f56434339f597208..cbcb845468ca8d86eeb560331baeb03cb59cea9b
100644
--- a/Source/core/frame/ContentSecurityPolicy.cpp
+++ b/Source/core/frame/ContentSecurityPolicy.cpp
@@ -42,6 +42,7 @@
#include "core/page/UseCounter.h"
#include "core/platform/ParsingUtilities.h"
#include "platform/JSONValues.h"
+#include "platform/NotImplemented.h"
#include "platform/network/FormData.h"
#include "platform/network/ResourceResponse.h"
#include "weborigin/KURL.h"
@@ -49,11 +50,30 @@
#include "weborigin/SchemeRegistry.h"
#include "weborigin/SecurityOrigin.h"
#include "wtf/HashSet.h"
+#include "wtf/SHA1.h"
+#include "wtf/StringHasher.h"
+#include "wtf/text/Base64.h"
+#include "wtf/text/StringBuilder.h"
#include "wtf/text/TextPosition.h"
#include "wtf/text/WTFString.h"
+namespace WTF {
+
+struct VectorIntHash {
+ static unsigned hash(const Vector<uint8_t>& v) { return
StringHasher::computeHash(v.data(), v.size()); }
+ static bool equal(const Vector<uint8_t>& a, const Vector<uint8_t>& b) {
return a == b; };
+ static const bool safeToCompareToEmptyOrDeleted = true;
+};
+template<> struct DefaultHash<Vector<uint8_t> > {
+ typedef VectorIntHash Hash;
+};
+
+} // namespace WTF
+
namespace WebCore {
+typedef std::pair<unsigned, Vector<uint8_t> > SourceHashValue;
+
// Normally WebKit uses "static" for internal linkage, but using "static" for
// these functions causes a compile error because these functions are used as
// template parameters.
@@ -69,7 +89,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 == '/';
}
@@ -275,6 +297,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 SourceHashValue& hashValue) const { return
m_hashes.contains(hashValue); }
+ uint8_t hashAlgorithmsUsed() const { return m_hashAlgorithmsUsed; }
private:
bool parseSource(const UChar* begin, const UChar* end, String& scheme,
String& host, int& port, String& path, bool& hostHasWildcard, bool&
portHasWildcard);
@@ -283,12 +307,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, Vector<uint8_t>& hash,
ContentSecurityPolicy::HashAlgorithms&);
void addSourceSelf();
void addSourceStar();
void addSourceUnsafeInline();
void addSourceUnsafeEval();
void addSourceNonce(const String& nonce);
+ void addSourceHash(const ContentSecurityPolicy::HashAlgorithms&, const
Vector<uint8_t>& hash);
ContentSecurityPolicy* m_policy;
Vector<CSPSource> m_list;
@@ -297,6 +323,8 @@ private:
bool m_allowInline;
bool m_allowEval;
HashSet<String> m_nonces;
+ HashSet<SourceHashValue> m_hashes;
+ uint8_t m_hashAlgorithmsUsed;
};
CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy, const String&
directiveName)
@@ -305,6 +333,7 @@ CSPSourceList::CSPSourceList(ContentSecurityPolicy* policy,
const String& direct
, m_allowStar(false)
, m_allowInline(false)
, m_allowEval(false)
+ , m_hashAlgorithmsUsed(0)
{
}
@@ -403,6 +432,16 @@ bool CSPSourceList::parseSource(const UChar* begin, const
UChar* end, String& sc
addSourceNonce(nonce);
return true;
}
+
+ Vector<uint8_t> hash;
+ ContentSecurityPolicy::HashAlgorithms algorithm =
ContentSecurityPolicy::HashAlgorithmsNone;
+ if (!parseHash(begin, end, hash, algorithm))
+ return false;
+
+ if (hash.size() > 0) {
+ addSourceHash(algorithm, hash);
+ return true;
+ }
}
const UChar* position = begin;
@@ -492,16 +531,54 @@ 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))
+ if ((position + 1) != end || *position != '\'' || !(position -
nonceBegin))
return false;
nonce = String(nonceBegin, position - nonceBegin);
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,
Vector<uint8_t>& hash, ContentSecurityPolicy::HashAlgorithms& hashAlgorithm)
+{
+ 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;
+ hashAlgorithm = ContentSecurityPolicy::HashAlgorithmsSha1;
+ } 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;
+
+ Vector<char> hashVector;
+ base64Decode(hashBegin, position - hashBegin, hashVector);
+ hash.append(reinterpret_cast<uint8_t*>(hashVector.data()),
hashVector.size());
+ return true;
+}
+
// ; <scheme> production from RFC 3986
// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
//
@@ -644,6 +721,12 @@ void CSPSourceList::addSourceNonce(const String& nonce)
m_nonces.add(nonce);
}
+void CSPSourceList::addSourceHash(const ContentSecurityPolicy::HashAlgorithms&
algorithm, const Vector<uint8_t>& hash)
+{
+ m_hashes.add(SourceHashValue(algorithm, hash));
+ m_hashAlgorithmsUsed |= algorithm;
+}
+
class CSPDirective {
public:
CSPDirective(const String& name, const String& value,
ContentSecurityPolicy* policy)
@@ -760,6 +843,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 SourceHashValue& hashValue) const { return
m_sourceList.allowHash(hashValue); }
+
+ uint8_t hashAlgorithmsUsed() const { return
m_sourceList.hashAlgorithmsUsed(); }
private:
CSPSourceList m_sourceList;
@@ -794,6 +880,7 @@ public:
bool allowBaseURI(const KURL&, ContentSecurityPolicy::ReportingStatus)
const;
bool allowScriptNonce(const String&) const;
bool allowStyleNonce(const String&) const;
+ bool allowScriptHash(const SourceHashValue&) const;
void gatherReportURIs(DOMStringList&) const;
const String& evalDisabledErrorMessage() const { return
m_evalDisabledErrorMessage; }
@@ -822,6 +909,7 @@ private:
bool checkEval(SourceListDirective*) const;
bool checkInline(SourceListDirective*) const;
bool checkNonce(SourceListDirective*, const String&) const;
+ bool checkHash(SourceListDirective*, const SourceHashValue&) const;
bool checkSource(SourceListDirective*, const KURL&) const;
bool checkMediaType(MediaListDirective*, const String& type, const String&
typeAttribute) const;
@@ -924,6 +1012,11 @@ bool CSPDirectiveList::checkNonce(SourceListDirective*
directive, const String&
return !directive || directive->allowNonce(nonce);
}
+bool CSPDirectiveList::checkHash(SourceListDirective*…
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/26481005/368001
Message was sent while issue was closed.
Change committed as 160866 |
