Match only for actual session cookie

OVH has implemented load balancing in a very questionable way where the reverse proxy actually internally adds some cookies which would trigger a security exception. To work around this, this change only checks for the session cookie.
This commit is contained in:
Lukas Reschke 2016-08-09 19:09:53 +02:00
parent 1914e7082a
commit 2ae08d6fc2
No known key found for this signature in database
GPG Key ID: B9F6980CF6E759B1
2 changed files with 92 additions and 3 deletions

View File

@ -485,6 +485,19 @@ class Request implements \ArrayAccess, \Countable, IRequest {
return $this->csrfTokenManager->isTokenValid($token); return $this->csrfTokenManager->isTokenValid($token);
} }
/**
* Whether the cookie checks are required
*
* @return bool
*/
private function cookieCheckRequired() {
if($this->getCookie(session_name()) === null && $this->getCookie('oc_token') === null) {
return false;
}
return true;
}
/** /**
* Checks if the strict cookie has been sent with the request if the request * Checks if the strict cookie has been sent with the request if the request
* is including any cookies. * is including any cookies.
@ -493,7 +506,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
* @since 9.1.0 * @since 9.1.0
*/ */
public function passesStrictCookieCheck() { public function passesStrictCookieCheck() {
if(count($this->cookies) === 0) { if(!$this->cookieCheckRequired()) {
return true; return true;
} }
if($this->getCookie('nc_sameSiteCookiestrict') === 'true' if($this->getCookie('nc_sameSiteCookiestrict') === 'true'
@ -511,7 +524,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
* @since 9.1.0 * @since 9.1.0
*/ */
public function passesLaxCookieCheck() { public function passesLaxCookieCheck() {
if(count($this->cookies) === 0) { if(!$this->cookieCheckRequired()) {
return true; return true;
} }
if($this->getCookie('nc_sameSiteCookielax') === 'true') { if($this->getCookie('nc_sameSiteCookielax') === 'true') {

View File

@ -1469,6 +1469,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookiestrict' => 'true', 'nc_sameSiteCookiestrict' => 'true',
], ],
], ],
@ -1495,6 +1496,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookiestrict' => 'true', 'nc_sameSiteCookiestrict' => 'true',
'nc_sameSiteCookielax' => 'true', 'nc_sameSiteCookielax' => 'true',
], ],
@ -1509,7 +1511,76 @@ class RequestTest extends \Test\TestCase {
$this->assertTrue($request->passesStrictCookieCheck()); $this->assertTrue($request->passesStrictCookieCheck());
} }
public function testFailsSRFCheckWithPostAndWithCookies() { public function testPassesStrictCookieCheckWithRandomCookies() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'RandomCookie' => 'asdf',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertTrue($request->passesStrictCookieCheck());
}
public function testFailsStrictCookieCheckWithSessionCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
session_name() => 'asdf',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertFalse($request->passesStrictCookieCheck());
}
public function testFailsStrictCookieCheckWithRememberMeCookie() {
/** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName'])
->setConstructorArgs([
[
'server' => [
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
],
'cookies' => [
'oc_token' => 'asdf',
],
],
$this->secureRandom,
$this->config,
$this->csrfTokenManager,
$this->stream
])
->getMock();
$this->assertFalse($request->passesStrictCookieCheck());
}
public function testFailsCSRFCheckWithPostAndWithCookies() {
/** @var Request $request */ /** @var Request $request */
$request = $this->getMockBuilder('\OC\AppFramework\Http\Request') $request = $this->getMockBuilder('\OC\AppFramework\Http\Request')
->setMethods(['getScriptName']) ->setMethods(['getScriptName'])
@ -1519,6 +1590,7 @@ class RequestTest extends \Test\TestCase {
'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'requesttoken' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'foo' => 'bar', 'foo' => 'bar',
], ],
], ],
@ -1545,6 +1617,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookielax' => 'true', 'nc_sameSiteCookielax' => 'true',
], ],
], ],
@ -1568,6 +1641,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookiestrict' => 'true', 'nc_sameSiteCookiestrict' => 'true',
], ],
], ],
@ -1591,6 +1665,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookielax' => 'true', 'nc_sameSiteCookielax' => 'true',
], ],
], ],
@ -1614,6 +1689,7 @@ class RequestTest extends \Test\TestCase {
'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds', 'HTTP_REQUESTTOKEN' => 'AAAHGxsTCTc3BgMQESAcNR0OAR0=:MyTotalSecretShareds',
], ],
'cookies' => [ 'cookies' => [
session_name() => 'asdf',
'nc_sameSiteCookiestrict' => 'true', 'nc_sameSiteCookiestrict' => 'true',
], ],
], ],