From 898c756f0754ba18b4113849277d0faa03a6cc11 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 13:40:56 +0900 Subject: [PATCH 1/8] test: add tests for Token Randomization in Cookie CSRF --- .../SecurityCSRFCookieRandomizeTokenTest.php | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php diff --git a/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php b/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php new file mode 100644 index 000000000000..e4345ebec15c --- /dev/null +++ b/tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php @@ -0,0 +1,88 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Security; + +use CodeIgniter\Config\Factories; +use CodeIgniter\Cookie\Cookie; +use CodeIgniter\HTTP\IncomingRequest; +use CodeIgniter\HTTP\URI; +use CodeIgniter\HTTP\UserAgent; +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\Mock\MockAppConfig; +use CodeIgniter\Test\Mock\MockSecurity; +use Config\Security as SecurityConfig; + +/** + * @internal + */ +final class SecurityCSRFCookieRandomizeTokenTest extends CIUnitTestCase +{ + /** + * @var string CSRF protection hash + */ + private string $hash = '8b9218a55906f9dcc1dc263dce7f005a'; + + /** + * @var string CSRF randomized token + */ + private string $randomizedToken = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1'; + + protected function setUp(): void + { + parent::setUp(); + + $_COOKIE = []; + + $config = new SecurityConfig(); + $config->csrfProtection = Security::CSRF_PROTECTION_COOKIE; + $config->tokenRandomize = true; + Factories::injectMock('config', 'Security', $config); + + // Set Cookie value + $security = new MockSecurity(new MockAppConfig()); + $_COOKIE[$security->getCookieName()] = $this->hash; + + $this->resetServices(); + } + + public function testTokenIsReadFromCookie() + { + $security = new MockSecurity(new MockAppConfig()); + + $this->assertSame( + $this->randomizedToken, + $security->getHash() + ); + } + + public function testCSRFVerifySetNewCookie() + { + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_POST['foo'] = 'bar'; + $_POST['csrf_test_name'] = $this->randomizedToken; + + $request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent()); + + $security = new Security(new MockAppConfig()); + + $this->assertInstanceOf(Security::class, $security->verify($request)); + $this->assertLogged('info', 'CSRF token verified.'); + $this->assertCount(1, $_POST); + + /** @var Cookie $cookie */ + $cookie = $this->getPrivateProperty($security, 'cookie'); + $newHash = $cookie->getValue(); + + $this->assertNotSame($this->hash, $newHash); + $this->assertSame(32, strlen($newHash)); + } +} From 4466c88de8918c81e2c18e86a46125c2f8decb75 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 09:42:44 +0900 Subject: [PATCH 2/8] test: fix setUp() When Security depends on Request, we need to reset it. --- tests/system/Security/SecurityTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system/Security/SecurityTest.php b/tests/system/Security/SecurityTest.php index 160e35b17cb6..62d0afc92be6 100644 --- a/tests/system/Security/SecurityTest.php +++ b/tests/system/Security/SecurityTest.php @@ -36,7 +36,7 @@ protected function setUp(): void $_COOKIE = []; - Factories::reset(); + $this->resetServices(); } public function testBasicConfigIsSaved() From 333290dfeaab338b7976a63728cfd8ea27910ae7 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 15:56:11 +0900 Subject: [PATCH 3/8] refactor: remove $_COOKIE --- system/Security/Security.php | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index a85fb732945b..c0da7551ce52 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -154,6 +154,11 @@ class Security implements SecurityInterface */ private ?Session $session = null; + /** + * CSRF Hash in Cookie + */ + private ?string $hashInCookie = null; + /** * Constructor. * @@ -192,7 +197,8 @@ public function __construct(App $config) $this->configureSession(); } - $this->request = Services::request(); + $this->request = Services::request(); + $this->hashInCookie = $this->request->getCookie($this->cookieName); $this->generateHash(); } @@ -308,7 +314,7 @@ public function verify(RequestInterface $request) if ($this->regenerate) { $this->hash = null; if ($this->isCSRFCookie()) { - unset($_COOKIE[$this->cookieName]); + $this->hashInCookie = null; } else { // Session based CSRF protection $this->session->remove($this->tokenName); @@ -504,7 +510,7 @@ protected function generateHash(): string // sub-pages causing this feature to fail if ($this->isCSRFCookie()) { if ($this->isHashInCookie()) { - return $this->hash = $_COOKIE[$this->cookieName]; + return $this->hash = $this->hashInCookie; } } elseif ($this->session->has($this->tokenName)) { // Session based CSRF protection @@ -526,9 +532,14 @@ protected function generateHash(): string private function isHashInCookie(): bool { - return isset($_COOKIE[$this->cookieName]) - && is_string($_COOKIE[$this->cookieName]) - && preg_match('#^[0-9a-f]{32}$#iS', $_COOKIE[$this->cookieName]) === 1; + if ($this->hashInCookie === null) { + return false; + } + + $length = static::CSRF_HASH_BYTES * 2; + $pattern = '#^[0-9a-f]{' . $length . '}$#iS'; + + return preg_match($pattern, $this->hashInCookie) === 1; } private function saveHashInCookie(): void From 06f4cebe358aec51440d2c02b831f36d69bea85b Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 15:58:43 +0900 Subject: [PATCH 4/8] refactor: move `if` --- system/Security/Security.php | 44 +++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index c0da7551ce52..5ec843ae416a 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -200,7 +200,9 @@ public function __construct(App $config) $this->request = Services::request(); $this->hashInCookie = $this->request->getCookie($this->cookieName); - $this->generateHash(); + if ($this->hash === null) { + $this->generateHash(); + } } private function isCSRFCookie(): bool @@ -321,7 +323,9 @@ public function verify(RequestInterface $request) } } - $this->generateHash(); + if ($this->hash === null) { + $this->generateHash(); + } log_message('info', 'CSRF token verified.'); @@ -503,28 +507,26 @@ public function sanitizeFilename(string $str, bool $relativePath = false): strin */ protected function generateHash(): string { - if ($this->hash === null) { - // If the cookie exists we will use its value. - // We don't necessarily want to regenerate it with - // each page load since a page could contain embedded - // sub-pages causing this feature to fail - if ($this->isCSRFCookie()) { - if ($this->isHashInCookie()) { - return $this->hash = $this->hashInCookie; - } - } elseif ($this->session->has($this->tokenName)) { - // Session based CSRF protection - return $this->hash = $this->session->get($this->tokenName); + // If the cookie exists we will use its value. + // We don't necessarily want to regenerate it with + // each page load since a page could contain embedded + // sub-pages causing this feature to fail + if ($this->isCSRFCookie()) { + if ($this->isHashInCookie()) { + return $this->hash = $this->hashInCookie; } + } elseif ($this->session->has($this->tokenName)) { + // Session based CSRF protection + return $this->hash = $this->session->get($this->tokenName); + } - $this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES)); + $this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES)); - if ($this->isCSRFCookie()) { - $this->saveHashInCookie(); - } else { - // Session based CSRF protection - $this->saveHashInSession(); - } + if ($this->isCSRFCookie()) { + $this->saveHashInCookie(); + } else { + // Session based CSRF protection + $this->saveHashInSession(); } return $this->hash; From e056312d0c20f51c4d0b7bb64be66883d27b457b Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 16:02:11 +0900 Subject: [PATCH 5/8] refactor: extract restoreHash() method --- system/Security/Security.php | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 5ec843ae416a..3dc2ce61e6f4 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -502,6 +502,21 @@ public function sanitizeFilename(string $str, bool $relativePath = false): strin return stripslashes($str); } + /** + * Restore hash from Session or Cookie + */ + private function restoreHash(): void + { + if ($this->isCSRFCookie()) { + if ($this->isHashInCookie()) { + $this->hash = $this->hashInCookie; + } + } elseif ($this->session->has($this->tokenName)) { + // Session based CSRF protection + $this->hash = $this->session->get($this->tokenName); + } + } + /** * Generates the CSRF Hash. */ @@ -511,13 +526,10 @@ protected function generateHash(): string // We don't necessarily want to regenerate it with // each page load since a page could contain embedded // sub-pages causing this feature to fail - if ($this->isCSRFCookie()) { - if ($this->isHashInCookie()) { - return $this->hash = $this->hashInCookie; - } - } elseif ($this->session->has($this->tokenName)) { - // Session based CSRF protection - return $this->hash = $this->session->get($this->tokenName); + $this->restoreHash(); + + if ($this->hash !== null) { + return $this->hash; } $this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES)); From 01842619d0b7a3810089f2672c8b0c71833a5063 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 16:05:29 +0900 Subject: [PATCH 6/8] refactor: generateHash() always generate new hash --- system/Security/Security.php | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 3dc2ce61e6f4..365aa752091b 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -200,6 +200,7 @@ public function __construct(App $config) $this->request = Services::request(); $this->hashInCookie = $this->request->getCookie($this->cookieName); + $this->restoreHash(); if ($this->hash === null) { $this->generateHash(); } @@ -314,16 +315,6 @@ public function verify(RequestInterface $request) } if ($this->regenerate) { - $this->hash = null; - if ($this->isCSRFCookie()) { - $this->hashInCookie = null; - } else { - // Session based CSRF protection - $this->session->remove($this->tokenName); - } - } - - if ($this->hash === null) { $this->generateHash(); } @@ -518,20 +509,10 @@ private function restoreHash(): void } /** - * Generates the CSRF Hash. + * Generates (Regenerate) the CSRF Hash. */ protected function generateHash(): string { - // If the cookie exists we will use its value. - // We don't necessarily want to regenerate it with - // each page load since a page could contain embedded - // sub-pages causing this feature to fail - $this->restoreHash(); - - if ($this->hash !== null) { - return $this->hash; - } - $this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES)); if ($this->isCSRFCookie()) { From f4c1fa4cc7aad697571c4b0d2954412d2d3b3fab Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 16:08:35 +0900 Subject: [PATCH 7/8] refactor: extract removeTokenInRequest() method --- system/Security/Security.php | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index 365aa752091b..f2c754d202ae 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -302,6 +302,22 @@ public function verify(RequestInterface $request) throw SecurityException::forDisallowedAction(); } + $this->removeTokenInRequest($request); + + if ($this->regenerate) { + $this->generateHash(); + } + + log_message('info', 'CSRF token verified.'); + + return $this; + } + + /** + * Remove token in POST or JSON request data + */ + private function removeTokenInRequest(RequestInterface $request): void + { $json = json_decode($request->getBody() ?? ''); if (isset($_POST[$this->tokenName])) { @@ -313,14 +329,6 @@ public function verify(RequestInterface $request) unset($json->{$this->tokenName}); $request->setBody(json_encode($json)); } - - if ($this->regenerate) { - $this->generateHash(); - } - - log_message('info', 'CSRF token verified.'); - - return $this; } private function getPostedToken(RequestInterface $request): ?string From 2b890fe8678e5d5ea24fb02f4052a6e59f8cc89c Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 31 Jul 2022 16:21:48 +0900 Subject: [PATCH 8/8] docs: add doc comments --- system/Security/Security.php | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/system/Security/Security.php b/system/Security/Security.php index f2c754d202ae..529e10b0a8f7 100644 --- a/system/Security/Security.php +++ b/system/Security/Security.php @@ -54,7 +54,7 @@ class Security implements SecurityInterface protected $tokenRandomize = false; /** - * CSRF Hash + * CSRF Hash (without randomization) * * Random hash for Cross Site Request Forgery protection. * @@ -88,7 +88,7 @@ class Security implements SecurityInterface protected $cookie; /** - * CSRF Cookie Name + * CSRF Cookie Name (with Prefix) * * Cookie name for Cross Site Request Forgery protection. * @@ -155,7 +155,10 @@ class Security implements SecurityInterface private ?Session $session = null; /** - * CSRF Hash in Cookie + * CSRF Hash in Request Cookie + * + * The cookie value is always CSRF hash (without randomization) even if + * $tokenRandomize is true. */ private ?string $hashInCookie = null; @@ -249,7 +252,7 @@ public function CSRFVerify(RequestInterface $request) } /** - * Returns the CSRF Hash. + * Returns the CSRF Token. * * @deprecated Use `CodeIgniter\Security\Security::getHash()` instead of using this method. * @@ -351,7 +354,7 @@ private function getPostedToken(RequestInterface $request): ?string } /** - * Returns the CSRF Hash. + * Returns the CSRF Token. */ public function getHash(): ?string { @@ -360,6 +363,10 @@ public function getHash(): ?string /** * Randomize hash to avoid BREACH attacks. + * + * @params string $hash CSRF hash + * + * @return string CSRF token */ protected function randomize(string $hash): string { @@ -376,7 +383,11 @@ protected function randomize(string $hash): string /** * Derandomize the token. * + * @params string $token CSRF token + * * @throws InvalidArgumentException "hex2bin(): Hexadecimal input string must have an even length" + * + * @return string CSRF hash */ protected function derandomize(string $token): string {