CSPv3 with nonce support and stricter rules

[sid77]

Hi 👋 , This is a spike for adding CSPv3 with nonce support and a stricter rules set as well.

CSPv3 is still a working draft and currently supported by the latest stable versions of Chrome only, Firefox just recently added support for the 'strict-dynamic' keyword which is expected to be rolled out with the next stable version (52.0).

From my tests, though, current code base doesn't require 'strict-dynamic' support, I left it there as a possible solution for removing 'unsafe-eval' (see below).

What is currently provided:

• src/auth.go can now outputs both URL-encoded and standard base64 strings, I believe URL-encoded randomString() output could have worked as well for CSP nonces but I preferred to provide the standard base64 version for them. At this point it might make sense to move this function outside src/auth.go and into a proper utilities package. Or maybe not 😄

• src/api.go is now serving a stricter CSPv3 header and provides CSP nonce support as well. I've implemented it with a custom http.ResponseWriter for static contents which replace any occurrence of {{ nonce }} in the body with the currently injected CSP nonce from the header. Again: does it make sense to move all of this into a new src/csp.go package or something like that?

• static/index.html has been updated to use the {{ nonce }} token mentioned above

• .gitignore ignores the built ./interlock binary, which presence was driving me crazy during development 😛

What is missing:

• 'unsafe-eval' should be removed from current CSP rules set

• maybe some code cleanup

Regarding 'unsafe-eval': to my understanding, this entry is currently needed to allow dynamic script loading with jQuery getScript(). The entry should go away for a robust CSP rule set, otherwise an attacker can execute possibly injected code.

I'm still trying to work around the need of 'unsafe-eval', maybe leveraging 'strict-dynamic' as mentioned above.

Anyway, before submitting more code I'd like to see if this spike makes sense for futures development of interlock. What do you think?

Cheers, Marco

[abarisani]

Hi Marco and thanks for this contribution.

I think merging this should be postponed until CSPv3 is widely adopted as I assume, without having it tested, that this would break IE and Safari as it is, correct?

Otherwise is there a way to merge it and keeping non CSPv3 browsers happy? I guess not as it would kinda defy the point of CSP in the first place but I am open to suggestions.

Also please note that we require copyright assignment from 3rd party contributions to the original one, (so your would need to remove your copyright from your pull request). I am sorry if this is inconvenient for you.

Thanks!

[abarisani]

Concerning .gitignore, you have 2 options without affecting the repository:

1) have a .gitignore with interlock as well as .gitignore itself 2) set your own private rules in .git/info/exclude

Having said that your .gitignore, added to the repository, is considered best practice (even if I personally don't like it and favor personal .gitignore files outside the repository). Therefore I am happy to take that pull request but I think it should be separate from the CSPv3 stuff.

[danbia]

Hi Marco, regarding the unsafe-eval: the entry is not currently needed only to allow dynamic script loading with jQuery getScript(). unsafe-eval affects the eval() but also its relatives like setTimeout(String), setInterval(String), and new Function(String) constructor. We agree that in the long term we should probably removeunsafe-eval from the CSP rules set but this would require various changes to the code.

[sid77]

Hi everybody again! I'll try to address all the comments below.

I think merging this should be postponed until CSPv3 is widely adopted as I assume, without having it tested, that this would break IE and Safari as it is, correct?

Ah, yes, sure! This is a spike, a POC. I wanted to throw out some code and check with you if the ideas are OK with the project before doing more work on it.

Otherwise is there a way to merge it and keeping non CSPv3 browsers happy? I guess not as it would kinda defy the point of CSP in the first place but I am open to suggestions.

If a browser doesn't support CSPv3 it will simply ignores the 'strict-dynamic' keyword. A CSPv3 to CSPv1 backward compatible policy for current code base should be:

default-src 'none';
script-src 'strict-dynamic' 'nonce-random1234' 'self' 'unsafe-eval';
style-src 'self' 'unsafe-inline';
img-src 'self';
connect-src 'self';

If a browser supports up to CSPv3, the nonces allow scripts execution. 'self' is ignored because of 'strict-dynamic' presence. As a plus,'strict-dynamic' propagates execution property to resources loaded from whitelisted entries.

If a browser supports up to CSPv2, 'strict-dynamic' is ignored and scripts are run using nonces. 'self' is part of the whitelisted entries in CSPv2 but with this code base its presence is not needed

If a browser supports up to CSPv1, both 'strict-dynamic' and nonces are ignored. Script execution is allowed by 'self'.

Google CSP evaluator is quite useful to toy around with policies. It also highlight an ugly shortcoming of whitelisting 'self' (or any other domain-based entries) as JSONP endpoints (if any) can be used to bypass CSP.

I've also just realised that the way I'm applying the nonce just doesn't work as the Content-Length header is not being updated 🙄

With this in mind a smaller step should be to go for this policy:

default-src 'none';
script-src 'self' 'unsafe-eval';
style-src 'self' 'unsafe-inline';
img-src 'self';
connect-src 'self';

Which is what is currently being served with a default-deny applied on top of it.

Also please note that we require copyright assignment from 3rd party contributions to the original one, (so your would need to remove your copyright from your pull request). I am sorry if this is inconvenient for you.

Works for me

.gitignore topic

I'll cleanup, split and resubmit

'unsafe-eval' topic

From my tests it looks like in this code base it's only needed for jQuery getScript() but I admit I'm not super skilled in JS and, honestly, don't have a working solution to drop the eval need either.

Thanks for all the inputs!

[danbia]

Hi Marco, regarding the 'unsafe-eval' topic. I confirm that this would affect also other functions apart from the 'getScript()'.