Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CSP nonces #1871

Merged
merged 3 commits into from
Oct 25, 2016
Merged

Use CSP nonces #1871

merged 3 commits into from
Oct 25, 2016

Conversation

LukasReschke
Copy link
Member

@LukasReschke LukasReschke commented Oct 24, 2016

CSP nonces are a feature available with CSP v2. Basically instead of saying "JS resources from the same domain are ok to be served" we now say "Ressources from everywhere are allowed as long as they add a nonce attribute to the script tag with the right nonce.

At the moment the nonce is basically just a <?php p(base64_encode($_['requesttoken'])) ?>, we have to decode the requesttoken since : is not an allowed value in the nonce. So if somebody does on their own include JS files (instead of using the addScript public API, they now must also include that attribute.)

IE does currently not implement CSP v2, thus there is a whitelist included that delivers the new CSP v2 policy to newer browsers. Check http://caniuse.com/#feat=contentsecuritypolicy2 for the current browser support list. An alternative approach would be to just add 'unsafe-inline' as well as 'unsafe-inline' is ignored by CSPv2 when a nonce is set. But this would make this security feature unusable at all in IE. Not worth it at the moment IMO.

Implementing this offers the following advantages:

  1. Security: As we host resources from the same domain by design we don't have to worry about 'self' anymore being in the whitelist
  2. Performance: We can move oc.js again to inline JS. This makes the loading way quicker as we don't have to load on every load of a new web page a blocking dynamically non-cached JavaScript file.

If you want to toy with CSP see also https://csp-evaluator.withgoogle.com/


cc @rullzer

@mention-bot
Copy link

@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Henni, @BernhardPosselt and @DeepDiver1975 to be potential reviewers.

CSP nonces are a feature available with CSP v2. Basically instead of saying "JS resources from the same domain are ok to be served" we now say "Ressources from everywhere are allowed as long as they add a `nonce` attribute to the script tag with the right nonce.

At the moment the nonce is basically just a `<?php p(base64_encode($_['requesttoken'])) ?>`, we have to decode the requesttoken since `:` is not an allowed value in the nonce. So if somebody does on their own include JS files (instead of using the `addScript` public API, they now must also include that attribute.)

IE does currently not implement CSP v2, thus there is a whitelist included that delivers the new CSP v2 policy to newer browsers. Check http://caniuse.com/#feat=contentsecuritypolicy2 for the current browser support list. An alternative approach would be to just add `'unsafe-inline'` as well as `'unsafe-inline'` is ignored by CSPv2 when a nonce is set. But this would make this security feature unusable at all in IE. Not worth it at the moment IMO.

Implementing this offers the following advantages:

1. **Security:** As we host resources from the same domain by design we don't have to worry about 'self' anymore being in the whitelist
2. **Performance:** We can move oc.js again to inline JS. This makes the loading way quicker as we don't have to load on every load of a new web page a blocking dynamically non-cached JavaScript file.

If you want to toy with CSP see also https://csp-evaluator.withgoogle.com/

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke LukasReschke added the 3. to review Waiting for reviews label Oct 24, 2016
@LukasReschke LukasReschke added this to the Nextcloud 11.0 milestone Oct 24, 2016
@LukasReschke LukasReschke changed the title [WIP] Use CSP nonce Use CSP nonce Oct 24, 2016
@LukasReschke LukasReschke changed the title Use CSP nonce Use CSP nonces Oct 24, 2016
@rullzer
Copy link
Member

rullzer commented Oct 24, 2016

Awesome! 🎈

@@ -47,6 +47,7 @@
'script',
[
'src' => $linkToJs,
'nonce' => base64_encode(\OC::$server->getCsrfTokenManager()->getToken()->getEncryptedValue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some kind of CSPNounceManager? That if we ever need to change the way it is generated we just have to do so in 1 place and everything keeps working.

For now it could just be a simple wrapper of course.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Let's do that. 👍

Copy link
Member Author

@LukasReschke LukasReschke Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@ChristophWurst
Copy link
Member

So if somebody does on their own include JS files (instead of using the addScript public API, they now must also include that attribute.)

😢 how? This will break the mail app as we're using requirejs to dynamically load scripts in debug mode. On production, we use addScript for the packaged version though.

@LukasReschke
Copy link
Member Author

LukasReschke commented Oct 24, 2016

😢 how? This will break the mail app as we're using requirejs to dynamically load scripts in debug mode. On production, we use addScript for the packaged version though.

Fix it? 😉

… or tell me how to setup that mail app to that state to experience that together with that branch. Like with stupid commands including composer install etc.

Everything that goes further than git clone is annoying and I won't figure out magically ;-)

@BernhardPosselt
Copy link
Member

Everything that goes further than git clone is annoying and I won't figure out magically ;-)

Running make should take care of exactly that

@LukasReschke
Copy link
Member Author

Running make should take care of exactly that

Been there. Done that. 90% of the fancy makefiles that people include in apps explode totally on OS X. I won't execute random makefiles anymore ;-)

@LukasReschke
Copy link
Member Author

LukasReschke commented Oct 24, 2016

Plus having to install npm, bower and all other kind of random tools just sucks big time…

That's like: "Sure you can contribute. You hopefully have the right OS in the right version and then you have to execute these bunch of tools so that it just works" 🙈 🙊 🙉

Honestly, my time is too rare for that and I do see that as a major contribution blocking bummer… But anyways, off topic.

@BernhardPosselt
Copy link
Member

@LukasReschke should IMHO be addressed, I'd recommend to file bugs on the respective repos

@LukasReschke
Copy link
Member Author

LukasReschke commented Oct 24, 2016

Ok. That should be what requireJS needs to work again:

diff --git a/js/require_config.js b/js/require_config.js
index 7e90525..5b1a5d1 100644
--- a/js/require_config.js
+++ b/js/require_config.js
@@ -11,8 +11,8 @@
  */

 (function() {
-   'use strict';

+   'use strict';
    requirejs.config({
        baseUrl: './../../../apps/mail/js',
        paths: {
@@ -44,6 +44,18 @@
        baseUrl: OC.linkTo('mail', 'js')
    });

+   requirejs.createNode = function (config, moduleName) {
+       var node = config.xhtml ?
+           document.createElementNS('http://www.w3.org/1999/xhtml', 'html:script') :
+           document.createElement('script');
+       node.type = config.scriptType || 'text/javascript';
+       node.charset = 'utf-8';
+       node.async = true;
+
+       node.setAttribute("nonce", btoa(OC.requestToken));
+       return node;
+   };
+
    require([
        'init'
    ]);

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@codecov-io
Copy link

codecov-io commented Oct 24, 2016

Current coverage is 57.30% (diff: 92.68%)

Sunburst

Diff Coverage File Path
0% apps/theming/appinfo/app.php
••• 33% lib/private/Server.php
•••••••••• 100% ...lic/AppFramework/Http/EmptyContentSecurityPolicy.php
•••••••••• 100% core/templates/layout.base.php
•••••••••• 100% core/templates/layout.user.php
•••••••••• 100% lib/private/Security/CSRF/CsrfToken.php
•••••••••• 100% new ...e/Security/CSP/ContentSecurityPolicyNonceManager.php
•••••••••• 100% lib/private/Security/CSRF/CsrfTokenManager.php
•••••••••• 100% ...ate/AppFramework/DependencyInjection/DIContainer.php
•••••••••• 100% ...Framework/Middleware/Security/SecurityMiddleware.php

Review all 11 files changed

No coverage report found for master at cfae91a.

Powered by Codecov. Last update cfae91a...ee8b8ad

@rullzer
Copy link
Member

rullzer commented Oct 25, 2016

Very cool especially the cool things we can do with it.
Looking good and even tests!
LGTM!

@MorrisJobke
Copy link
Member

MorrisJobke commented Oct 25, 2016

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 8957436 into master Oct 25, 2016
@MorrisJobke MorrisJobke deleted the use-csp-nonces branch October 25, 2016 12:46
LukasReschke added a commit to nextcloud/mail that referenced this pull request Oct 25, 2016
LukasReschke added a commit to nextcloud/mail that referenced this pull request Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants