Skip to content

Commit

Permalink
Add Same Site Cookie protection
Browse files Browse the repository at this point in the history
  • Loading branch information
LukasReschke committed Jun 10, 2016
1 parent 73a9afe commit 2c6a5fc
Show file tree
Hide file tree
Showing 11 changed files with 520 additions and 30 deletions.
1 change: 1 addition & 0 deletions apps/files/controller/apicontroller.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public function __construct($appName,
*
* @NoAdminRequired
* @NoCSRFRequired
* @StrictCookieRequired
*
* @param int $x
* @param int $y
Expand Down
66 changes: 66 additions & 0 deletions lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,70 @@ public static function setRequiredIniValues() {
@ini_set('gd.jpeg_ignore_warning', 1);
}

/**
* Send the same site cookies
*/
private static function sendSameSiteCookies() {
$cookieParams = session_get_cookie_params();
$secureCookie = ($cookieParams['secure'] === true) ? 'secure; ' : '';
$policies = [
'lax',
'strict',
];
foreach($policies as $policy) {
header(
sprintf(
'Set-Cookie: nc_sameSiteCookie%s=true; path=%s; httponly;' . $secureCookie . 'expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=%s',
$policy,
$cookieParams['path'],
$policy
),
false
);
}
}

/**
* Same Site cookie to further mitigate CSRF attacks. This cookie has to
* be set in every request if cookies are sent to add a second level of
* defense against CSRF.
*
* If the cookie is not sent this will set the cookie and reload the page.
* We use an additional cookie since we want to protect logout CSRF and
* also we can't directly interfere with PHP's session mechanism.
*/
private static function performSameSiteCookieProtection() {
if(count($_COOKIE) > 0) {
$request = \OC::$server->getRequest();
$requestUri = $request->getScriptName();
$processingScript = explode('/', $requestUri);
$processingScript = $processingScript[count($processingScript)-1];

// For the "index.php" endpoint only a lax cookie is required.
if($processingScript === 'index.php') {
if(!$request->passesLaxCookieCheck()) {
self::sendSameSiteCookies();
header('Location: '.$_SERVER['REQUEST_URI']);
exit();
}
} else {
// All other endpoints require the lax and the strict cookie
if(!$request->passesStrictCookieCheck()) {
self::sendSameSiteCookies();
// Debug mode gets access to the resources without strict cookie
// due to the fact that the SabreDAV browser also lives there.
if(!\OC::$server->getConfig()->getSystemValue('debug', false)) {
http_response_code(\OCP\AppFramework\Http::STATUS_SERVICE_UNAVAILABLE);
exit();
}
}
}
} elseif(!isset($_COOKIE['nc_sameSiteCookielax']) || !isset($_COOKIE['nc_sameSiteCookiestrict'])) {
self::sendSameSiteCookies();
}
}


public static function init() {
// calculate the root directories
OC::$SERVERROOT = str_replace("\\", '/', substr(__DIR__, 0, -4));
Expand Down Expand Up @@ -589,6 +653,8 @@ public static function init() {
ini_set('session.cookie_secure', true);
}

self::performSameSiteCookieProtection();

if (!defined('OC_CONSOLE')) {
$errors = OC_Util::checkServer(\OC::$server->getConfig());
if (count($errors) > 0) {
Expand Down
33 changes: 33 additions & 0 deletions lib/private/appframework/http/request.php
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,10 @@ public function passesCSRFCheck() {
return false;
}

if(!$this->passesStrictCookieCheck()) {
return false;
}

if (isset($this->items['get']['requesttoken'])) {
$token = $this->items['get']['requesttoken'];
} elseif (isset($this->items['post']['requesttoken'])) {
Expand All @@ -463,6 +467,35 @@ public function passesCSRFCheck() {
return $this->csrfTokenManager->isTokenValid($token);
}

/**
* Checks if the strict cookie has been sent with the request
*
* @return bool
* @since 9.1.0
*/
public function passesStrictCookieCheck() {
if($this->getCookie('nc_sameSiteCookiestrict') === 'true'
&& $this->passesLaxCookieCheck()) {
return true;
}

return false;
}

/**
* Checks if the lax cookie has been sent with the request
*
* @return bool
* @since 9.1.0
*/
public function passesLaxCookieCheck() {
if($this->getCookie('nc_sameSiteCookielax') === 'true') {
return true;
}

return false;
}

/**
* Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
* If `mod_unique_id` is installed this value will be taken.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php
/**
* @author Lukas Reschke <lukas@statuscode.ch>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OC\Appframework\Middleware\Security\Exceptions;

use OCP\AppFramework\Http;

/**
* Class StrictCookieMissingException is thrown when the strict cookie has not
* been sent with the request but is required.
*
* @package OC\Appframework\Middleware\Security\Exceptions
*/
class StrictCookieMissingException extends SecurityException {
public function __construct() {
parent::__construct('Strict Cookie has not been found in request.', Http::STATUS_PRECONDITION_FAILED);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException;
use OC\Appframework\Middleware\Security\Exceptions\NotAdminException;
use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException;
use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
use OC\AppFramework\Utility\ControllerMethodReflector;
use OC\Security\CSP\ContentSecurityPolicyManager;
use OCP\AppFramework\Http\ContentSecurityPolicy;
Expand Down Expand Up @@ -132,6 +133,13 @@ public function beforeController($controller, $methodName) {
}
}

// Check for strict cookie requirement
if($this->reflector->hasAnnotation('StrictCookieRequired') || !$this->reflector->hasAnnotation('NoCSRFRequired')) {
if(!$this->request->passesStrictCookieCheck()) {
throw new StrictCookieMissingException();
}
}

// CSRF check - also registers the CSRF token since the session may be closed later
Util::callRegister();
if(!$this->reflector->hasAnnotation('NoCSRFRequired')) {
Expand Down Expand Up @@ -184,6 +192,9 @@ public function afterController($controller, $methodName, Response $response) {
*/
public function afterException($controller, $methodName, \Exception $exception) {
if($exception instanceof SecurityException) {
if($exception instanceof StrictCookieMissingException) {
return new RedirectResponse(\OC::$WEBROOT);
}

if (stripos($this->request->getHeader('Accept'),'html') === false) {
$response = new JSONResponse(
Expand Down
4 changes: 4 additions & 0 deletions lib/private/eventsource.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ protected function init() {
} else {
header("Content-Type: text/event-stream");
}
if(!\OC::$server->getRequest()->passesStrictCookieCheck()) {
header('Location: '.\OC::$WEBROOT);
exit();
}
if (!(\OC::$server->getRequest()->passesCSRFCheck())) {
$this->send('error', 'Possible CSRF attack. Connection will be closed.');
$this->close();
Expand Down
5 changes: 5 additions & 0 deletions lib/private/json.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public static function checkLoggedIn() {
* @deprecated Use annotation based CSRF checks from the AppFramework instead
*/
public static function callCheck() {
if(!\OC::$server->getRequest()->passesStrictCookieCheck()) {
header('Location: '.\OC::$WEBROOT);
exit();
}

if( !(\OC::$server->getRequest()->passesCSRFCheck())) {
$l = \OC::$server->getL10N('lib');
self::error(array( 'data' => array( 'message' => $l->t('Token expired. Please reload page.'), 'error' => 'token_expired' )));
Expand Down
16 changes: 16 additions & 0 deletions lib/public/irequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,22 @@ public function getCookie($key);
*/
public function passesCSRFCheck();

/**
* Checks if the strict cookie has been sent with the request
*
* @return bool
* @since 9.0.0
*/
public function passesStrictCookieCheck();

/**
* Checks if the lax cookie has been sent with the request
*
* @return bool
* @since 9.1.0
*/
public function passesLaxCookieCheck();

/**
* Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
* If `mod_unique_id` is installed this value will be taken.
Expand Down
5 changes: 5 additions & 0 deletions lib/public/util.php
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,11 @@ public static function callRegister() {
* @deprecated 9.0.0 Use annotations based on the app framework.
*/
public static function callCheck() {
if(!\OC::$server->getRequest()->passesStrictCookieCheck()) {
header('Location: '.\OC::$WEBROOT);
exit();
}

if (!(\OC::$server->getRequest()->passesCSRFCheck())) {
exit();
}
Expand Down
Loading

0 comments on commit 2c6a5fc

Please sign in to comment.