diff --git a/system/Security/Security.php b/system/Security/Security.php index a85fb732945b..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. * @@ -154,6 +154,14 @@ class Security implements SecurityInterface */ private ?Session $session = null; + /** + * CSRF Hash in Request Cookie + * + * The cookie value is always CSRF hash (without randomization) even if + * $tokenRandomize is true. + */ + private ?string $hashInCookie = null; + /** * Constructor. * @@ -192,9 +200,13 @@ public function __construct(App $config) $this->configureSession(); } - $this->request = Services::request(); + $this->request = Services::request(); + $this->hashInCookie = $this->request->getCookie($this->cookieName); - $this->generateHash(); + $this->restoreHash(); + if ($this->hash === null) { + $this->generateHash(); + } } private function isCSRFCookie(): bool @@ -240,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. * @@ -293,6 +305,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])) { @@ -304,22 +332,6 @@ public function verify(RequestInterface $request) unset($json->{$this->tokenName}); $request->setBody(json_encode($json)); } - - if ($this->regenerate) { - $this->hash = null; - if ($this->isCSRFCookie()) { - unset($_COOKIE[$this->cookieName]); - } else { - // Session based CSRF protection - $this->session->remove($this->tokenName); - } - } - - $this->generateHash(); - - log_message('info', 'CSRF token verified.'); - - return $this; } private function getPostedToken(RequestInterface $request): ?string @@ -342,7 +354,7 @@ private function getPostedToken(RequestInterface $request): ?string } /** - * Returns the CSRF Hash. + * Returns the CSRF Token. */ public function getHash(): ?string { @@ -351,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 { @@ -367,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 { @@ -493,32 +513,32 @@ public function sanitizeFilename(string $str, bool $relativePath = false): strin } /** - * Generates the CSRF Hash. + * Restore hash from Session or Cookie */ - protected function generateHash(): string + private function restoreHash(): void { - 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 = $_COOKIE[$this->cookieName]; - } - } elseif ($this->session->has($this->tokenName)) { - // Session based CSRF protection - return $this->hash = $this->session->get($this->tokenName); + 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); + } + } - $this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES)); + /** + * Generates (Regenerate) the CSRF Hash. + */ + protected function generateHash(): string + { + $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; @@ -526,9 +546,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 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)); + } +} 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()