tonyair
/
silverstripe-framework
mirror of https://github.com/silverstripe/silverstripe-framework
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

ENH Add samesite attribute to cookies. 

Co-authored-by: pine3ree <pine3ree@gmail.com>
This commit is contained in:
Guy Sartorelli 2022-05-26 17:09:11 +12:00
parent 6f27dadae8
commit 31c974c528
10 changed files with 506 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md
View File

@ -762,11 +762,15 @@ disable this behaviour using `CanonicalURLMiddleware::singleton()->setForceBasic
configuration in YAML.
We also want to ensure cookies are not shared between secure and non-secure sessions, so we must tell Silverstripe CMS to
use a [secure session](https://docs.silverstripe.org/en/3/developer_guides/cookies_and_sessions/sessions/#secure-session-cookie).
To do this, you may set the `cookie_secure` parameter to `true` in your `config.yml` for `Session`
use a [secure session](/developer_guides/cookies_and_sessions/sessions/#secure-session-cookie).
To do this, you may set the `cookie_secure` parameter to `true` in your `config.yml` for `Session`.
It is also a good idea to set the `samesite` attribute for the session cookie to `Strict` unless you have a specific use case for
sharing the session cookie across domains.
```yml
SilverStripe\Control\Session:
cookie_samesite: 'Strict'
cookie_secure: true
```
@ -784,6 +788,11 @@ SilverStripe\Core\Injector\Injector:
TokenCookieSecure: true
```
[info]
There is not currently an easy way to pass a `samesite` attribute value for setting this cookie - but you can set the
default value for the attribute for all cookies. See [the main cookies documentation](/developer_guides/cookies_and_sessions/cookies#samesite-attribute) for more information.
[/info]
For other cookies set by your application we should also ensure the users are provided with secure cookies by setting
the "Secure" and "HTTPOnly" flags. These flags prevent them from being stolen by an attacker through javascript.

18
docs/en/02_Developer_Guides/18_Cookies_And_Sessions/01_Cookies.md
View File

@ -5,6 +5,10 @@ icon: cookie-bite
---
# Cookies
Note that cookies can have security implications - before setting your own cookies, make sure to read through the
[secure coding](/developer_guides/security/secure_coding#secure-sessions-cookies-and-tls-https) documentation.
## Accessing and Manipulating Cookies
Cookies are a mechanism for storing data in the remote browser and thus tracking or identifying return users.
@ -52,6 +56,20 @@ Cookie::force_expiry($name, $path = null, $domain = null);
// Cookie::force_expiry('MyApplicationPreference')
```
### Samesite attribute
The `samesite` attribute is set on all cookies with a default value of `Lax`. You can change the default value by setting the `default_samesite` value on the
[Cookie](api:SilverStripe\Control\Cookie) class:
```yml
SilverStripe\Control\Cookie:
default_samesite: 'Strict'
```
[info]
Note that this _doesn't_ apply for the session cookie, which is handled separately. See [Sessions](/developer_guides/cookies_and_sessions/sessions#samesite-attribute).
[/info]
## Cookie_Backend
The [Cookie](api:SilverStripe\Control\Cookie) class manipulates and sets cookies using a [Cookie_Backend](api:SilverStripe\Control\Cookie_Backend). The backend is in charge of the logic

16
docs/en/02_Developer_Guides/18_Cookies_And_Sessions/02_Sessions.md
View File

@ -101,7 +101,19 @@ including form and page comment information. None of this is vital but `clear_al
$session->clearAll();
```
## Secure Session Cookie
## Cookies
### Samesite attribute
The session cookie is handled slightly differently than most cookies on the site, which provides the opportunity to handle the samesite attribute separately from other cookies.
You can change the `samesite` attribute for session cookies like so:
```yml
SilverStripe\Control\Session:
cookie_samesite: 'Strict'
```
### Secure Session Cookie
In certain circumstances, you may want to use a different `session_name` cookie when using the `https` protocol for security purposes. To do this, you may set the `cookie_secure` parameter to `true` on your `config.yml`
@ -113,6 +125,8 @@ SilverStripe\Control\Session:
This uses the session_name `SECSESSID` for `https` connections instead of the default `PHPSESSID`. Doing so adds an extra layer of security to your session cookie since you no longer share `http` and `https` sessions.
Note that if you set `cookie_samesite` to `None` (which is _strongly_ discouraged), the `cookie_secure` value will _always_ be `true`.
## Relaxing checks around user agent strings
Out of the box, Silverstripe CMS will invalidate a user's session if the `User-Agent` header changes. This provides some supplemental protection against session high-jacking attacks.

55
docs/en/04_Changelogs/4.12.0.md Normal file
View File

@ -0,0 +1,55 @@
---
title: 4.12.0 (unreleased)
---
# 4.12.0 (unreleased)
## Overview
- [Regression test and Security audit](#audit)
- [Features and enhancements](#features-and-enhancements)
- [Samesite attribute on cookies](#cookies-samesite)
- [Other features](#other-features)
- [Bugfixes](#bugfixes)
## Regression test and Security audit{#audit}
This release has been comprehensively regression tested and passed to a third party for a security-focused audit.
While it is still advised that you perform your own due diligence when upgrading your project, this work is performed to ensure a safe and secure upgrade with each recipe release.
## Features and enhancements {#features-and-enhancements}
### Samesite attribute on cookies {#cookies-samesite}
The `samesite` attribute is now set on all cookies. To avoid backward compatability issues, the `Lax` value has been set by default, but we recommend reviewing the requirements of your project and setting an appropriate value.
The default value can be set for all cookies (except for the session cookie) in yaml configuration like so:
```yml
SilverStripe\Control\Cookie:
default_samesite: 'Strict'
```
Check out the [cookies documentation](/developer_guides/cookies_and_sessions/cookies#samesite-attribute) for more information.
The session cookie is handled separately. You can configure it like so:
```yml
SilverStripe\Control\Session:
cookie_samesite: 'Strict'
```
Note that if you set the `samesite` attribute to `None`, the `secure` is automatically set to `true` as required by the specification.
For more information about the `samesite` attribute check out https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
### Other new features {#other-features}
## Bugfixes {#bugfixes}
This release includes a number of bug fixes to improve a broad range of areas. Check the change logs for full details of these fixes split by module. Thank you to the community members that helped contribute these fixes as part of the release!
<!--- Changes below this line will be automatically regenerated -->
<!--- Changes above this line will be automatically regenerated -->

42
src/Control/Cookie.php
View File

@ -2,6 +2,8 @@
namespace SilverStripe\Control;
use LogicException;
use Psr\Log\LoggerInterface;
use SilverStripe\Core\Config\Configurable;
use SilverStripe\Core\Injector\Injector;
@ -19,6 +21,12 @@ class Cookie
*/
private static $report_errors = true;
/**
* Must be "Strict", "Lax", or "None"
* @config
*/
private static string $default_samesite = 'Lax';
/**
* Fetch the current instance of the cookie backend.
*
@ -92,4 +100,38 @@ class Cookie
{
return self::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly);
}
/**
* Validate if the samesite value for a cookie is valid for the current request.
*
* Logs a warning if the samesite value is "None" for a non-https request.
* @throws LogicException if the value is not "Strict", "Lax", or "None".
*/
public static function validateSameSite(string $sameSite): void
{
$validValues = [
'Strict',
'Lax',
'None',
];
if (!in_array($sameSite, $validValues)) {
throw new LogicException('Cookie samesite must be "Strict", "Lax", or "None"');
}
if ($sameSite === 'None' && !Director::is_https(self::getRequest())) {
Injector::inst()->get(LoggerInterface::class)->warning('Cookie samesite cannot be "None" for non-https requests.');
}
}
/**
* Get the current request, if any.
*/
private static function getRequest(): ?HTTPRequest
{
$request = null;
if (Controller::has_curr()) {
$request = Controller::curr()->getRequest();
}
// NullHTTPRequest always has a scheme of http - set to null so we can fallback on default_base_url
return ($request instanceof NullHTTPRequest) ? null : $request;
}
}

33
src/Control/CookieJar.php
View File

@ -18,7 +18,6 @@ use LogicException;
*/
class CookieJar implements Cookie_Backend
{
/**
* Hold the cookies that were existing at time of instantiation (ie: The ones
* sent to PHP by the browser)
@ -168,9 +167,18 @@ class CookieJar implements Cookie_Backend
$secure = false,
$httpOnly = true
) {
$sameSite = $this->getSameSite($name);
Cookie::validateSameSite($sameSite);
// if headers aren't sent, we can set the cookie
if (!headers_sent($file, $line)) {
return setcookie($name ?? '', $value ?? '', $expiry ?? 0, $path ?? '', $domain ?? '', $secure ?? false, $httpOnly ?? false);
return setcookie($name ?? '', $value ?? '', [
'expires' => $expiry ?? 0,
'path' => $path ?? '',
'domain' => $domain ?? '',
'secure' => $this->cookieIsSecure($sameSite, (bool) $secure),
'httponly' => $httpOnly ?? false,
'samesite' => $sameSite,
]);
}
if (Cookie::config()->uninherited('report_errors')) {
@ -180,4 +188,25 @@ class CookieJar implements Cookie_Backend
}
return false;
}
/**
* Cookies must be secure if samesite is "None"
*/
private function cookieIsSecure(string $sameSite, bool $secure): bool
{
return $sameSite === 'None' ? true : $secure;
}
/**
* Get the correct samesite value - Session cookies use a different configuration variable.
*
* @deprecated 5.0 The relevant methods will include a `$sameSite` parameter instead.
*/
private function getSameSite(string $name): string
{
if ($name === session_name()) {
return Session::config()->get('cookie_samesite');
}
return Cookie::config()->get('default_samesite');
}
}

87
src/Control/Session.php
View File

@ -135,6 +135,12 @@ class Session
*/
private static $cookie_name_secure = 'SECSESSID';
/**
* Must be "Strict", "Lax", or "None".
* @config
*/
private static string $cookie_samesite = 'Lax';
/**
* Name of session cache limiter to use.
* Defaults to '' to disable cache limiter entirely.
@ -283,31 +289,15 @@ class Session
throw new BadMethodCallException("Session has already started");
}
$path = $this->config()->get('cookie_path');
if (!$path) {
$path = Director::baseURL();
}
$domain = $this->config()->get('cookie_domain');
$secure = Director::is_https($request) && $this->config()->get('cookie_secure');
$session_path = $this->config()->get('session_store_path');
$timeout = $this->config()->get('timeout');
// Director::baseURL can return absolute domain names - this extracts the relevant parts
// for the session otherwise we can get broken session cookies
if (Director::is_absolute_url($path)) {
$urlParts = parse_url($path ?? '');
$path = $urlParts['path'];
if (!$domain) {
$domain = $urlParts['host'];
}
}
// If the session cookie is already set, then the session can be read even if headers_sent() = true
// This helps with edge-case such as debugging.
$data = [];
if (!session_id() && (!headers_sent() || $this->requestContainsSessionId($request))) {
if (!headers_sent()) {
session_set_cookie_params($timeout ?: 0, $path, $domain ?: null, $secure, true);
$cookieParams = $this->buildCookieParams($request);
session_set_cookie_params($cookieParams);
$limiter = $this->config()->get('sessionCacheLimiter');
if (isset($limiter)) {
@ -323,7 +313,7 @@ class Session
// separate (less secure) session for non-HTTPS requests
// if headers_sent() is true then it's best to throw the resulting error rather than risk
// a security hole.
if ($secure) {
if ($cookieParams['secure']) {
session_name($this->config()->get('cookie_name_secure'));
}
@ -331,8 +321,16 @@ class Session
// Session start emits a cookie, but only if there's no existing session. If there is a session timeout
// tied to this request, make sure the session is held for the entire timeout by refreshing the cookie age.
if ($timeout && $this->requestContainsSessionId($request)) {
Cookie::set(session_name(), session_id(), $timeout / 86400, $path, $domain ?: null, $secure, true);
if ($cookieParams['lifetime'] && $this->requestContainsSessionId($request)) {
Cookie::set(
session_name(),
session_id(),
$cookieParams['lifetime'] / 86400,
$cookieParams['path'],
$cookieParams['domain'],
$cookieParams['secure'],
true
);
}
} else {
// If headers are sent then we can't have a session_cache_limiter otherwise we'll get a warning
@ -354,6 +352,53 @@ class Session
$this->started = true;
}
/**
* Build the parameters used for setting the session cookie.
*/
private function buildCookieParams(HTTPRequest $request): array
{
$timeout = $this->config()->get('timeout');
$path = $this->config()->get('cookie_path');
$domain = $this->config()->get('cookie_domain');
if (!$path) {
$path = Director::baseURL();
}
// Director::baseURL can return absolute domain names - this extracts the relevant parts
// for the session otherwise we can get broken session cookies
if (Director::is_absolute_url($path)) {
$urlParts = parse_url($path ?? '');
$path = $urlParts['path'];
if (!$domain) {
$domain = $urlParts['host'];
}
}
$sameSite = static::config()->get('cookie_samesite');
Cookie::validateSameSite($sameSite);
$secure = $this->isCookieSecure($sameSite, Director::is_https($request));
return [
'lifetime' => $timeout ?: 0,
'path' => $path,
'domain' => $domain ?: null,
'secure' => $secure,
'httponly' => true,
'samesite' => $sameSite,
];
}
/**
* Determines what the value for the `secure` cookie attribute should be.
*/
private function isCookieSecure(string $sameSite, bool $isHttps): bool
{
if ($sameSite === 'None') {
return true;
}
return $isHttps && $this->config()->get('cookie_secure');
}
/**
* Destroy this session
*

35
tests/php/Control/CookieJarTest.php
View File

@ -2,8 +2,11 @@
namespace SilverStripe\Control\Tests;
use ReflectionMethod;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Control\CookieJar;
use SilverStripe\Control\Session;
use SilverStripe\Core\Config\Config;
/**
* Testing CookieJar
@ -164,4 +167,36 @@ class CookieJarTest extends SapphireTest
$this->assertEmpty($cookieJar->get('newCookie'));
$this->assertEmpty($cookieJar->get('newCookie', false));
}
/**
* Check that the session cookie samesite configuration is used for session cookies.
*/
public function testGetSameSite(): void
{
$cookieJar = new CookieJar();
$methodGetSameSite = new ReflectionMethod($cookieJar, 'getSameSite');
$methodGetSameSite->setAccessible(true);
Config::modify()->set(Session::class, 'cookie_samesite', 'None');
$this->assertSame('None', $methodGetSameSite->invoke($cookieJar, session_name()));
$this->assertSame('Lax', $methodGetSameSite->invoke($cookieJar, 'some-random-cookie'));
}
/**
* Check that the cookies are correctly set as secure for samesite === "None"
* The passed in value for secure should be respected otherwise.
*/
public function testCookieIsSecure(): void
{
$cookieJar = new CookieJar();
$methodCookieIsSecure = new ReflectionMethod($cookieJar, 'cookieIsSecure');
$methodCookieIsSecure->setAccessible(true);
$this->assertTrue($methodCookieIsSecure->invoke($cookieJar, 'None', false));
$this->assertTrue($methodCookieIsSecure->invoke($cookieJar, 'None', true));
$this->assertTrue($methodCookieIsSecure->invoke($cookieJar, 'Lax', true));
$this->assertFalse($methodCookieIsSecure->invoke($cookieJar, 'Lax', false));
$this->assertTrue($methodCookieIsSecure->invoke($cookieJar, 'Strict', true));
$this->assertFalse($methodCookieIsSecure->invoke($cookieJar, 'Strict', false));
}
}

68
tests/php/Control/CookieTest.php
View File

@ -2,10 +2,15 @@
namespace SilverStripe\Control\Tests;
use Exception;
use LogicException;
use Monolog\Logger;
use Psr\Log\LoggerInterface;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Control\CookieJar;
use SilverStripe\Control\Cookie;
use SilverStripe\Control\Director;
class CookieTest extends SapphireTest
{
@ -201,4 +206,67 @@ class CookieTest extends SapphireTest
$this->assertEmpty(Cookie::get('newCookie'));
$this->assertEmpty(Cookie::get('newCookie', false));
}
/**
* Check that warnings are not logged for https requests and when samesite is not "None"
* Test passes if no warning is logged
*/
public function testValidateSameSiteNoWarning(): void
{
// Throw an exception when a warning is logged so we can catch it
$mockLogger = $this->getMockBuilder(Logger::class)->setConstructorArgs(['testLogger'])->getMock();
$catchMessage = 'A warning was logged';
$mockLogger->expects($this->never())
->method('warning')
->willThrowException(new Exception($catchMessage));
Injector::inst()->registerService($mockLogger, LoggerInterface::class);
// Only samesite === 'None' should log a warning on non-https requests
Director::config()->set('alternate_base_url', 'http://insecure.example.com/');
Cookie::validateSameSite('Lax');
Cookie::validateSameSite('Strict');
// There should be no warnings logged for secure requests
Director::config()->set('alternate_base_url', 'https://secure.example.com/');
Cookie::validateSameSite('None');
Cookie::validateSameSite('Lax');
Cookie::validateSameSite('Strict');
}
/**
* Check whether warnings are correctly logged for non-https requests and samesite === "None"
*/
public function testValidateSameSiteWarning(): void
{
// Throw an exception when a warning is logged so we can catch it
$mockLogger = $this->getMockBuilder(Logger::class)->setConstructorArgs(['testLogger'])->getMock();
$catchMessage = 'A warning was logged';
$mockLogger->expects($this->once())
->method('warning')
->willThrowException(new Exception($catchMessage));
Injector::inst()->registerService($mockLogger, LoggerInterface::class);
Director::config()->set('alternate_base_url', 'http://insecure.example.com/');
$this->expectException(Exception::class);
$this->expectExceptionMessage($catchMessage);
Cookie::validateSameSite('None');
}
/**
* An exception should be thrown for an empty samesite value
*/
public function testValidateSameSiteInvalidEmpty(): void
{
$this->expectException(LogicException::class);
Cookie::validateSameSite('');
}
/**
* An exception should be thrown for an invalid samesite value
*/
public function testValidateSameSiteInvalidNotEmpty(): void
{
$this->expectException(LogicException::class);
Cookie::validateSameSite('invalid');
}
}

166
tests/php/Control/SessionTest.php
View File

@ -2,11 +2,19 @@
namespace SilverStripe\Control\Tests;
use http\Exception\BadMessageException;
use Exception;
use LogicException;
use Monolog\Logger;
use Psr\Log\LoggerInterface;
use ReflectionMethod;
use SilverStripe\Control\Cookie;
use SilverStripe\Control\Director;
use SilverStripe\Control\Session;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\NullHTTPRequest;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector;
/**
* Tests to cover the {@link Session} class
@ -359,4 +367,160 @@ class SessionTest extends SapphireTest
$_SESSION
);
}
public function testIsCookieSecure(): void
{
$session = new Session(null);
$methodIsCookieSecure = new ReflectionMethod($session, 'isCookieSecure');
$methodIsCookieSecure->setAccessible(true);
$this->assertFalse($methodIsCookieSecure->invoke($session, 'Lax', true));
$this->assertFalse($methodIsCookieSecure->invoke($session, 'Lax', false));
$this->assertTrue($methodIsCookieSecure->invoke($session, 'None', false));
$this->assertTrue($methodIsCookieSecure->invoke($session, 'None', true));
Config::modify()->set(Session::class, 'cookie_secure', true);
$this->assertTrue($methodIsCookieSecure->invoke($session, 'Lax', true));
$this->assertFalse($methodIsCookieSecure->invoke($session, 'Lax', false));
$this->assertTrue($methodIsCookieSecure->invoke($session, 'None', false));
$this->assertTrue($methodIsCookieSecure->invoke($session, 'None', true));
}
public function testBuildCookieParams(): void
{
$session = new Session(null);
$methodBuildCookieParams = new ReflectionMethod($session, 'buildCookieParams');
$methodBuildCookieParams->setAccessible(true);
$params = $methodBuildCookieParams->invoke($session, new NullHTTPRequest());
$this->assertSame(
[
'lifetime' => Session::config()->get('timeout'), // 0 by default but kitchen sink sets this to 1440
'path' => '/',
'domain' => null,
'secure' => false,
'httponly' => true,
'samesite' => 'Lax',
],
$params
);
Config::modify()->set(Session::class, 'timeout', 123);
Config::modify()->set(Session::class, 'cookie_path', 'test-path');
Config::modify()->set(Session::class, 'cookie_domain', 'test-domain');
$params = $methodBuildCookieParams->invoke($session, new NullHTTPRequest());
$this->assertSame(
[
'lifetime' => 123,
'path' => 'test-path',
'domain' => 'test-domain',
'secure' => false,
'httponly' => true,
'samesite' => 'Lax',
],
$params
);
Config::modify()->set(Session::class, 'cookie_path', '');
Config::modify()->set(Director::class, 'alternate_base_url', 'https://secure.example.com/some-path/');
$params = $methodBuildCookieParams->invoke($session, new NullHTTPRequest());
$this->assertSame(
[
'lifetime' => 123,
'path' => '/some-path/',
'domain' => 'test-domain',
'secure' => false,
'httponly' => true,
'samesite' => 'Lax',
],
$params
);
}
public function provideSecureSamesiteData(): array
{
$data = [];
foreach ([true, false] as $secure) {
foreach (['Strict', 'Lax', 'None'] as $sameSite) {
foreach (['https://secure.example.com/', 'http://insecure.example.com/'] as $alternateBase) {
if ($sameSite === 'None') {
// secure is always true if samesite is "None"
$secure = true;
} else {
// secure cannot be true for insecure requests
$secure = (strpos($alternateBase, 'https:') === 0) && $secure;
}
$data[] = [
$secure,
$sameSite,
$alternateBase,
[
'secure' => $secure,
'samesite' => $sameSite,
]
];
}
}
}
return $data;
}
/**
* @dataProvider provideSecureSamesiteData
*/
public function testBuildCookieParamsSecureAndSamesite(
bool $secure,
string $sameSite,
string $alternateBase,
array $expected
): void {
$session = new Session(null);
$methodBuildCookieParams = new ReflectionMethod($session, 'buildCookieParams');
$methodBuildCookieParams->setAccessible(true);
Config::modify()->set(Session::class, 'cookie_secure', $secure);
Config::modify()->set(Session::class, 'cookie_samesite', $sameSite);
Config::modify()->set(Director::class, 'alternate_base_url', $alternateBase);
$params = $methodBuildCookieParams->invoke($session, new NullHTTPRequest());
foreach ($expected as $key => $value) {
$secure = $secure ? 'true' : 'false';
$this->assertSame($value, $params[$key], "Inputs were 'secure': $secure, 'samesite': $sameSite, 'anternateBase': $alternateBase");
}
}
/**
* Check that the samesite value is being validated
*/
public function testBuildCookieParamsSamesiteIsValidated(): void
{
$session = new Session(null);
$methodBuildCookieParams = new ReflectionMethod($session, 'buildCookieParams');
$methodBuildCookieParams->setAccessible(true);
// Throw an exception when a warning is logged so we can catch it
$mockLogger = $this->getMockBuilder(Logger::class)->setConstructorArgs(['testLogger'])->getMock();
$catchMessage = 'A warning was logged';
$mockLogger->expects($this->once())
->method('warning')
->willThrowException(new Exception($catchMessage));
Injector::inst()->registerService($mockLogger, LoggerInterface::class);
// samesite "None" should log a warning for non-https requests
Config::modify()->set(Director::class, 'alternate_base_url', 'http://insecure.example.com/some-path');
Config::modify()->set(Session::class, 'cookie_samesite', 'None');
$this->expectException(Exception::class);
$this->expectExceptionMessage($catchMessage);
$methodBuildCookieParams->invoke($session, new NullHTTPRequest());
}
public function testInvalidSamesite(): void
{
$session = new Session(null);
$methodBuildCookieParams = new ReflectionMethod($session, 'buildCookieParams');
$methodBuildCookieParams->setAccessible(true);
$this->expectException(LogicException::class);
Config::modify()->set(Session::class, 'cookie_samesite', 'invalid');
$methodBuildCookieParams->invoke($session, new NullHTTPRequest());
}
}