Skip to content

Commit ba8ca6e

Browse files
kenjiskenjis
authored
Merge pull request #6320 from kenjis/remove-superglobal-from-Security
refactor: CSRF protection
2 parents 170d84a + 2b890fe commit ba8ca6e

File tree

3 files changed

+160
-47
lines changed

3 files changed

+160
-47
lines changed

‎system/Security/Security.php

Lines changed: 71 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class Security implements SecurityInterface
5454
protected $tokenRandomize = false;
5555

5656
/**
57-
* CSRF Hash
57+
* CSRF Hash (without randomization)
5858
*
5959
* Random hash for Cross Site Request Forgery protection.
6060
*
@@ -88,7 +88,7 @@ class Security implements SecurityInterface
8888
protected $cookie;
8989

9090
/**
91-
* CSRF Cookie Name
91+
* CSRF Cookie Name (with Prefix)
9292
*
9393
* Cookie name for Cross Site Request Forgery protection.
9494
*
@@ -154,6 +154,14 @@ class Security implements SecurityInterface
154154
*/
155155
private ?Session $session = null;
156156

157+
/**
158+
* CSRF Hash in Request Cookie
159+
*
160+
* The cookie value is always CSRF hash (without randomization) even if
161+
* $tokenRandomize is true.
162+
*/
163+
private ?string $hashInCookie = null;
164+
157165
/**
158166
* Constructor.
159167
*
@@ -192,9 +200,13 @@ public function __construct(App $config)
192200
$this->configureSession();
193201
}
194202

195-
$this->request = Services::request();
203+
$this->request = Services::request();
204+
$this->hashInCookie = $this->request->getCookie($this->cookieName);
196205

197-
$this->generateHash();
206+
$this->restoreHash();
207+
if ($this->hash === null) {
208+
$this->generateHash();
209+
}
198210
}
199211

200212
private function isCSRFCookie(): bool
@@ -240,7 +252,7 @@ public function CSRFVerify(RequestInterface $request)
240252
}
241253

242254
/**
243-
* Returns the CSRF Hash.
255+
* Returns the CSRF Token.
244256
*
245257
* @deprecated Use `CodeIgniter\Security\Security::getHash()` instead of using this method.
246258
*
@@ -293,6 +305,22 @@ public function verify(RequestInterface $request)
293305
throw SecurityException::forDisallowedAction();
294306
}
295307

308+
$this->removeTokenInRequest($request);
309+
310+
if ($this->regenerate) {
311+
$this->generateHash();
312+
}
313+
314+
log_message('info', 'CSRF token verified.');
315+
316+
return $this;
317+
}
318+
319+
/**
320+
* Remove token in POST or JSON request data
321+
*/
322+
private function removeTokenInRequest(RequestInterface $request): void
323+
{
296324
$json = json_decode($request->getBody() ?? '');
297325

298326
if (isset($_POST[$this->tokenName])) {
@@ -304,22 +332,6 @@ public function verify(RequestInterface $request)
304332
unset($json->{$this->tokenName});
305333
$request->setBody(json_encode($json));
306334
}
307-
308-
if ($this->regenerate) {
309-
$this->hash = null;
310-
if ($this->isCSRFCookie()) {
311-
unset($_COOKIE[$this->cookieName]);
312-
} else {
313-
// Session based CSRF protection
314-
$this->session->remove($this->tokenName);
315-
}
316-
}
317-
318-
$this->generateHash();
319-
320-
log_message('info', 'CSRF token verified.');
321-
322-
return $this;
323335
}
324336

325337
private function getPostedToken(RequestInterface $request): ?string
@@ -342,7 +354,7 @@ private function getPostedToken(RequestInterface $request): ?string
342354
}
343355

344356
/**
345-
* Returns the CSRF Hash.
357+
* Returns the CSRF Token.
346358
*/
347359
public function getHash(): ?string
348360
{
@@ -351,6 +363,10 @@ public function getHash(): ?string
351363

352364
/**
353365
* Randomize hash to avoid BREACH attacks.
366+
*
367+
* @params string $hash CSRF hash
368+
*
369+
* @return string CSRF token
354370
*/
355371
protected function randomize(string $hash): string
356372
{
@@ -367,7 +383,11 @@ protected function randomize(string $hash): string
367383
/**
368384
* Derandomize the token.
369385
*
386+
* @params string $token CSRF token
387+
*
370388
* @throws InvalidArgumentException "hex2bin(): Hexadecimal input string must have an even length"
389+
*
390+
* @return string CSRF hash
371391
*/
372392
protected function derandomize(string $token): string
373393
{
@@ -493,42 +513,47 @@ public function sanitizeFilename(string $str, bool $relativePath = false): strin
493513
}
494514

495515
/**
496-
* Generates the CSRF Hash.
516+
* Restore hash from Session or Cookie
497517
*/
498-
protected function generateHash(): string
518+
private function restoreHash(): void
499519
{
500-
if ($this->hash === null) {
501-
// If the cookie exists we will use its value.
502-
// We don't necessarily want to regenerate it with
503-
// each page load since a page could contain embedded
504-
// sub-pages causing this feature to fail
505-
if ($this->isCSRFCookie()) {
506-
if ($this->isHashInCookie()) {
507-
return $this->hash = $_COOKIE[$this->cookieName];
508-
}
509-
} elseif ($this->session->has($this->tokenName)) {
510-
// Session based CSRF protection
511-
return $this->hash = $this->session->get($this->tokenName);
520+
if ($this->isCSRFCookie()) {
521+
if ($this->isHashInCookie()) {
522+
$this->hash = $this->hashInCookie;
512523
}
524+
} elseif ($this->session->has($this->tokenName)) {
525+
// Session based CSRF protection
526+
$this->hash = $this->session->get($this->tokenName);
527+
}
528+
}
513529

514-
$this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES));
530+
/**
531+
* Generates (Regenerate) the CSRF Hash.
532+
*/
533+
protected function generateHash(): string
534+
{
535+
$this->hash = bin2hex(random_bytes(static::CSRF_HASH_BYTES));
515536

516-
if ($this->isCSRFCookie()) {
517-
$this->saveHashInCookie();
518-
} else {
519-
// Session based CSRF protection
520-
$this->saveHashInSession();
521-
}
537+
if ($this->isCSRFCookie()) {
538+
$this->saveHashInCookie();
539+
} else {
540+
// Session based CSRF protection
541+
$this->saveHashInSession();
522542
}
523543

524544
return $this->hash;
525545
}
526546

527547
private function isHashInCookie(): bool
528548
{
529-
return isset($_COOKIE[$this->cookieName])
530-
&& is_string($_COOKIE[$this->cookieName])
531-
&& preg_match('#^[0-9a-f]{32}$#iS', $_COOKIE[$this->cookieName]) === 1;
549+
if ($this->hashInCookie === null) {
550+
return false;
551+
}
552+
553+
$length = static::CSRF_HASH_BYTES * 2;
554+
$pattern = '#^[0-9a-f]{' . $length . '}$#iS';
555+
556+
return preg_match($pattern, $this->hashInCookie) === 1;
532557
}
533558

534559
private function saveHashInCookie(): void

‎tests/system/Security/SecurityCSRFCookieRandomizeTokenTest.php

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
<?php
2+
3+
/**
4+
* This file is part of CodeIgniter 4 framework.
5+
*
6+
* (c) CodeIgniter Foundation <[email protected]>
7+
*
8+
* For the full copyright and license information, please view
9+
* the LICENSE file that was distributed with this source code.
10+
*/
11+
12+
namespace CodeIgniter\Security;
13+
14+
use CodeIgniter\Config\Factories;
15+
use CodeIgniter\Cookie\Cookie;
16+
use CodeIgniter\HTTP\IncomingRequest;
17+
use CodeIgniter\HTTP\URI;
18+
use CodeIgniter\HTTP\UserAgent;
19+
use CodeIgniter\Test\CIUnitTestCase;
20+
use CodeIgniter\Test\Mock\MockAppConfig;
21+
use CodeIgniter\Test\Mock\MockSecurity;
22+
use Config\Security as SecurityConfig;
23+
24+
/**
25+
* @internal
26+
*/
27+
final class SecurityCSRFCookieRandomizeTokenTest extends CIUnitTestCase
28+
{
29+
/**
30+
* @var string CSRF protection hash
31+
*/
32+
private string $hash = '8b9218a55906f9dcc1dc263dce7f005a';
33+
34+
/**
35+
* @var string CSRF randomized token
36+
*/
37+
private string $randomizedToken = '8bc70b67c91494e815c7d2219c1ae0ab005513c290126d34d41bf41c5265e0f1';
38+
39+
protected function setUp(): void
40+
{
41+
parent::setUp();
42+
43+
$_COOKIE = [];
44+
45+
$config = new SecurityConfig();
46+
$config->csrfProtection = Security::CSRF_PROTECTION_COOKIE;
47+
$config->tokenRandomize = true;
48+
Factories::injectMock('config', 'Security', $config);
49+
50+
// Set Cookie value
51+
$security = new MockSecurity(new MockAppConfig());
52+
$_COOKIE[$security->getCookieName()] = $this->hash;
53+
54+
$this->resetServices();
55+
}
56+
57+
public function testTokenIsReadFromCookie()
58+
{
59+
$security = new MockSecurity(new MockAppConfig());
60+
61+
$this->assertSame(
62+
$this->randomizedToken,
63+
$security->getHash()
64+
);
65+
}
66+
67+
public function testCSRFVerifySetNewCookie()
68+
{
69+
$_SERVER['REQUEST_METHOD'] = 'POST';
70+
$_POST['foo'] = 'bar';
71+
$_POST['csrf_test_name'] = $this->randomizedToken;
72+
73+
$request = new IncomingRequest(new MockAppConfig(), new URI('http://badurl.com'), null, new UserAgent());
74+
75+
$security = new Security(new MockAppConfig());
76+
77+
$this->assertInstanceOf(Security::class, $security->verify($request));
78+
$this->assertLogged('info', 'CSRF token verified.');
79+
$this->assertCount(1, $_POST);
80+
81+
/** @var Cookie $cookie */
82+
$cookie = $this->getPrivateProperty($security, 'cookie');
83+
$newHash = $cookie->getValue();
84+
85+
$this->assertNotSame($this->hash, $newHash);
86+
$this->assertSame(32, strlen($newHash));
87+
}
88+
}

‎tests/system/Security/SecurityTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ protected function setUp(): void
3636

3737
$_COOKIE = [];
3838

39-
Factories::reset();
39+
$this->resetServices();
4040
}
4141

4242
public function testBasicConfigIsSaved()

0 commit comments

Comments
 (0)