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

Allow AppFramework applications to specify a custom CSP header #13989

Merged
merged 2 commits into from Feb 18, 2015

Conversation

Projects
None yet
8 participants
@LukasReschke
Member

LukasReschke commented Feb 9, 2015

This change allows AppFramework applications to specify a custom CSP header for example when the default policy is too strict. Furthermore this allows us to partially migrate away from inline-CSS and allowed eval() in our JavaScript components.

Legacy ownCloud components will still use the previous policy. Application developers can use this as following in their controllers:

$response = new TemplateResponse('activity', 'list', []);
$csp = new ContentSecurityPolicy();
$csp->addAllowedScriptDomain('www.owncloud.org');
$response->setContentSecurityPolicy($csp);
return $response;

Fixes #11857 which is a pre-requisite for #13458 and #11925

@Raydiation Care to take a look at this? :)
@DeepDiver1975 You as well? – This is required in case we want to harden our CSP policy even further to allow applications such as documents to have some lightened rules. Might break one or two things but we will notice this fast and can port the specific controllers that break to use the AppFramework :-)

@owncloud-bot

This comment has been minimized.

Contributor

owncloud-bot commented Feb 9, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9070/
Test PASSed.

@BernhardPosselt

This comment has been minimized.

Contributor

BernhardPosselt commented Feb 15, 2015

Yes, the Response class seems indeed the right place to tackle it. As for the naming: *Helper is redundant. There are two ways to do it IMHO:

$csp = new OCP\AppFramework\Http\CSP();
$csp->allowImageSrc('some.domain.com');
$response = new Response();
$response->setCSP($csp);
$csp = 'some string with csp settings';
$response->setCSP($csp);
// or
$response->allowImageSrc('some.domain.com');

The second one is easier to use because you dont need to read up on how to use the CSP class but I think the first approach is the correct one because it is easier to set default values and allow users to overwrite safe defaults and prevent easy mistakes

For instance we never want the dev to set the complete csp string at once because he'll likely make a mistake.

Will review further later ;)

* @param bool $state
* @return $this
*/
public function inlineScriptState($state = false) {

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 15, 2015

Contributor

Id use something that implies the boolean more, like allowInlineScript($enabled = false), same for the following methods

/**
* @return string
*/
public function getPolicy() {

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 15, 2015

Contributor

Maybe use buildPolicy or createPolicy to imply that this is not a simple and very fast getter

*
* @package OCP\AppFramework\Http
*/
class ContentSecurityPolicyHelper {

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 15, 2015

Contributor

Rename to either CSP or ContentSecurityPolicy

if($this->lastModified) {
$mergeWith['Last-Modified'] =
$this->lastModified->format(\DateTime::RFC2822);
}
if(!isset($this->headers['Content-Security-Policy'])) {
$contentSecurityPolicy = new ContentSecurityPolicyHelper();

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 15, 2015

Contributor

Shouldnt we restrict CSP to the template response? I dont think the headers are needed in any other response than the one which actually serves HTML.

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 15, 2015

Contributor

We should also add a method to set the CSP like private function setCSP or setContentSecurityPolicy(ContentSecurityPolicy $csp) because we want to make it type safe and add nice code completion for IDEs

This comment has been minimized.

@LukasReschke

LukasReschke Feb 16, 2015

Member

Shouldnt we restrict CSP to the template response? I dont think the headers are needed in any other response than the one which actually serves HTML.

Mhm… Actually CSP makes as well sense on JSON or Text Responses since browsers tend to do some nasty things sometimes and a second line of defence wouldn't hurt here.

We should also add a method to set the CSP like private function setCSP or setContentSecurityPolicy(ContentSecurityPolicy $csp) because we want to make it type safe and add nice code completion for IDEs

True :)

@BernhardPosselt

This comment has been minimized.

Contributor

BernhardPosselt commented Feb 15, 2015

Good idea and nicely executed, you're getting good at this ;)

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 16, 2015

Thanks @Raydiation for reviewing. Much appreciated. I'll take a look at the comments today and incorporate them :)

@LukasReschke LukasReschke force-pushed the enhancment/security/11857 branch from be875ff Feb 16, 2015

Allow AppFramework applications to specify a custom CSP header
This change allows AppFramework applications to specify a custom CSP header for example when the default policy is too strict. Furthermore this allows us to partially migrate away from CSS and allowed eval() in our JavaScript components.

Legacy ownCloud components will still use the previous policy. Application developers can use this as following in their controllers:
```php
$response = new TemplateResponse('activity', 'list', []);
$cspHelper = new ContentSecurityPolicyHelper();
$cspHelper->addAllowedScriptDomain('www.owncloud.org');
$response->addHeader('Content-Security-Policy', $cspHelper->getPolicy());
return $response;
```

Fixes #11857 which is a pre-requisite for #13458 and #11925

@LukasReschke LukasReschke force-pushed the enhancment/security/11857 branch to b20174b Feb 16, 2015

@@ -930,15 +930,6 @@
),
/**
* Custom CSP policy, changing this will overwrite the standard policy
*/
'custom_csp_policy' =>

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Feb 16, 2015

Member

so - there is no need to configure this globally anymore?

I guess many installations out there already tweak this - e.g. to allow the installation to be iframed etc.

What's the migration path for such a scenario? @LukasReschke THX

This comment has been minimized.

@LukasReschke

LukasReschke Feb 16, 2015

Member

I guess many installations out there already tweak this - e.g. to allow the installation to be iframed etc.

Iframing is defined by another policy ;-)

The thing that might break are people using inline JS for example for tracking using Piwik / Google Analytics. But honestly, if you do such stuff you should also know how to fix it yourself (as in: It does not make sense to ship insecure defaults because of those few people ;-)

This comment has been minimized.

@LukasReschke

LukasReschke Feb 16, 2015

Member

Migration path: "Fix your blackmagic. This was anyways just a very ugly workaround in times where we didn't have the AppFramework and wanted apps to specify policies on their own."

This comment has been minimized.

@LukasReschke
@owncloud-bot

This comment has been minimized.

Contributor

owncloud-bot commented Feb 16, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9474/
Test PASSed.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 16, 2015

Analysis Tools Updated
Some analysis tools were updated to newer versions. This might result in changes which are unrelated to your code changes!
PHP Analyzer: 1.6.0 -> 1.6.1 (changelog)

🙈

* @param bool $state
* @return $this
*/
public function evalScriptState($state = true) {

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 16, 2015

Contributor

allowEval

if($this->lastModified) {
$mergeWith['Last-Modified'] =
$this->lastModified->format(\DateTime::RFC2822);
}
// Build Content-Security-Policy and use default if none has been specified
if(is_null($this->contentSecurityPolicy)) {

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 16, 2015

Contributor

This can be moved right up and you can get rid of the if with

private $contentSecurityPolicy = new ContentSecurityPolicy();

This comment has been minimized.

@LukasReschke

LukasReschke Feb 16, 2015

Member

I don't think one can set expressions as default field value:

Parse error: syntax error, unexpected 'new' (T_NEW) in /Users/lreschke/Programming/core/lib/public/appframework/http/response.php on line 76

This comment has been minimized.

@BernhardPosselt

BernhardPosselt Feb 16, 2015

Contributor

Hm, shitty php

@BernhardPosselt

This comment has been minimized.

Contributor

BernhardPosselt commented Feb 16, 2015

👍

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Feb 16, 2015

The inspection completed: 10 new issues, 44 updated code elements

@owncloud-bot

This comment has been minimized.

Contributor

owncloud-bot commented Feb 16, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9485/
Test PASSed.

@th3fallen

This comment has been minimized.

Contributor

th3fallen commented Feb 18, 2015

works here 👍

th3fallen added a commit that referenced this pull request Feb 18, 2015

Merge pull request #13989 from owncloud/enhancment/security/11857
Allow AppFramework applications to specify a custom CSP header

@th3fallen th3fallen merged commit 8d09cc3 into master Feb 18, 2015

1 check passed

default Merged build finished.
Details

@LukasReschke LukasReschke deleted the enhancment/security/11857 branch Feb 18, 2015

@LukasReschke LukasReschke restored the enhancment/security/11857 branch Feb 25, 2015

@LukasReschke LukasReschke deleted the enhancment/security/11857 branch Feb 25, 2015

@MorrisJobke MorrisJobke added this to the 8.1-current milestone Mar 16, 2015

@libasys

This comment has been minimized.

Contributor

libasys commented Apr 20, 2015

This is a nice feature, but i have a problem with "$allowedImageDomains", this checks the image source, but what happens if i work directly with the data like:

data:image/jpg;base64,/9j/4AA  ....

so the browser not load the image source but shows this error:
... because it violates the following Content Security Policy directive: "img-src 'self'"

so i have to set allowedImageDomains = '*' or is there are a other mechanism?

Thanks

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Apr 20, 2015

@libasys data: should help. – Untested.

@libasys

This comment has been minimized.

Contributor

libasys commented Apr 20, 2015

I tested this and added following to the default behaviour of $allowedImageDomains on lib/public/appframework/http/contentsecuritypolicy.php

/** @var array Domains from which images can get loaded */
    private $allowedImageDomains = [
        '\'self\' data:',
    ];

This should be a default behaviour of $allowedImageDomains! Now it works as expected!

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Apr 20, 2015

This should be a default behaviour of $allowedImageDomains! Now it works as expected!

I'd like to prevent this. There are at the moment not much apps that need this behaviour. You can do this on single controllers like that:

$csp = new OCP\AppFramework\Http\ContentSecurityPolicy();
$csp->addAllowedImageDomain('data:');
$response = new TemplateResponse($this->appName, 'public', $shareTmpl, 'base');
$response->setContentSecurityPolicy($csp);
return $response;
@libasys

This comment has been minimized.

Contributor

libasys commented Apr 20, 2015

perfect! It works! :) Thanks! Should become part of the devs manual on owncloud.org with an example.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Apr 20, 2015

Yup. – I still need to do it for 8.1.

Ref owncloud/documentation#876

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment