mbk-lab
/
nextcloud
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add Same Site Cookie protection

This commit is contained in:
Lukas Reschke 2016-06-10 17:15:43 +02:00
parent 73a9afed6d
commit 2c6a5fcf91
No known key found for this signature in database
GPG Key ID: 9AB0ADB949B6898C
11 changed files with 524 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
apps/files/controller/apicontroller.php
View File

@ -80,6 +80,7 @@ class ApiController extends Controller {
*
* @NoAdminRequired
* @NoCSRFRequired
* @StrictCookieRequired
*
* @param int $x
* @param int $y

66
lib/base.php
View File

@ -480,6 +480,70 @@ class OC {
@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));
@ -589,6 +653,8 @@ class OC {
ini_set('session.cookie_secure', true);
}
self::performSameSiteCookieProtection();
if (!defined('OC_CONSOLE')) {
$errors = OC_Util::checkServer(\OC::$server->getConfig());
if (count($errors) > 0) {

33
lib/private/appframework/http/request.php
View File

@ -448,6 +448,10 @@ class Request implements \ArrayAccess, \Countable, IRequest {
return false;
}
if(!$this->passesStrictCookieCheck()) {
return false;
}
if (isset($this->items['get']['requesttoken'])) {
$token = $this->items['get']['requesttoken'];
} elseif (isset($this->items['post']['requesttoken'])) {
@ -463,6 +467,35 @@ class Request implements \ArrayAccess, \Countable, IRequest {
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.

36
lib/private/appframework/middleware/security/exceptions/strictcookiemissingexception.php Normal file
View File

@ -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);
}
}

11
lib/private/appframework/middleware/security/securitymiddleware.php
View File

@ -30,6 +30,7 @@ use OC\Appframework\Middleware\Security\Exceptions\AppNotEnabledException;
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;
@ -132,6 +133,13 @@ class SecurityMiddleware extends Middleware {
}
}
// 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')) {
@ -184,6 +192,9 @@ class SecurityMiddleware extends Middleware {
*/
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(

4
lib/private/eventsource.php
View File

@ -76,6 +76,10 @@ class OC_EventSource implements \OCP\IEventSource {
} 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();

5
lib/private/json.php
View File

@ -77,6 +77,11 @@ class OC_JSON{
* @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' )));

16
lib/public/irequest.php
View File

@ -143,6 +143,22 @@ interface IRequest {
*/
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.

5
lib/public/util.php
View File

@ -504,6 +504,11 @@ class Util {
* @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();
}

286
tests/lib/appframework/http/RequestTest.php
View File

@ -1,15 +1,16 @@
<?php
/**
* @copyright 2013 Thomas Tanghus (thomas@tanghus.net)
* @copyright 2015 Lukas Reschke lukas@owncloud.com
* @copyright 2016 Lukas Reschke lukas@owncloud.com
*
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/
namespace OC\AppFramework\Http;
namespace Test\AppFramework\Http;
use OC\AppFramework\Http\Request;
use OC\Security\CSRF\CsrfToken;
use OC\Security\CSRF\CsrfTokenManager;
use OCP\Security\ISecureRandom;
@ -740,15 +741,15 @@ class RequestTest extends \Test\TestCase {
*/
public function testUserAgent($testAgent, $userAgent, $matches) {
$request = new Request(
[
'server' => [
'HTTP_USER_AGENT' => $testAgent,
]
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
[
'server' => [
'HTTP_USER_AGENT' => $testAgent,
]
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
);
$this->assertSame($matches, $request->isUserAgent($userAgent));
@ -762,11 +763,11 @@ class RequestTest extends \Test\TestCase {
*/
public function testUndefinedUserAgent($testAgent, $userAgent, $matches) {
$request = new Request(
[],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
[],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
);
$this->assertFalse($request->isUserAgent($userAgent));
@ -1322,6 +1323,10 @@ class RequestTest extends \Test\TestCase {
'get' => [
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
@ -1348,6 +1353,10 @@ class RequestTest extends \Test\TestCase {
'post' => [
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
@ -1357,15 +1366,91 @@ class RequestTest extends \Test\TestCase {
->getMock();
$token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds');
$this->csrfTokenManager
->expects($this->once())
->method('isTokenValid')
->with($token)
->willReturn(true);
->expects($this->once())
->method('isTokenValid')
->with($token)
->willReturn(true);
$this->assertTrue($request->passesCSRFCheck());
}
public function testPassesCSRFCheckWithHeader() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds');
$this->csrfTokenManager
->expects($this->once())
->method('isTokenValid')
->with($token)
->willReturn(true);
$this->assertTrue($request->passesCSRFCheck());
}
public function testFailsCSRFCheckWithGetAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'get' => [
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
$this->assertFalse($request->passesCSRFCheck());
}
public function testFailsCSRFCheckWithPostAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'post' => [
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
$this->assertFalse($request->passesCSRFCheck());
}
public function testFailsCSRFCheckWithHeaderAndWithoutCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
@ -1381,14 +1466,153 @@ class RequestTest extends \Test\TestCase {
$this->stream
])
->getMock();
$token = new CsrfToken('AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds');
$this->csrfTokenManager
->expects($this->once())
->method('isTokenValid')
->with($token)
->willReturn(true);
->expects($this->never())
->method('isTokenValid');
$this->assertTrue($request->passesCSRFCheck());
$this->assertFalse($request->passesCSRFCheck());
}
public function testFailsCSRFCheckWithHeaderAndNotAllChecksPassing() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->csrfTokenManager
->expects($this->never())
->method('isTokenValid');
$this->assertFalse($request->passesCSRFCheck());
}
public function testPassesStrictCookieCheckWithAllCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertTrue($request->passesStrictCookieCheck());
}
public function testFailStrictCookieCheckWithOnlyLaxCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertFalse($request->passesStrictCookieCheck());
}
public function testFailStrictCookieCheckWithOnlyStrictCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertFalse($request->passesStrictCookieCheck());
}
public function testPassesLaxCookieCheck() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookielax' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertTrue($request->passesLaxCookieCheck());
}
public function testFailsLaxCookieCheckWithOnlyStrictCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'nc_sameSiteCookiestrict' => 'true',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertFalse($request->passesLaxCookieCheck());
}
/**
@ -1426,10 +1650,10 @@ class RequestTest extends \Test\TestCase {
$token = new CsrfToken($invalidToken);
$this->csrfTokenManager
->expects($this->any())
->method('isTokenValid')
->with($token)
->willReturn(false);
->expects($this->any())
->method('isTokenValid')
->with($token)
->willReturn(false);
$this->assertFalse($request->passesCSRFCheck());
}
@ -1450,4 +1674,4 @@ class RequestTest extends \Test\TestCase {
$this->assertFalse($request->passesCSRFCheck());
}
}
}

95
tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php
View File

@ -31,6 +31,7 @@ use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryExcept
use OC\Appframework\Middleware\Security\Exceptions\NotAdminException;
use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException;
use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
use OC\AppFramework\Utility\ControllerMethodReflector;
use OC\Security\CSP\ContentSecurityPolicy;
use OCP\AppFramework\Http\RedirectResponse;
@ -255,7 +256,9 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->request->expects($this->once())
->method('passesCSRFCheck')
->will($this->returnValue(false));
$this->request->expects($this->once())
->method('passesStrictCookieCheck')
->will($this->returnValue(true));
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
@ -274,19 +277,81 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @PublicPage
*/
public function testFailCsrfCheck(){
public function testPassesCsrfCheck(){
$this->request->expects($this->once())
->method('passesCSRFCheck')
->will($this->returnValue(true));
$this->request->expects($this->once())
->method('passesStrictCookieCheck')
->will($this->returnValue(true));
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @PublicPage
* @expectedException \OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException
*/
public function testFailCsrfCheck(){
$this->request->expects($this->once())
->method('passesCSRFCheck')
->will($this->returnValue(false));
$this->request->expects($this->once())
->method('passesStrictCookieCheck')
->will($this->returnValue(true));
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @PublicPage
* @StrictCookieRequired
* @expectedException \OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException
*/
public function testStrictCookieRequiredCheck() {
$this->request->expects($this->never())
->method('passesCSRFCheck');
$this->request->expects($this->once())
->method('passesStrictCookieCheck')
->will($this->returnValue(false));
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @PublicPage
* @NoCSRFRequired
*/
public function testNoStrictCookieRequiredCheck() {
$this->request->expects($this->never())
->method('passesStrictCookieCheck')
->will($this->returnValue(false));
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @PublicPage
* @NoCSRFRequired
* @StrictCookieRequired
*/
public function testPassesStrictCookieRequiredCheck() {
$this->request
->expects($this->once())
->method('passesStrictCookieCheck')
->willReturn(true);
$this->reader->reflect(__CLASS__, __FUNCTION__);
$this->middleware->beforeController(__CLASS__, __FUNCTION__);
}
/**
* @NoCSRFRequired
@ -360,6 +425,30 @@ class SecurityMiddlewareTest extends \Test\TestCase {
$this->assertEquals($expected , $response);
}
public function testAfterExceptionRedirectsToWebRootAfterStrictCookieFail() {
$this->request = new Request(
[
'server' => [
'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
'REQUEST_URI' => 'owncloud/index.php/apps/specialapp',
],
],
$this->getMock('\OCP\Security\ISecureRandom'),
$this->getMock('\OCP\IConfig')
);
$this->middleware = $this->getMiddleware(false, false);
$response = $this->middleware->afterException(
$this->controller,
'test',
new StrictCookieMissingException()
);
$expected = new RedirectResponse(\OC::$WEBROOT);
$this->assertEquals($expected , $response);
}
/**
* @return array
*/