Closed Bug 979580 Opened 8 years ago Closed 8 years ago

Un-pref nonce-source and hash-source

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
relnote-firefox --- 31+
relnote-b2g --- ?

People

(Reporter: grobinson, Assigned: grobinson)

References

Details

Attachments

(1 file, 2 obsolete files)

CSP 1.1 is still being revised in the WG, but two of the most requested features, nonce-source and hash-source, have been implemented in Firefox for some time now. They are behind a pref. I propose that we un-pref these features with the goal of landing them "on by default" in Firefox 29.

At the point, the situation will be "CSP 1.0 + some features from 1.1". These features will be nonce-source, hash-source, and frame-ancestors (which we've had implemented since X-Content-Security-Policy, and is only now included in the spec).

This, and the trend of gradually rolling out the new 1.1 features it implies, should not be a problem for compatibility: if sites use unrecognized directives or source-expressions, the CSP parser should ignore them. This may result in less protection than developers intended, but this is acceptable because CSP is intended to be defense-in-depth.

The only thing we need to conclusively resolve in regards to nonce/hash-source is the treatment of 'unsafe-inline'. There is a proposal [0] to ignore 'unsafe-inline' in a directive if at least one nonce-source or hash-source is present, for backwards compatibility. This needs to be conclusively resolved in the spec and implemented in FF before it hits release, so we don't have to backtrack on this behavior.

[0] http://lists.w3.org/Archives/Public/public-webappsec/2013Dec/0044.html
Assignee: nobody → grobinson
Attached patch 979580-unpref-nonce-hash.patch (obsolete) — Splinter Review
Remove the "experimentalEnabled" pref checks for nonce and hash, and stop turning the pref on in the tests via pushPrefEnv. Note that the pref and the pref observer are left in so we can use them later. test_{nonce,hash}_source.html pass.
Attachment #8385813 - Flags: review?(sstamm)
Comment on attachment 8385813 [details] [diff] [review]
979580-unpref-nonce-hash.patch

Review of attachment 8385813 [details] [diff] [review]:
-----------------------------------------------------------------

As I understand it, this pref no longer disables anything.  Will there be experimental directives coming that we will put behind this?

I assume you've run this through try to make sure we don't break any b2g or CSP tests.  Sorry it took me so long to get to this!

r=me with a yes to my first question and a green try run.
Attachment #8385813 - Flags: review?(sstamm) → review+
Component: Security → DOM: Security
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Comment on attachment 8385813 [details] [diff] [review]
> 979580-unpref-nonce-hash.patch
> 
> Review of attachment 8385813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As I understand it, this pref no longer disables anything.  Will there be
> experimental directives coming that we will put behind this?

As we continue to implement new spec stuff, it may be useful to use this pref on features that are experimental (hence the name) and may change, as well as to time features in order to synchronize with other implementations.

> I assume you've run this through try to make sure we don't break any b2g or
> CSP tests.  Sorry it took me so long to get to this!
> 
> r=me with a yes to my first question and a green try run.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c078079046b
Try run looks good, except for one orange. It's also a CSP test, test_CSP_inlinescript.html, and it times out. Looks like some of the tests fail to run due to this logged error:

 0:12.11 ************************************************************
 0:12.11 * Call to xpconnect wrapped JSObject produced this error:  *
 0:12.11 [Exception... "[JavaScript Error: "policy._directives[directive] is undefined" {file: "file:///home/garrett/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/components/contentSecurityPolicy.js" line: 245}]'[JavaScript Error: "policy._directives[directive] is undefined" {file: "file:///home/garrett/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/components/contentSecurityPolicy.js" line: 245}]' when calling method: [nsIContentSecurityPolicy::getAllowsHash]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
 0:12.11 ************************************************************

Should be a pretty easy fix.
Attached patch 979580-fix-broken-tests (obsolete) — Splinter Review
Fix a bug that was uncovered on try. The nonce/hash tests did not test any policy that only set default-src, and as a result handled that case incorrectly. Since nonce/hash cannot be set on default-src, it is sufficient to ignore policies that do not have a corresponding {script,style}-src directive.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d87ef7b25488
Attachment #8392320 - Flags: review?(sstamm)
Oops, that try push didn't include both patches. Correct try push: https://tbpl.mozilla.org/?tree=Try&rev=332edae1142e
Status: NEW → ASSIGNED
Try looks good.
Comment on attachment 8392320 [details] [diff] [review]
979580-fix-broken-tests

Review of attachment 8392320 [details] [diff] [review]:
-----------------------------------------------------------------

Yep.  That'll fix that js error.  Feel free to fold the two patches together before landing (attach the folded patch to the bug please).

Also, what's with the browser chrome tests failing on the try run?  Are those expected based on the parent cset?
Attachment #8392320 - Flags: review?(sstamm) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8)
> Also, what's with the browser chrome tests failing on the try run?  Are
> those expected based on the parent cset?

I starred most of them, all appeared to be known or related to known oranges.
Folded, added commit message.
Attachment #8385813 - Attachment is obsolete: true
Attachment #8392320 - Attachment is obsolete: true
Attachment #8395023 - Flags: review+
In Comment 1, I mentioned that the treatment of "unsafe-inline" in the context of {nonce,hash}-source still needs to be resolved. Currently, nonce and hash are like any other source expression. If combined with unsafe-inline, they are rendered useless because all inline scripts are allowed. This matches Chrome's behavior.

The proposal was to toggle it such that unsafe-inline is not enforced if used in conjunction with nonce or hash. This was for backwards compatibility. I will follow up in the WG to see if we can make a final decision on this. Implementing this will be easy - just need to change the order of checks at the inline script/style call sites. If we do decide to implement this behavior, will track in a followup bug.
https://hg.mozilla.org/mozilla-central/rev/b96e87685369
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Yay, hashes and nonces!

Garrett, re: your question in comment 12, yes, the intention is to disable 'unsafe-inline' if either a nonce or hash source expression is present. That's in the spec at [1], and I'm about to upload a patch to Blink for review. I intend for that to be in when we ship hashes and nonces.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=979580
(In reply to Mike West from comment #14)
> Yay, hashes and nonces!
> 
> Garrett, re: your question in comment 12, yes, the intention is to disable
> 'unsafe-inline' if either a nonce or hash source expression is present.
> That's in the spec at [1], and I'm about to upload a patch to Blink for
> review. I intend for that to be in when we ship hashes and nonces.
> 
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=979580

Just noticed this comment, thanks for chiming in, Mike! I'll file a follow-up bug to implement that behavior change.
Blocks: 1004703
relnote-firefox: --- → ?
relnote-b2g: --- → ?
Matthew, do you know some MDN documentation on this subject? It would be nice to have a link to point to.
For now, I added in the release notes "Enable CSP 1.1 nonce-source and hash-source by default"
Flags: needinfo?(MattN+bmo)
Updated the note to "CSP 1.1 nonce-source and hash-source enabled by default" -- I think we're OK without a link here, developers who care about CSP will know what this means and how to find more info.
Flags: needinfo?(MattN+bmo)
Blocks: 1177074
You need to log in before you can comment on or make changes to this bug.