User Agent: Mozilla/5.0 (X11; CrOS x86_64 8350.68.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36 Steps to reproduce: `data:text/html,<meta http-equiv="content-security-policy" content="script-src 'nonce-{yay}'"><script nonce="{yay}">alert(1);</script>` throws an error in Chrome and doesn't execute the script. In Firefox, it executes `alert(1)`. Expected results: Firefox should reject `{` and `}` characters, as they aren't part of the `base64-value` grammar.

Here's a patch that changes both nonce- and hash-source to check for valid base64-values, per spec (https://w3c.github.io/webappsec-csp/#grammardef-nonce-source). Note that there are a couple of blink-contributed web platform tests that accept invalid values for the nonce (there can only be one or two equal-signs at the END of a base64 value, not in the middle). I changed those as well. I'm not sure if it's sufficient to just change the files in-tree, or if changes to them will also have to be mentioned to the Blink developers? Try seems fine with it (discounting the scary-looking unrelated oranges there): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b514efbfde11f7a22cd6d32ed793ddf3f3694178

(In reply to Thomas Wisniewski from comment #1) > Note that there are a couple of blink-contributed web platform tests that > accept invalid values for the nonce (there can only be one or two > equal-signs at the END of a base64 value, not in the middle). I changed > those as well. I'm not sure if it's sufficient to just change the files > in-tree, or if changes to them will also have to be mentioned to the Blink > developers? If these tests are wrong and they pass in Blink, could this be a problem from a webcompat point of view? Maybe we should add telemetry to see how often we encounter invalid CSP hashes.

Another option is to just not be 100% compliant with the base64-values grammar for now, and relax it so that equal signs can be anywhere in the string, like Chrome apparently treats this. I'm open to either suggestion, as I don't really feel that being more restrictive than Chrome is as big of a problem than being less restrictive than Chrome, in this case. But I'm pretty new to CSP considerations, so what do you think?

Comment on attachment 8814259 [details] [diff] [review] 1309219-only_allow_valid_base64-values_for_CSP_nonce_and_hash_sources.diff Review of attachment 8814259 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/nsCSPParser.cpp @@ +660,5 @@ > > +bool IsValidBase64Value(const char16_t* cur, const char16_t* end) > +{ > + // May end with one or two = > + if (end >= cur && *(end-1) == EQUALS) end--; If `end == cur` (i.e. a one-character string), then `*(end-1)` would be looking at the character before the first one. ::: dom/security/test/gtest/TestCSPParser.cpp @@ +552,5 @@ > + "script-src 'none'" }, > + { "script-src 'nonce-invalid==='", > + "script-src 'none'" }, > + { "script-src 'sha256-invalid==='", > + "script-src 'none'" }, Could you add tests for these degenerate cases? - "sha256-===" - "sha256-==" - "sha256-=" - "sha256-" and the matching nonce-* ones.

(In reply to Thomas Wisniewski from comment #3) > Another option is to just not be 100% compliant with the base64-values > grammar for now, and relax it so that equal signs can be anywhere in the > string, like Chrome apparently treats this. If we match the Blink behaviour then we don't really have to worry about breaking sites. Mike, are the equal signs something you'd be willing to fix quickly in Blink?

Here's an updated patch addressing the review comments. But I'll hold off on seeking a final review until we determine if we'd like to relax the equals-sign restriction.

Our handling of '=' will be fixed in https://codereview.chromium.org/2535473002. Thanks for the report!

Comment on attachment 8814276 [details] [diff] [review] 1309219-only_allow_valid_base64-values_for_CSP_nonce_and_hash_sources.diff Alright, thanks Mike! If the Blink team is going to do this as well, then I suppose we might as well land this. Mind giving the latest patch another review, François?

Thanks! The patch still applies cleanly to inbound, so I'm requesting check-in.

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad8afe2b218 Only allow valid base64-values for CSP nonce and hash sources, per spec. r=francois

Thomas, thanks for taking on the work and fixing this issue. Any chance I could convince you to file a follow up to land the testcase of comment 0 and do some minor code cleanups? E.g. we have helpers like isCharacterToken, or also isNumberToken which we should use.

Sure, I don't mind making a follow-up patch. Should we file a fresh bug, or just continue in this one? But why do you feel we need to add the data-URL test in comment 0, given that the gtests in my patch should cover these changes? If it's just to add a test covering a data-URL visit like this, then I'll need some pointers on how to even test such a thing (having the harness visit a data-URL, then confirm whether it loaded/ran).

(In reply to Thomas Wisniewski from comment #13) > Sure, I don't mind making a follow-up patch. Should we file a fresh bug, or > just continue in this one? I think it would be better to file a follow up - thanks! > But why do you feel we need to add the data-URL test in comment 0, given > that the gtests in my patch should cover these changes? If it's just to add > a test covering a data-URL visit like this, then I'll need some pointers on > how to even test such a thing (having the harness visit a data-URL, then > confirm whether it loaded/ran). Ah, I missed that part. Yeah, gtests cover exactly that case - awesome.