Merge pull request #6320 from kenjis/remove-superglobal-from-Security

refactor: CSRF protection

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,42 +513,47 @@ 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;
  }
 
  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

tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php

@@ -0,0 +1,88 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <admin@codeigniter.com>
+ *
+ * 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));
+ }
+}

tests/system/Security/SecurityTest.php

@@ -36,7 +36,7 @@ protected function setUp(): void
 
  $_COOKIE = [];
 
- Factories::reset();
+ $this->resetServices();
  }
 
  public function testBasicConfigIsSaved()