There is much discussion on webappsec about divergent approaches to whitelisting inline scripts and styles. One approach, script-nonce/nonce-source, is being explored in Bug 855326. Another approach is script-hash, which would allow scripts/styles to include a hash attribute containing some hash of their contents, which the browser is responsible for validating. If the hash matches and the CSP is set to allow it, then the browser will execute the chosen script/style. This bug can be used to track experiments in this direction including patches.

My description of how script-hash could work is not actually what is being discussed - see URL for the most recent proposal.

Update URL with latest proposal.

This is a working approach to implementing hash-source. I had to refactor the violation reporting at both the hash and nonce callsites, and also ended up refactoring the nonce test to be more specific (addl. benefit: it is much simpler now). Per our discussion, this does *not* implement the "nonce/hash overrides 'unsafe-inline'" behavior proposed on webappsec [0], which is part of the latest draft spec [1]. It would be easy to implement, however, by simply checking nonce/hash before 'unsafe-inline'. [0] http://lists.w3.org/Archives/Public/public-webappsec/2013Jul/0019.html [1] http://lists.w3.org/Archives/Public/public-webappsec/2013Sep/0053.html

Why again are we not creating JS classes for the new nonce and hash source expressions to abstract the differing logic behind the same method names (toString, etc)?

I should elaborate. The idea would be to create a couple of classes that look similar to the CSPSource class: CSPHashSource and CSPNonceSource or something. Then we could augment the CSPSource factory (.create) to identify source-expressions that are nonces and hashes and parse them as their own classes (CSPHashSource.fromString, etc). That would give us a CSPSourceList with heterogeneous elements, but as long as they all have permits(), toString(), clone(), etc, they can work interchangeably. This also lets us use instanceof to check whether a source is a hash or nonce without having to check _hash or _nonce.

Creates separate CSPNonceSource and CSPHashSource classes to abstract source-expression-specific logic and avoid putting too much in CSPSource.

Comment on attachment 8334283 [details] [diff] [review] WIP 2 Review of attachment 8334283 [details] [diff] [review]: ----------------------------------------------------------------- Watch out for trailing whitespace. Looks pretty good, clean up, think about the various source classes/factories, and flag me and mrbkap for review. Also, is there a bug filed for refactoring the CSP boilerplate out of each test and into a head.js file? If not, please file one because the test_CSP*.html mochitest files are all starting to look really similar. ::: content/base/src/CSPUtils.jsm @@ +1401,5 @@ > + > + // check for a hash-source match > + if (R_HASHSRC.test(aStr)) { > + if (!CSPPrefObserver.experimentalEnabled) return null; > + return new CSPHashSource(aStr); Please use the factory pattern for consistency, or change CSPSources to use inheritence. CSPSource would be a really simple base class and extend it to match the ABNF's sub-types: CSPSchemeSource, CSPHostSource, CSPKeywordSource, CSPNonceSource, CSPHashSource. This way all the objects are created consistently. If you make CSPSource be a base class, there's a good example of a clear way to make the child classes in accessible/src/jsat/Presentation.jsm ... though you will probably have to reimplement each method for each child type anyway, so inheritence doesn't get you much here. Also don't forget to wire in a reference to aCSPRep or self if it's needed (like for console logging). Though on a quick scan it looks like CSPSource._CSPRep is never used, so we could probably just remove it. @@ +1741,5 @@ > > +this.CSPNonceSource = function CSPNonceSource(aStr) { > + let nonceSourceMatch = R_NONCESRC.exec(aStr); > + this._nonce = nonceSourceMatch[1]; > +} If you do this in a factory method instead of a constructor then the caller can use the factory to test and parse in one step: if (aSrc = CSPNonceSource.fromString(str)) { return aSrc; } And the factory can be: CSPNonceSource.fromString = function blah(aStr) { let nsm = R_NONCESRC.exec(aStr); if (!nsm) { return null; } var aSrc = new CSPNonceSource(); aSrc._nonce = nonceSourceMatch[1]; }; This is how the other objects create each other. It allows the factory to decide not only what type of thing to make (so you're not locked into the constructor) and also do error checking with an opportunity to return null. @@ +1774,5 @@ > + > +this.CSPHashSource = function CSPHashSource(aStr) { > + let hashSrcMatch = R_HASHSRC.exec(aStr); > + this._algo = hashSrcMatch[1]; > + this._hash = hashSrcMatch[2]; You should probably validate the algorithm here too so that an invalid one causes a parse error. @@ +1809,5 @@ > + Components.Constructor("@mozilla.org/security/hash;1", > + "nsICryptoHash", > + "initWithString"); > + // TODO: handle invalid algo > + var hash = new CryptoHash(this._algo); Do that at parse time so it's not revalidated on every permits() call. ::: content/base/src/contentSecurityPolicy.js @@ +235,5 @@ > + for (policy of this._policies) ]; > + > + shouldReportViolation.value = this._policies.some(function(policy, i) { > + return policy._directives[directive]._hasHashSource && !policyAllowsHash[i]; > + }); Please add a comment here that explains why this extra logic is needed. ("Violations are only reported for policies where there's a hash source to violate" or something like that) ::: content/base/src/nsScriptLoader.cpp @@ +433,3 @@ > NS_ENSURE_SUCCESS(rv, false); > + if (reportInlineViolation) > + violations.AppendElement(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT); Nit: in this file, please use braces around the if body. ::: content/base/test/csp/file_hash_source.html^headers^ @@ +1,1 @@ > +Content-Security-Policy: script-src 'sha256-WbAC34SXjnKYS6XkS87UxGSlYw3J+E+kWMIOCLhsWHY='; style-src 'sha256-AIbZ1sS8bt3AqlTRdxcad7U+3jlpClg0x/fy2tFSQWM='; Please copy the hash values into the .html file too (in a comment) in case we change the .html file and don't want to forget to update this header. ::: content/base/test/csp/test_nonce_source.html @@ +50,5 @@ > ok(/_bad/.test(testid), "Blocked URI with testid " + testid); > ranTests(1); > } catch (e) { > + // if the subject is blocked inline, data will be a violation message > + // we can't distinguish which resources triggered these, so we ignore them Wait, really? We can't differentiate between nonce and hash violations?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7) > Comment on attachment 8334283 [details] [diff] [review] > WIP 2 > Please use the factory pattern for consistency Done. > If you make CSPSource be a base class, there's a good example of a clear way > to make the child classes in accessible/src/jsat/Presentation.jsm ... though > you will probably have to reimplement each method for each child type > anyway, so inheritence doesn't get you much here. For that reason, I think it's probably better just to duck type like we do now. > Also don't forget to wire in a reference to aCSPRep or self if it's needed > (like for console logging). Though on a quick scan it looks like > CSPSource._CSPRep is never used, so we could probably just remove it. I think I'll leave it out for now, since, as you say, it is not used anywhere. It would be easy to wire in later if needed. > @@ +1741,5 @@ > > > > +this.CSPNonceSource = function CSPNonceSource(aStr) { > > + let nonceSourceMatch = R_NONCESRC.exec(aStr); > > + this._nonce = nonceSourceMatch[1]; > > +} > > If you do this in a factory method instead of a constructor then the caller > can use the factory to test and parse in one step: > > if (aSrc = CSPNonceSource.fromString(str)) { > return aSrc; > } > > And the factory can be: > CSPNonceSource.fromString = function blah(aStr) { > let nsm = R_NONCESRC.exec(aStr); > if (!nsm) { return null; } > var aSrc = new CSPNonceSource(); > aSrc._nonce = nonceSourceMatch[1]; > }; > > This is how the other objects create each other. It allows the factory to > decide not only what type of thing to make (so you're not locked into the > constructor) and also do error checking with an opportunity to return null. > Switched to the factory pattern. I still think I need to do the R_NONCESRC/R_HASHSRC test in CSPSource.fromString. You need to be able to distinguish between the case where CSPHashSource returns null because the regex didn't match (you should continue, and try to match other possible sources), and the case where it returns null because it matched the regex but experimental features are not enabled (you should not continue, it's not going to match anything else). > You should probably validate the algorithm here too so that an invalid one > causes a parse error. With the permissible algos defined in R_HASHSRC, this is unnecessary as long as those algos correspond to valid arguments for nsICryptoHash.init(), since an invalid algo will cause the regex matching to fail and the constructor will never be called. I've split out the list of valid hash algos into its own variable and added a comment to make this clear. > Wait, really? We can't differentiate between nonce and hash violations? No (we can), but we can't distinguish between *different* nonce violations - they all have the same observer message. That's why I switched the tests to use the "change a style attribute" technique so it's possible to distinguish them.

Updated patch reflecting last feedback round. This patch also undoes the overloading of "permits" from the implementation of nonce-source, and creates two separate but parallel threads for verifying nonces and hashes. I think this easier to reason about and avoid bugs with, but it does make shouldLoad a bit ugly (violation reporting is still not done for that case).

Comment on attachment 8342783 [details] [diff] [review] WIP 3 Review of attachment 8342783 [details] [diff] [review]: ----------------------------------------------------------------- Obviously the printfs and such need to go, but the direction looks good. I like the separation out of permitsHash and permitsNonce (and the new source types), but am a bit concerned about the recalculation of hashes (and passing of big values to hash) for violation reporting. Flag me for review when ready. Also grab reviewers for the JS and layout files. ::: content/base/src/CSPUtils.jsm @@ +1029,5 @@ > + slObj._hasNonceSource = true; > + > + if (src._hash) > + slObj._hasHashSource = true; > + I think you want "src instanceof CSPNonceSource" (etc) here, right? @@ +1786,5 @@ > + let hashSourceObj = new CSPHashSource(); > + let hashSrcMatch = R_HASHSRC.exec(aStr); > + hashSourceObj._algo = hashSrcMatch[1]; > + hashSourceObj._hash = hashSrcMatch[2]; > + return hashSourceObj; You might want to do additional error checking here to make sure that _algo and _hash are nonnull. ::: content/base/src/contentSecurityPolicy.js @@ +231,5 @@ > + > + let directive = ContentSecurityPolicy._MAPPINGS[aContentType]; > + // allow it to execute? > + let policyAllowsHash = [ policy.permitsHash(aContent, directive) > + for (policy of this._policies) ]; "allow it to execute?" is slightly misleading here. Delete the comment. @@ +262,5 @@ > * can recheck to determine which policies were violated and send the > * appropriate reports. > */ > logViolationDetails: > + function(aViolationType, aSourceFile, aScriptSample, aLineNum, aNonce, aContent) { Please document aContent in the comment block. Is there another way we could get the hash into here? Can we calculate the hash outside the call to logViolationDetails? This could be a perf issue if we're passing around massive content blocks that need to get hashed. Maybe the initial call to permitsHash could be permitsHashedValue() that returns the hash as an outparam, then that outparam can be fed into the logging? This means two functions: one to check permitsHash on content and one to check it on an already-generated hash. Since this is pretty much internal to CSP, we can create a second permitsPrecomputedHash call that's not in the IDL that takes a hash instead of the content to hash so the logging doesn't have to recompute the hash. ::: content/base/test/csp/test_nonce_source.html @@ +75,3 @@ > } > > function checkStyles () { styles and scripts, right?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10) > Comment on attachment 8342783 [details] [diff] [review] > WIP 3 > > Review of attachment 8342783 [details] [diff] [review]: > ----------------------------------------------------------------- > > Obviously the printfs and such need to go, but the direction looks good. I > like the separation out of permitsHash and permitsNonce (and the new source > types), but am a bit concerned about the recalculation of hashes (and > passing of big values to hash) for violation reporting. Flag me for review > when ready. Also grab reviewers for the JS and layout files. Is "passing big values to hash" actually a problem?. They're nsCOMPtr's. I agree about avoiding recalculation for logViolationDetails (although I see that as a larger architectural problem). > ::: content/base/src/CSPUtils.jsm > @@ +1029,5 @@ > > + slObj._hasNonceSource = true; > > + > > + if (src._hash) > > + slObj._hasHashSource = true; > > + > > I think you want "src instanceof CSPNonceSource" (etc) here, right? Actually, just removed that code entirely as it is no longer used. > @@ +1786,5 @@ > > + let hashSourceObj = new CSPHashSource(); > > + let hashSrcMatch = R_HASHSRC.exec(aStr); > > + hashSourceObj._algo = hashSrcMatch[1]; > > + hashSourceObj._hash = hashSrcMatch[2]; > > + return hashSourceObj; > > You might want to do additional error checking here to make sure that _algo > and _hash are nonnull. Ok. > ::: content/base/src/contentSecurityPolicy.js > @@ +231,5 @@ > > + > > + let directive = ContentSecurityPolicy._MAPPINGS[aContentType]; > > + // allow it to execute? > > + let policyAllowsHash = [ policy.permitsHash(aContent, directive) > > + for (policy of this._policies) ]; > > "allow it to execute?" is slightly misleading here. Delete the comment. Ok. > @@ +262,5 @@ > > * can recheck to determine which policies were violated and send the > > * appropriate reports. > > */ > > logViolationDetails: > > + function(aViolationType, aSourceFile, aScriptSample, aLineNum, aNonce, aContent) { > > Please document aContent in the comment block. Is there another way we > could get the hash into here? Can we calculate the hash outside the call to > logViolationDetails? This could be a perf issue if we're passing around > massive content blocks that need to get hashed. > > Maybe the initial call to permitsHash could be permitsHashedValue() that > returns the hash as an outparam, then that outparam can be fed into the > logging? This means two functions: one to check permitsHash on content and > one to check it on an already-generated hash. Since this is pretty much > internal to CSP, we can create a second permitsPrecomputedHash call that's > not in the IDL that takes a hash instead of the content to hash so the > logging doesn't have to recompute the hash. That's a good idea, and could be a good solution for now (again, I think having to re-check each policy when we call logViolationDetails is something that should be fixed generally, but the C++ rewrite is probably the best place to tackle that). > ::: content/base/test/csp/test_nonce_source.html > @@ +75,3 @@ > > } > > > > function checkStyles () { > > styles and scripts, right? Yup, forgot to update that after rewriting the test. Thanks!

(In reply to Garrett Robinson [:grobinson] from comment #11) > Actually, just removed that code entirely as it is no longer used. I was mistaken - that code was kept (and now uses instanceof).

This is the first version of the patch that I feel is ready to land. Implements most of the suggestions from the last feedback from sstamm. The only thing I did not implement was the suggested workaround for having to pass to compute the hash a second time in logViolationDetails. That workaround is unfortunately not very clean because we do not know which hash functions are being used by the policy, so we would have to compute all possible hashes the first time around and then return an array of the possible values. This is actually more wasteful unless a policy uses all the possible different hash functions, which we believe will be uncommon (but should be supported). I believe the real solution here is to fix violation reporting with the new CSP multipolicy implementation, and that is something in discussion for bug 925004. dholbert, please review the changes to nsStyleUtil.cpp. mrbkap, please review the changes to nsScriptLoader.cpp. All that remains to do is more comprehensive tests, for both failure and edge cases. Since this feature is experimental and pref'ed off by default, I am not sure if this should be a requirement to land this, or can be tracked in a follow-up.

Comment on attachment 8349597 [details] [diff] [review] 883975-hash-source-1.patch >+++ b/layout/style/nsStyleUtil.cpp >+ nsTArray<unsigned short> violations; Since this is a local variable and small, it should be nsAutoTArray (which uses stack-allocated memory, up to N entries), to avoid unnecessary heap churn. Looks like the maximum size of this is 3 (I think?), so nsAutoTArray<unsigned short, 3> should be good. (or maybe 5 or 10, if you anticipate it maybe storing more violation types in the future). >+ bool reportNonceViolation = false; > nsAutoString nonce; >- // If inline styles are allowed ('unsafe-inline'), skip the (irrelevant) >- // nonce check > if (!allowInlineStyle) { > // We can only find a nonce if aContent is provided >- foundNonce = !!aContent && >+ bool foundNonce = !!aContent && > aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::nonce, nonce); > if (foundNonce) { [snip] > rv = csp->GetAllowsNonce(nonce, nsIContentPolicy::TYPE_STYLESHEET, >- &reportViolation, &allowInlineStyle); >+ &reportNonceViolation, &allowInlineStyle); > if (NS_FAILED(rv)) { > if (aRv) > *aRv = rv; > return false; > } >+ >+ if (reportNonceViolation) { >+ violations.AppendElement(nsIContentSecurityPolicy::VIOLATION_TYPE_NONCE_STYLE); >+ } reportNonceViolation is only used in the "if (allowNonce)" clause, so we should improve its scoping. Just declare it right before the GetAllowsNonce() call, and don't bother giving it a default value. (both of which will make it more consistent with the reportInlineViolation variable up higher) >+ bool reportHashViolation = false; >+ if (!allowInlineStyle) { >+ nsAutoString styleText(aStyleText); >+ rv = csp->GetAllowsHash(styleText, nsIContentPolicy::TYPE_STYLESHEET, >+ &reportHashViolation, &allowInlineStyle); >+ if (NS_FAILED(rv)) { >+ if (aRv) >+ *aRv = rv; >+ return false; >+ } >+ if (reportHashViolation) { >+ violations.AppendElement(nsIContentSecurityPolicy::VIOLATION_TYPE_HASH_STYLE); >+ } >+ } Same here -- move reportHashViolation into the "if", and don't give it a default value. >+ if (violations.Length() > 0) { Style nit: layout code (and I think gecko code in general?) tends to use IsEmpty instead of Length() > 0 (or Length() != 0), with nsTArrays. So, replace with: if (!violations.IsEmpty()) >+ for (int i=0; i < violations.Length(); i++) { s/int i=0/uint32_t i = 0/ (need spaces around the "=", and the signed-ness of the variable needs to match the signed-ness of Length()) (technically the *type* of the variable should match, but the full (templated) size_type expression for what Length() returns is too long to bother, and in practice it's equivalent to uint32_t.)

(I think all of those nits also apply to nsScriptLoader.cpp, too; looks like that code is pretty similar)

Comment on attachment 8349597 [details] [diff] [review] 883975-hash-source-1.patch Drive-by notes on other chunks of the patch: >+++ b/content/base/public/nsIContentSecurityPolicy.idl >+interface nsIDOMHTMLElement; Is this needed? It doesn't seem to be used in any of the new lines in this file. >+ boolean getAllowsHash(in AString aContent, >+ in unsigned short aContentType, >+ out boolean shouldReportViolation); >+ (Looks like this new function needs a chunk of documentation. > * (optional) If this is a nonce violation, include the nonce so we can > * recheck to determine which policies were violated and send the > * appropriate reports. > */ > void logViolationDetails(in unsigned short violationType, > in AString sourceFile, > in AString scriptSample, > in int32_t lineNum, >- [optional] in AString nonce); >+ [optional] in AString nonce, >+ [optional] in AString content); Looks like the new param (content) needs to be added to the documentation. >+++ b/content/base/src/CSPUtils.jsm >+ // When this is true, the source list contains at least one nonce-source >+ this._hasNonceSource = false; >+ >+ // When this is true, the source list contains at least one nonce-source >+ this._hasHashSource = false; s/nonce/hash/ in the second comment >diff --git a/content/base/src/contentSecurityPolicy.js b/content/base/src/contentSecurityPolicy.js >+ shouldReportViolation.value = this._policies.some(function(policy, i) { >+ // Don't report a violation if the policy didn't use nonce-source >+ return policy._directives[directive]._hasNonceSource && !policyAllowsNonce[i]; >+ }); [...] >+ shouldReportViolation.value = this._policies.some(function(policy, i) { >+ // Violations are only reported for policies with a hash source to violate >+ return policy._directives[directive]._hasHashSource && !policyAllowsHash[i]; >+ }); Nit: might be worth wording those comments more consistently, since they seem to describe identical behavior (just for nonce vs. hash) >+++ b/content/base/src/nsScriptLoader.cpp >+using std::vector; ^ This 'using' statement is unused. :) (I'm guessing because you replace your vector usage with nsTArray) >@@ -607,17 +646,18 @@ nsScriptLoader::ProcessScriptElement(nsI > NS_ASSERTION(mDocument->GetCurrentContentSink() || > aElement->GetParserCreated() == FROM_PARSER_XSLT, >- "Non-XSLT Defer script on a document without an active parser; bug 592366."); >+ // XXX putthe semicolon after parser back before committing (stupid Sublime) >+ "Non-XSLT Defer script on a document without an active parser bug 592366."); Looks like you might want to revert this change? (looks like this was just a temporary local tweak to make your editor happy?) (If it really breaks your editor, I'm sure you could replace the semicolon with a hyphen or something, though it might be nicer to do that separately from this patch, since it's non-functional and not really related to what this patch is doing.)

Updated patch with feedback from dholbert.

Comment on attachment 8349659 [details] [diff] [review] 883975-hash-source-2.patch Review of attachment 8349659 [details] [diff] [review]: ----------------------------------------------------------------- r=me. CSP bits look good. Deferring to dholbert and mrbkap for the layout and js changes. Maybe file a follow-up bug for the optimization for not recomputing the hash for violation reports... ::: content/base/src/contentSecurityPolicy.js @@ +775,5 @@ > + if (nonceSourceValid && aContext instanceof Ci.nsIDOMHTMLElement) { > + let nonce = aContext.getAttribute('nonce'); > + if (nonce) > + permitted = permitted || policy.permitsNonce(nonce, cspContext); > + } You can skip the entire nonce-source stuff here if permitted is true before the if block. This can be rewritten as: // check if location is permitted let permitted = policy.permits(aContentLocation, cspContext); // check any valid nonce if location is not permitted if (!permitted && nonceSourceValid && aContext instanceof Ci.nsIDOMHTMLElement && aContext.hasAttribute('nonce')) { permitted = policy.permitsNonce(aContext.getAttribute('nonce')); } It's just different syntax and a slight optimization, but in case you want to make this slightly easier to read... Do you still need to finish this TODO for the violation reporting, or is there a follow-up bug for this?

Comment on attachment 8349659 [details] [diff] [review] 883975-hash-source-2.patch >+++ b/content/base/public/nsIContentSecurityPolicy.idl >@@ -117,16 +117,20 @@ interface nsIContentSecurityPolicy : nsI >+ boolean getAllowsHash(in AString aContent, >+ in unsigned short aContentType, >+ out boolean shouldReportViolation); >+ (As noted in IRC /msg - looks like this new function in the IDL file still needs documentation) >+++ b/layout/style/nsStyleUtil.cpp >+ if (!allowInlineStyle) { >+ bool reportHashViolation; >+ nsAutoString styleText(aStyleText); >+ rv = csp->GetAllowsHash(styleText, nsIContentPolicy::TYPE_STYLESHEET, >+ &reportHashViolation, &allowInlineStyle); No need to declare |styleText| here. Just directly pass aStyleText through to GetAllowsHash. >+ // What violation(s) should be reported? >+ // >+ // 1. If the style tag has a nonce attribute, and the nonce does not match >+ // the policy, report VIOLATION_TYPE_NONCE_STYLE. >+ // 2. If the policy has at least one hash-source, and the hashed contents of >+ // the style tag did not match any of them, report VIOLATION_TYPE_HASH_STYLE >+ // 3. Otherwise, report VIOLATION_TYPE_INLINE_STYLE. Add something like "...if appropriate." at the end of that last line. (Otherwise, it sounds kinda like we unconditionally report VIOLATION_TYPE_INLINE_STYLE, even when there was no actual violation.) > // This inline style is not allowed by CSP, so report the violation > nsAutoCString asciiSpec; > aSourceURI->GetAsciiSpec(asciiSpec); > nsAutoString styleText(aStyleText); >+ nsAutoString styleSample(styleText); [snip] >- styleText.Truncate(40); [snip] >+ styleSample.Truncate(40); As above, you don't actually need a |styleText| variable here anymore. (We needed it in the old code, because we modified it; but now you're modifying styleSample instead, so you can drop styleText entirely and replace all its usages with aStyleText.) >+ for (uint32_t i = 0; i < violations.Length(); i++) { >+ // Skip reporting the redundant inline style violation if there are >+ // other (nonce and/or hash violations) as well. >+ if (violations.Length() > 1 && >+ violations[i] == nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE) { >+ continue; >+ } >+ csp->LogViolationDetails(violations[i], NS_ConvertUTF8toUTF16(asciiSpec), >+ styleSample, aLineNumber, nonce, styleText); >+ } Question: Am I correct in thinking that the 0th entry in |violations| is guaranteed to be VIOLATION_TYPE_INLINE_STYLE? i.e. we don't expect to ever add other entries to |violations| without first adding that one? If that's correct (and I'm pretty sure it is), let's make it explicit with an assertion just before the loop, e.g. MOZ_ASSERT(violations[0] == nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_STYLE, "how did we get any violations without first hitting an inline style violation?"); ...and let's simplify the loop body (dropping the "continue" and removing the extra check of every violations[i]) -- let's replace it with something like: // We don't report the 0th violation (VIOLATION_TYPE_INLINE_STYLE) unless // it's the only one. if (i > 0 || violations.Length() == 1) { csp->LogViolationDetails(violations[i], NS_ConvertUTF8toUTF16(asciiSpec), styleSample, aLineNumber, nonce, styleText); } > if (!allowInlineStyle) { >- NS_ASSERTION(reportViolation, >+ NS_ASSERTION(violations.Length() > 0, Use IsEmpty() here, too. r=me on the nsStyleUtil.cpp stuff with that.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #18) > Do you still need to finish this TODO for the violation reporting, or is > there a follow-up bug for this? No, that was leftover. The violation reporting could be improved (right now it just reports the URI that was blocked, and doesn't specify that it was blocked because of a nonce violation). I'd like it to say that, and also give the offending nonce. This should be easy but unfortunately is not because the violation reporting code is a mess. I'd like to track making the reporting more detailed in a follow-up (that would also rewrite and hopefully simplify a lot of the reporting) if possible.

Incorporates feedback from sstamm and dholbert. r+ from sstamm and dholbert. Try: https://tbpl.mozilla.org/?tree=Try&rev=ab7c91eaf37d

Comment on attachment 8350332 [details] [diff] [review] 883975-hash-source-3.patch One nit on the test: >+++ b/content/base/test/csp/file_hash_source.html >+ window.parent.inlineScriptTestResult("allowed", "blocked", "This script has does not have an allowed hash for scripts"); s/has does/does/ Also: one other thing that needs testing (maybe included in the group of "edge cases" you mentioned in comment 13): we should test content with *both* nonce-source and hash-source protecting the same inline-script, with all the various combinations of {both valid, only one valid, neither valid}. (Same for inline-style.) I don't think we need to block landing this on those tests existing, but I think we should add them before enabling this by default. Can you make sure a followup bug is filed for that & any other tests you want to add for this? [Setting explicit r+ since I forgot to set the flag on the previous version]

Comment on attachment 8350332 [details] [diff] [review] 883975-hash-source-3.patch One other nit on the tests: It might be better to split out the *_nonce_source.html tweaks into their own patch, since (I think?) those changes don't depend on the hash-related changes. That way, if the nonce_source tweaks end up causing some fallout (like a randomorange) and need to be backed out, we can easily revert them without having to also yank the rest of this patch; and similarly, if we have to backout the hash stuff for some reason, we won't have to clobber your nonce improvements while we're at it.

Comment on attachment 8350332 [details] [diff] [review] 883975-hash-source-3.patch Also -- the failure messages for the nonce test (and probably the hash one, too) really should be reworded to talk about what *should* happen, instead of declaring what *did* happen (since we don't actually know what happened). For example, consider this is() check: >--- a/content/base/test/csp/test_nonce_source.html >+ is(getElementColorById('inline-script-incorrect-nonce'), black, >+ "Inline script with incorrect nonce did not execute"); If that check fails, we're going to get a failure message in the logs that looks something like this: TEST_UNEXPECTED FAIL: inline script with incorrect nonce did not execute. ...and that message *sounds* like it's saying "the test failed because the script did not execute" But really, it *means* to say the script actually *did* execute (but should not have done so). Anyway -- if you replace "did not" with "should not", the ambiguity goes away.

Comment on attachment 8350332 [details] [diff] [review] 883975-hash-source-3.patch Sorry, one final nit: >diff --git a/content/base/test/csp/mochitest.ini b/content/base/test/csp/mochitest.ini >--- a/content/base/test/csp/mochitest.ini >+++ b/content/base/test/csp/mochitest.ini > [test_CSP_bug941404.html] >+[test_hash_source.html] >\ No newline at end of file Please add a newline to the end of this file (after the line for your test).

Thanks for the additional comments, dholbert! I will file a follow-up for the additional tests and make sure it is done before this is turned on by default. Additionally, try looks good except for build bustage on B2G ICS, which apparently does not think nsAutoTArray::AppendElement is valid (https://tbpl.mozilla.org/php/getParsedLog.php?id=32247976&tree=Try&full=1). Will dig into this tomorrow.

(In reply to Garrett Robinson [:grobinson] from comment #26) > Additionally, try looks good except for build bustage on B2G ICS, which > apparently does not think nsAutoTArray::AppendElement is valid > (https://tbpl.mozilla.org/php/getParsedLog.php?id=32247976&tree=Try&full=1). Specifically, I think it's complaining that the thing you're passing into AppendElement is the wrong type. It says: { error: no matching function for call to 'nsAutoTArray<short unsigned int, 3u>::AppendElement(nsIContentSecurityPolicy::<anonymous enum>)' } I'll bet a static_cast<unsigned short>() around the enum value should fix that.

Implements fixes from dholbert's last comments. Still needs review of nsScriptLoader.cpp from mrbkap. I split out the changes to the nonce source tests; they will be uploaded in a separate patch. Both the nonce and hash tests pass with this code.

Carrying over r+ from sstamm on previous patch (these are the changes to the nonce-source test, split into a separate patch per dholbert's suggestion).

Try run with both patches applied: https://tbpl.mozilla.org/?tree=Try&rev=9919da094b52

Adds edge case tests for the various possible combinations of nonce- and hash-source discussed in Comment 22.

Comment on attachment 8355684 [details] [diff] [review] 883975-hash-source.patch >--- a/content/base/src/CSPUtils.jsm >+++ b/content/base/src/CSPUtils.jsm >-const R_NONCESRC = new RegExp ("^'nonce-([a-zA-Z0-9\+\/]+)'$", 'i'); >+const R_NONCESRC = new RegExp ("^'nonce-([a-zA-Z0-9+/=]+)'$", 'i'); This change (new in the latest patch) doesn't look hash-related. Should it be in a different bug? (or at least different patch?) Also, this looks like it might be incorrect. I'm guessing the "=" addition there is to match "=" characters at the end of a nonce value (in "base64-value" defined at http://w3c.github.io/webappsec/specs/content-security-policy/csp-specification.dev.html ), but I believe that the regexp you have there will also allow "=" characters anywhere in the middle of a nonce value, which isn't supposed to happen. Maybe I'm missing something, though. (I haven't looked through the usages of this regexp in detail.)

(In reply to Daniel Holbert [:dholbert] from comment #32) > Comment on attachment 8355684 [details] [diff] [review] > 883975-hash-source.patch > > >--- a/content/base/src/CSPUtils.jsm > >+++ b/content/base/src/CSPUtils.jsm > >-const R_NONCESRC = new RegExp ("^'nonce-([a-zA-Z0-9\+\/]+)'$", 'i'); > >+const R_NONCESRC = new RegExp ("^'nonce-([a-zA-Z0-9+/=]+)'$", 'i'); > > This change (new in the latest patch) doesn't look hash-related. Should it > be in a different bug? (or at least different patch?) I spotted that while I was writing the tests - we recently updated the spec to include '=' as an allowable character in nonces/hashes. I will split this into a separate patch, but think it makes sense to keep it attached to this bug because this change came out of the WG discussion around the hash source spec proposal [0]. [0] http://lists.w3.org/Archives/Public/public-webappsec/2013Dec/0004.html > Also, this looks like it might be incorrect. I'm guessing the "=" addition > there is to match "=" characters at the end of a nonce value (in > "base64-value" defined at > http://w3c.github.io/webappsec/specs/content-security-policy/csp- > specification.dev.html ), but I believe that the regexp you have there will > also allow "=" characters anywhere in the middle of a nonce value, which > isn't supposed to happen. Good catch, I've updated the patch to correctly match "base64-value"s (and also removed the case-insensitive match flag, which is obviously incorrect here).

Rewrites the tests in the manner of the nonce tests rewrite, and for the same reason - it is difficult to distinguish multiple inline script failures due to the limitations of CSP violation reporting. Goes a bit further and DRYs things up in a way that I think is very nice, readable, and extensible. Adds tests for sha512, sha384, sha1, and md5, so this patch also closes bug 956437. Re-flagging sstamm for review since the tests have changed significantly. mrbkap, I just need you to review the changes to nsScriptLoader.cpp.

Comment on attachment 8356377 [details] [diff] [review] 883975-hash-source.patch Review of attachment 8356377 [details] [diff] [review]: ----------------------------------------------------------------- r=me with my comments here addressed. ::: content/base/public/nsIContentSecurityPolicy.idl @@ +136,5 @@ > + * always false. > + * @return > + * Whether or not this inline resource is whitelisted by a hash-source > + */ > + boolean getAllowsHash(in AString aContent, Need to bump the IID here. ::: content/base/src/nsScriptLoader.cpp @@ +438,3 @@ > NS_ENSURE_SUCCESS(rv, false); > + if (reportInlineViolation) { > + violations.AppendElement(static_cast<unsigned short>(nsIContentSecurityPolicy::VIOLATION_TYPE_INLINE_SCRIPT)); Is there any way to make this line any shorter? I'm tempted to suggest pulling the static_cast<...>(...) out into local constants, but even just splitting this line after the AppendElement( might do it.

Implements fixes from mrbkap's review. Prepared commit message for landing.

Comment on attachment 8357384 [details] [diff] [review] 883975-hash-source.patch Review of attachment 8357384 [details] [diff] [review]: ----------------------------------------------------------------- Accidentally cleared r? for sstamm - waiting for confirmation on rewritten tests.

Prepare commit message for landing.

Comment on attachment 8357384 [details] [diff] [review] 883975-hash-source.patch Review of attachment 8357384 [details] [diff] [review]: ----------------------------------------------------------------- Tests look good to me. Worth a run on try, though. ::: content/base/test/csp/file_hash_source.html @@ +13,5 @@ > + <p id="inline-script-valid-md5-hash">blocked</p> > + > + <!-- 'sha256-siVR8vAcqP06h2ppeNwqgjr0yZ6yned4X2VF84j4GmI=' --> > + <script>document.getElementById("inline-script-valid-hash").innerHTML = "allowed";</script> > + <script>document.getElementById("inline-script-invalid-hash").innerHTML = "allowed";</script> Calculate the hashes for the invalid blocks too and put them in comments here noting these are the hashes, but they're not allowed by CSP. That way if we mess up the hash values in the policy it's easier to detect with a quick grep. ::: content/base/test/csp/test_hash_source.html @@ +131,5 @@ > + function() { > + // save this for last so that our listeners are registered. > + // ... this loads the testbed of good and bad requests. > + document.getElementById('cspframe').src = 'file_hash_source.html'; > + document.getElementById('cspframe').addEventListener('load', checkInline, false); If styles get computed at all out of sync with the load event, these tests could create a random orange... I think load fires after the styles are set though, so it *should* work.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #39) > I think load fires after the styles are set > though, so it *should* work. That's correct.

Update test comments per sstamm's recommendation.

Try (both patches): https://tbpl.mozilla.org/?tree=Try&rev=e1b36679378f

Comment on attachment 8357460 [details] [diff] [review] 883975-hash-source.patch >+++ b/content/base/src/nsScriptLoader.cpp >@@ -419,64 +419,105 @@ CSPAllowsInlineScript(nsIScriptElement * > if (!allowInlineScript) { >- NS_ASSERTION(reportViolation, >+ NS_ASSERTION(violations.Length() > 0, > "CSP blocked inline script but is not reporting a violation"); Nit: This should be > NS_ASSERTION(!violations.IsEmpty(), instead of checking Length() > 0, for consistency with the equivalent style chunk. (No need to restart the Try run just for that change, but probably worth fixing before landing.)

Fixed dholbert's nit.

Try looks pretty good, but there are test failures on B2G. Investigating now.

There is a problem with creating the nsICryptoHash object on multiprocess (confirmed on B2G and e10s). Every call to create this object (new CryptoHash) in CSPUtils.jsm causes this error: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE) [nsIJSCID.createInstance]" nsresult: "0x80570015 (NS_ERROR_XPC_CI_RETURNED_FAILURE)" location: "JS frame :: resource://gre/modules/CSPUtils.jsm :: CSPHashSource.prototype.permits :: line 1831" data: no] ************************************************************

nsICryptoHash fundamentally cannot be used by child processes because NSS is not available to child processes by design. This is a tricky issue, but it should not block landing this patch, since this is an experimental feature and is pref'ed off by default. I have disabled test_hash_source on B2G in this patch. Try: https://tbpl.mozilla.org/?tree=Try&rev=42f3a734cc96

Try looks good.

The second patch is bitrotted. Please rebase.

Un-bitrotted.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d570802145c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/64712081790e

https://hg.mozilla.org/mozilla-central/rev/d570802145c9 https://hg.mozilla.org/mozilla-central/rev/64712081790e https://hg.mozilla.org/mozilla-central/rev/7eac28351524

And followup, https://hg.mozilla.org/integration/b2g-inbound/rev/5ec1de269424, since it doesn't work in debug b2g either (no surprise, we just enjoy pain so we have two separate json files in rather different orders).