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

Let users configure security headers in their Webserver #13455

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@LukasReschke
Member

LukasReschke commented Jan 19, 2015

Doing this in the PHP code is not the right approach for multiple reasons:

  1. A bug in the PHP code prevents them from being added to the response.
  2. They are only added when something is served via PHP and not in other cases (that makes for example the newest IE UXSS which is not yet patched by Microsoft exploitable on ownCloud)
  3. Some headers such as the Strict-Transport-Security might require custom modifications by administrators. This was not possible before and lead to buggy situations.

This pull request moves those headers out of the PHP code and adds a security check to the admin settings performed via JS. Sane defaults are also provided in our .htaccess


# Testplan: - [ ] When mod_headers is enabled the headers are added accordingly and no security warning is shown (maybe except SSL) - [ ] When removing some of those headers warnings are shown - [ ] When using SSL no SSL warning except "Strict-Transport-Security" is shown - [ ] When using SSL in combination with "Strict-Transport-Security" no warning is shown
# TODO: - [x] Remove `X-Frame-Options` from the code and move to `.htaccess` - [x] Add check to admin panel. - [x] Remove `X-XSS-Protection` from the code and move to `.htaccess` - [x] Add check to admin panel. - [x] Remove `X-Content-Type-Options` from the code and move to `.htaccess` - [x] Add check to admin panel. - [x] Remove `X-Robots-Tag` from the code and move to `.htaccess` - [x] Add check to admin panel. - [x] Remove `forceSSL` from the code - [x] Add check to admin panel. - [x] Autodetect SSL situations and set cookies accordingly - [ ] Document changes and add a breaking change text on https://github.com/owncloud/core/wiki/ownCloud-8.1-Features
# Breaking changes - Admins now have to ensure that the security headers are properly configured in the webserver config - The `forcessl` and `xframe_restriction` config are gone and have to be replaced by custom HTTP headers by the admin

Fixes #14073

@MorrisJobke

This comment has been minimized.

Member

MorrisJobke commented Jan 28, 2015

👍

@MorrisJobke

This comment has been minimized.

Member

MorrisJobke commented Jan 28, 2015

@DeepDiver1975 Something for 8.0?

@DeepDiver1975

This comment has been minimized.

Member

DeepDiver1975 commented Jan 28, 2015

@DeepDiver1975 Something for 8.0?

no showstopper - no critical fix -> 8.1 if @LukasReschke agrees

@DeepDiver1975 DeepDiver1975 added this to the 8.1-next milestone Jan 28, 2015

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Jan 28, 2015

8.1 is fine. That's why I didn't add it to a milestone yet. This has potential to break stuff.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 12, 2015

Will add stuff for #14073 here as well

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 12, 2015

Further work requires #14056 to be merged to be able to have testable code for my planned changes.

@LukasReschke LukasReschke changed the title from Add some headers to .htaccess as well to Let users configure security headers in their Webserver Feb 23, 2015

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 23, 2015

This is ready to review now.

cc @PVince81 For the JS part
cc @bantu @nickvergessen Please review.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 23, 2015

cc @th3fallen Care to test and review as well? :)

OC_Response::addSecurityHeaders();
if(self::$server->getRequest()->getServerProtocol() === 'https') {

This comment has been minimized.

@LukasReschke

LukasReschke Feb 23, 2015

Member

Remark: Verify if that works with proxies if they send the correct protocol in the headers.

This comment has been minimized.

@LukasReschke

LukasReschke Feb 23, 2015

Member

Verified: works if overwriteprotocol is defined in the config or HTTP_X_FORWARDED_PROTO is sent

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 23, 2015

Remark: This change has the potential that users fuckup their configuration and thus have insecure headers set.

But then again, we set those per default in our .htaccess and the admin menu now shows a big security warning if one is missing. In my opinion this comes back to: "Users should either read the documentation or use the packages by the vendor which are likely to be configured properly".

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 23, 2015

(this is also the place where we show the "Your data directory is publicly accessible" warning)

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Feb 23, 2015

As discussed with @PVince81 I'll take a look how we can unit tests the JS part.

@MorrisJobke

This comment has been minimized.

Member

MorrisJobke commented Feb 26, 2015

Rebase required ;)

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Mar 2, 2015

@PVince81 Thanks for the help. Was finally able to write unit tests for this 😄

@PVince81

View changes

core/js/js.js Outdated
@@ -211,6 +211,14 @@ var OC={
},
/**
* Protocol that is used to access this ownCloud instance with trailing :
* @return string Used protocol

This comment has been minimized.

@PVince81

PVince81 Mar 2, 2015

Member

=> {string}

You can build the JS Docs with "./buildjsdocs.sh" and check "build/jsdoc*index.html" to see how it would look like

This comment has been minimized.

@LukasReschke

LukasReschke Mar 2, 2015

Member

Nice to know :)

$.ajax({
type: 'GET',
url: OC.generateUrl('heartbeat')

This comment has been minimized.

@PVince81

PVince81 Mar 2, 2015

Member

Would it be possible to check this with the other one above from the same request ?

This comment has been minimized.

@LukasReschke

LukasReschke Mar 2, 2015

Member

I'm not sure. I understand that it generally is a very important thing to keep the amount of additional requests as low as possible. However, I do want to keep these checks in separate functions for clarity. As I'm not the JS 👑 here it would be cool if you could give me a hint if you think of a better alternative :)

This comment has been minimized.

@PVince81

PVince81 Mar 2, 2015

Member

You could put the call in a single method "checkStuff()" that calls heartbeat. Then in "afterCall" call every "this._checkXXX(xhr)" method that you want to run on the xhr object. The "this._checkXXX(xhr)" method would return a list of messages to be appended and given to resolve(). 😄

This comment has been minimized.

@LukasReschke

LukasReschke Mar 2, 2015

Member

Adjusted. Please take a look 🚀

@MorrisJobke

View changes

core/js/core.json Outdated
@@ -24,6 +24,7 @@
"config.js",
"multiselect.js",
"oc-requesttoken.js",
"setupchecks.js",

This comment has been minimized.

@MorrisJobke

MorrisJobke Mar 2, 2015

Member

intendation

This comment has been minimized.

@LukasReschke

LukasReschke Mar 2, 2015

Member

🙈 – so true…

@MorrisJobke

View changes

lib/private/appframework/app.php Outdated
$container->getServer()->getConfig()->getSystemValue('forcessl', false),
true
$container->getServer()->getRequest()->getServerProtocol() === 'https',
true

This comment has been minimized.

@MorrisJobke

MorrisJobke Mar 2, 2015

Member

Space added?

Let users configure security headers in their Webserver
Doing this in the PHP code is not the right approach for multiple reasons:

1. A bug in the PHP code prevents them from being added to the response.
2. They are only added when something is served via PHP and not in other cases (that makes for example the newest IE UXSS which is not yet patched by Microsoft exploitable on ownCloud)
3. Some headers such as the Strict-Transport-Security might require custom modifications by administrators. This was not possible before and lead to buggy situations.

This pull request moves those headers out of the PHP code and adds a security check to the admin settings performed via JS.
@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Mar 2, 2015

@owncloud-bot Retest this please.

1 similar comment
@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Mar 2, 2015

@owncloud-bot Retest this please.

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Mar 2, 2015

Gotta love that bot… Then let's create a jenkins branch…

@LukasReschke

This comment has been minimized.

Member

LukasReschke commented Mar 2, 2015

Jenkins PR will be at #14651

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 2, 2015

The inspection completed: 2 new issues

@PVince81

This comment has been minimized.

Member

PVince81 commented Mar 3, 2015

I committed to the Jenkins PR by mistake, but now tests pass there.
So I suggest we just continue there: #14651

@MorrisJobke MorrisJobke closed this Mar 4, 2015

@MorrisJobke MorrisJobke deleted the add-some-headers-to-htaccess branch Mar 4, 2015

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