From d1792f07d15538115b3523b6d3d20ff63ca27c67 Mon Sep 17 00:00:00 2001 From: Nicolas CARPi Date: Mon, 15 Apr 2024 23:28:45 +0200 Subject: [PATCH 1/7] remove insecure rng providers and remove the openssl provider. We now rely exclusively on random_bytes(), as there are no reasons not to. Fix #121 --- README.md | 1 - docs/optional-configuration.md | 10 +---- lib/Providers/Rng/CSRNGProvider.php | 8 ---- lib/Providers/Rng/HashRNGProvider.php | 40 ------------------- lib/Providers/Rng/IRNGProvider.php | 2 - lib/Providers/Rng/OpenSSLRNGProvider.php | 29 -------------- lib/TwoFactorAuth.php | 21 +--------- tests/Providers/Rng/CSRNGProviderTest.php | 1 - tests/Providers/Rng/HashRNGProviderTest.php | 26 ------------ tests/Providers/Rng/IRNGProviderTest.php | 29 ++------------ .../Providers/Rng/OpenSSLRNGProviderTest.php | 39 ------------------ tests/Providers/Rng/TestRNGProvider.php | 8 ---- 12 files changed, 7 insertions(+), 207 deletions(-) delete mode 100644 lib/Providers/Rng/HashRNGProvider.php delete mode 100644 lib/Providers/Rng/OpenSSLRNGProvider.php delete mode 100644 tests/Providers/Rng/HashRNGProviderTest.php delete mode 100644 tests/Providers/Rng/OpenSSLRNGProviderTest.php diff --git a/README.md b/README.md index 96f566d..0731499 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,6 @@ You can make use of the included [Endroid](https://robthree.github.io/TwoFactorA * Requires PHP version >=8.1 * [cURL](http://php.net/manual/en/book.curl.php) when using the provided `QRServerProvider` (default), `ImageChartsQRCodeProvider` or `QRicketProvider` but you can also provide your own QR-code provider. -* [random_bytes()](http://php.net/manual/en/function.random-bytes.php), [OpenSSL](http://php.net/manual/en/book.openssl.php) or [Hash](http://php.net/manual/en/book.hash.php) depending on which built-in RNG you use (TwoFactorAuth will try to 'autodetect' and use the best available); however: feel free to provide your own (CS)RNG. Optionally, you may need: diff --git a/docs/optional-configuration.md b/docs/optional-configuration.md index d3e2556..e10a279 100644 --- a/docs/optional-configuration.md +++ b/docs/optional-configuration.md @@ -21,15 +21,7 @@ Argument | Default value | Use ### RNG providers -This library also comes with some [Random Number Generator (RNG)](https://en.wikipedia.org/wiki/Random_number_generation) providers. The RNG provider generates a number of random bytes and returns these bytes as a string. These values are then used to create the secret. By default (no RNG provider specified) TwoFactorAuth will try to determine the best available RNG provider to use in this order. - -1. [CSRNGProvider](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Rng/CSRNGProvider.php) for PHP7+ -2. [OpenSSLRNGProvider](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Rng/OpenSSLRNGProvider.php) where openssl is available -3. [HashRNGProvider](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Rng/HashRNGProvider.php) **non-cryptographically secure** fallback - -Each of these RNG providers have some constructor arguments that allow you to tweak some of the settings to use when creating the random bytes. - -You can also implement your own by implementing the [`IRNGProvider` interface](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Rng/IRNGProvider.php). +Should you feel the need to use a CSPRNG different than `random_bytes()`, you can use the `rngprovider` argument of the constructor to provide an object implementing the [`IRNGProvider`](https://github.com/RobThree/TwoFactorAuth/blob/master/lib/Providers/Rng/IRNGProvider.php) interface. ### Time providers diff --git a/lib/Providers/Rng/CSRNGProvider.php b/lib/Providers/Rng/CSRNGProvider.php index 97351f3..f078575 100644 --- a/lib/Providers/Rng/CSRNGProvider.php +++ b/lib/Providers/Rng/CSRNGProvider.php @@ -13,12 +13,4 @@ public function getRandomBytes(int $bytecount): string { return random_bytes($bytecount); // PHP7+ } - - /** - * {@inheritdoc} - */ - public function isCryptographicallySecure(): bool - { - return true; - } } diff --git a/lib/Providers/Rng/HashRNGProvider.php b/lib/Providers/Rng/HashRNGProvider.php deleted file mode 100644 index 20241da..0000000 --- a/lib/Providers/Rng/HashRNGProvider.php +++ /dev/null @@ -1,40 +0,0 @@ -algorithm, $algos, true)) { - throw new RNGException('Unsupported algorithm specified'); - } - } - - /** - * {@inheritdoc} - */ - public function getRandomBytes(int $bytecount): string - { - $result = ''; - $hash = mt_rand(); - for ($i = 0; $i < $bytecount; $i++) { - $hash = hash($this->algorithm, $hash . mt_rand(), true); - $result .= $hash[mt_rand(0, strlen($hash) - 1)]; - } - return $result; - } - - /** - * {@inheritdoc} - */ - public function isCryptographicallySecure(): bool - { - return false; - } -} diff --git a/lib/Providers/Rng/IRNGProvider.php b/lib/Providers/Rng/IRNGProvider.php index ed18453..08b7fa3 100644 --- a/lib/Providers/Rng/IRNGProvider.php +++ b/lib/Providers/Rng/IRNGProvider.php @@ -7,6 +7,4 @@ interface IRNGProvider { public function getRandomBytes(int $bytecount): string; - - public function isCryptographicallySecure(): bool; } diff --git a/lib/Providers/Rng/OpenSSLRNGProvider.php b/lib/Providers/Rng/OpenSSLRNGProvider.php deleted file mode 100644 index 4063847..0000000 --- a/lib/Providers/Rng/OpenSSLRNGProvider.php +++ /dev/null @@ -1,29 +0,0 @@ -requirestrong; - } -} diff --git a/lib/TwoFactorAuth.php b/lib/TwoFactorAuth.php index f3f867e..45efcbc 100644 --- a/lib/TwoFactorAuth.php +++ b/lib/TwoFactorAuth.php @@ -7,9 +7,7 @@ use RobThree\Auth\Providers\Qr\IQRCodeProvider; use RobThree\Auth\Providers\Qr\QRServerProvider; use RobThree\Auth\Providers\Rng\CSRNGProvider; -use RobThree\Auth\Providers\Rng\HashRNGProvider; use RobThree\Auth\Providers\Rng\IRNGProvider; -use RobThree\Auth\Providers\Rng\OpenSSLRNGProvider; use RobThree\Auth\Providers\Time\HttpTimeProvider; use RobThree\Auth\Providers\Time\ITimeProvider; use RobThree\Auth\Providers\Time\LocalMachineTimeProvider; @@ -51,14 +49,11 @@ public function __construct( /** * Create a new secret */ - public function createSecret(int $bits = 80, bool $requirecryptosecure = true): string + public function createSecret(int $bits = 80): string { $secret = ''; $bytes = (int)ceil($bits / 5); // We use 5 bits of each byte (since we have a 32-character 'alphabet' / BASE32) $rngprovider = $this->getRngProvider(); - if ($requirecryptosecure && !$rngprovider->isCryptographicallySecure()) { - throw new TwoFactorAuthException('RNG provider is not cryptographically secure'); - } $rnd = $rngprovider->getRandomBytes($bytes); for ($i = 0; $i < $bytes; $i++) { $secret .= self::$_base32[ord($rnd[$i]) & 31]; //Mask out left 3 bits for 0-31 values @@ -174,19 +169,7 @@ public function getQrCodeProvider(): IQRCodeProvider */ public function getRngProvider(): IRNGProvider { - if ($this->rngprovider !== null) { - return $this->rngprovider; - } - if (function_exists('random_bytes')) { - return $this->rngprovider = new CSRNGProvider(); - } - if (function_exists('openssl_random_pseudo_bytes')) { - return $this->rngprovider = new OpenSSLRNGProvider(); - } - if (function_exists('hash')) { - return $this->rngprovider = new HashRNGProvider(); - } - throw new TwoFactorAuthException('Unable to find a suited RNGProvider'); + return $this->rngprovider ?? new CSRNGProvider(); } public function getTimeProvider(): ITimeProvider diff --git a/tests/Providers/Rng/CSRNGProviderTest.php b/tests/Providers/Rng/CSRNGProviderTest.php index 64f66e8..abb6488 100644 --- a/tests/Providers/Rng/CSRNGProviderTest.php +++ b/tests/Providers/Rng/CSRNGProviderTest.php @@ -21,7 +21,6 @@ public function testCSRNGProvidersReturnExpectedNumberOfBytes(): void foreach ($this->rngTestLengths as $l) { $this->assertSame($l, strlen($rng->getRandomBytes($l))); } - $this->assertTrue($rng->isCryptographicallySecure()); } else { $this->expectNotToPerformAssertions(); } diff --git a/tests/Providers/Rng/HashRNGProviderTest.php b/tests/Providers/Rng/HashRNGProviderTest.php deleted file mode 100644 index 4a71930..0000000 --- a/tests/Providers/Rng/HashRNGProviderTest.php +++ /dev/null @@ -1,26 +0,0 @@ -rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); - } - - $this->assertFalse($rng->isCryptographicallySecure()); - } -} diff --git a/tests/Providers/Rng/IRNGProviderTest.php b/tests/Providers/Rng/IRNGProviderTest.php index 7ff40c8..743774a 100644 --- a/tests/Providers/Rng/IRNGProviderTest.php +++ b/tests/Providers/Rng/IRNGProviderTest.php @@ -7,39 +7,18 @@ use PHPUnit\Framework\TestCase; use RobThree\Auth\Algorithm; use RobThree\Auth\TwoFactorAuth; -use RobThree\Auth\TwoFactorAuthException; class IRNGProviderTest extends TestCase { - public function testCreateSecretThrowsOnInsecureRNGProvider(): void + public function testCreateSecret(): void { - $rng = new TestRNGProvider(); - - $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); - - $this->expectException(TwoFactorAuthException::class); - $tfa->createSecret(); - } - - public function testCreateSecretOverrideSecureDoesNotThrowOnInsecureRNG(): void - { - $rng = new TestRNGProvider(); - - $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); - $this->assertSame('ABCDEFGHIJKLMNOP', $tfa->createSecret(80, false)); - } - - public function testCreateSecretDoesNotThrowOnSecureRNGProvider(): void - { - $rng = new TestRNGProvider(true); - - $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); - $this->assertSame('ABCDEFGHIJKLMNOP', $tfa->createSecret()); + $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null); + $this->assertIsString($tfa->createSecret()); } public function testCreateSecretGeneratesDesiredAmountOfEntropy(): void { - $rng = new TestRNGProvider(true); + $rng = new TestRNGProvider(); $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); $this->assertSame('A', $tfa->createSecret(5)); diff --git a/tests/Providers/Rng/OpenSSLRNGProviderTest.php b/tests/Providers/Rng/OpenSSLRNGProviderTest.php deleted file mode 100644 index 368c7cc..0000000 --- a/tests/Providers/Rng/OpenSSLRNGProviderTest.php +++ /dev/null @@ -1,39 +0,0 @@ -rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); - } - - $this->assertTrue($rng->isCryptographicallySecure()); - } - - /** - * @return void - */ - public function testNonStrongOpenSSLRNGProvidersReturnExpectedNumberOfBytes() - { - $rng = new OpenSSLRNGProvider(false); - foreach ($this->rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); - } - - $this->assertFalse($rng->isCryptographicallySecure()); - } -} diff --git a/tests/Providers/Rng/TestRNGProvider.php b/tests/Providers/Rng/TestRNGProvider.php index c166c66..500194e 100644 --- a/tests/Providers/Rng/TestRNGProvider.php +++ b/tests/Providers/Rng/TestRNGProvider.php @@ -25,12 +25,4 @@ public function getRandomBytes(int $bytecount): string return $result; } - - /** - * {@inheritdoc} - */ - public function isCryptographicallySecure(): bool - { - return $this->isSecure; - } } From e44997c90c340cc37f83968251011e551e432443 Mon Sep 17 00:00:00 2001 From: Nicolas CARPi Date: Mon, 15 Apr 2024 23:34:25 +0200 Subject: [PATCH 2/7] remove the isSecure property of the test rng class --- tests/Providers/Rng/TestRNGProvider.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/Providers/Rng/TestRNGProvider.php b/tests/Providers/Rng/TestRNGProvider.php index 500194e..fd5b591 100644 --- a/tests/Providers/Rng/TestRNGProvider.php +++ b/tests/Providers/Rng/TestRNGProvider.php @@ -8,10 +8,6 @@ class TestRNGProvider implements IRNGProvider { - public function __construct(private readonly bool $isSecure = false) - { - } - /** * {@inheritdoc} */ From f6da6bee6db93b47a1a27f666543ea01c167cf5b Mon Sep 17 00:00:00 2001 From: Nicolas CARPi Date: Mon, 15 Apr 2024 23:58:37 +0200 Subject: [PATCH 3/7] remove pointless test rng class we were testing a test class, which didn't make a lot of sense. --- tests/Providers/Rng/CSRNGProviderTest.php | 13 +++--------- tests/Providers/Rng/IRNGProviderTest.php | 13 ------------ tests/Providers/Rng/TestRNGProvider.php | 24 ----------------------- 3 files changed, 3 insertions(+), 47 deletions(-) delete mode 100644 tests/Providers/Rng/TestRNGProvider.php diff --git a/tests/Providers/Rng/CSRNGProviderTest.php b/tests/Providers/Rng/CSRNGProviderTest.php index abb6488..39739e9 100644 --- a/tests/Providers/Rng/CSRNGProviderTest.php +++ b/tests/Providers/Rng/CSRNGProviderTest.php @@ -11,18 +11,11 @@ class CSRNGProviderTest extends TestCase { use NeedsRngLengths; - /** - * @requires function random_bytes - */ public function testCSRNGProvidersReturnExpectedNumberOfBytes(): void { - if (function_exists('random_bytes')) { - $rng = new CSRNGProvider(); - foreach ($this->rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); - } - } else { - $this->expectNotToPerformAssertions(); + $rng = new CSRNGProvider(); + foreach ($this->rngTestLengths as $l) { + $this->assertSame($l, strlen($rng->getRandomBytes($l))); } } } diff --git a/tests/Providers/Rng/IRNGProviderTest.php b/tests/Providers/Rng/IRNGProviderTest.php index 743774a..fd2c742 100644 --- a/tests/Providers/Rng/IRNGProviderTest.php +++ b/tests/Providers/Rng/IRNGProviderTest.php @@ -15,17 +15,4 @@ public function testCreateSecret(): void $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null); $this->assertIsString($tfa->createSecret()); } - - public function testCreateSecretGeneratesDesiredAmountOfEntropy(): void - { - $rng = new TestRNGProvider(); - - $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); - $this->assertSame('A', $tfa->createSecret(5)); - $this->assertSame('AB', $tfa->createSecret(6)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ', $tfa->createSecret(128)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(160)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(320)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567A', $tfa->createSecret(321)); - } } diff --git a/tests/Providers/Rng/TestRNGProvider.php b/tests/Providers/Rng/TestRNGProvider.php deleted file mode 100644 index fd5b591..0000000 --- a/tests/Providers/Rng/TestRNGProvider.php +++ /dev/null @@ -1,24 +0,0 @@ - Date: Tue, 16 Apr 2024 00:06:40 +0200 Subject: [PATCH 4/7] Revert "remove pointless test rng class" This reverts commit f6da6bee6db93b47a1a27f666543ea01c167cf5b. --- tests/Providers/Rng/CSRNGProviderTest.php | 13 +++++++++--- tests/Providers/Rng/IRNGProviderTest.php | 13 ++++++++++++ tests/Providers/Rng/TestRNGProvider.php | 24 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 tests/Providers/Rng/TestRNGProvider.php diff --git a/tests/Providers/Rng/CSRNGProviderTest.php b/tests/Providers/Rng/CSRNGProviderTest.php index 39739e9..abb6488 100644 --- a/tests/Providers/Rng/CSRNGProviderTest.php +++ b/tests/Providers/Rng/CSRNGProviderTest.php @@ -11,11 +11,18 @@ class CSRNGProviderTest extends TestCase { use NeedsRngLengths; + /** + * @requires function random_bytes + */ public function testCSRNGProvidersReturnExpectedNumberOfBytes(): void { - $rng = new CSRNGProvider(); - foreach ($this->rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); + if (function_exists('random_bytes')) { + $rng = new CSRNGProvider(); + foreach ($this->rngTestLengths as $l) { + $this->assertSame($l, strlen($rng->getRandomBytes($l))); + } + } else { + $this->expectNotToPerformAssertions(); } } } diff --git a/tests/Providers/Rng/IRNGProviderTest.php b/tests/Providers/Rng/IRNGProviderTest.php index fd2c742..743774a 100644 --- a/tests/Providers/Rng/IRNGProviderTest.php +++ b/tests/Providers/Rng/IRNGProviderTest.php @@ -15,4 +15,17 @@ public function testCreateSecret(): void $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null); $this->assertIsString($tfa->createSecret()); } + + public function testCreateSecretGeneratesDesiredAmountOfEntropy(): void + { + $rng = new TestRNGProvider(); + + $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); + $this->assertSame('A', $tfa->createSecret(5)); + $this->assertSame('AB', $tfa->createSecret(6)); + $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ', $tfa->createSecret(128)); + $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(160)); + $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(320)); + $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567A', $tfa->createSecret(321)); + } } diff --git a/tests/Providers/Rng/TestRNGProvider.php b/tests/Providers/Rng/TestRNGProvider.php new file mode 100644 index 0000000..fd5b591 --- /dev/null +++ b/tests/Providers/Rng/TestRNGProvider.php @@ -0,0 +1,24 @@ + Date: Tue, 16 Apr 2024 00:43:12 +0200 Subject: [PATCH 5/7] Reapply "remove pointless test rng class" This reverts commit 06220d4a543b759f2d32e14d2f7b5fd61a8fe37c. --- tests/Providers/Rng/CSRNGProviderTest.php | 13 +++--------- tests/Providers/Rng/IRNGProviderTest.php | 13 ------------ tests/Providers/Rng/TestRNGProvider.php | 24 ----------------------- 3 files changed, 3 insertions(+), 47 deletions(-) delete mode 100644 tests/Providers/Rng/TestRNGProvider.php diff --git a/tests/Providers/Rng/CSRNGProviderTest.php b/tests/Providers/Rng/CSRNGProviderTest.php index abb6488..39739e9 100644 --- a/tests/Providers/Rng/CSRNGProviderTest.php +++ b/tests/Providers/Rng/CSRNGProviderTest.php @@ -11,18 +11,11 @@ class CSRNGProviderTest extends TestCase { use NeedsRngLengths; - /** - * @requires function random_bytes - */ public function testCSRNGProvidersReturnExpectedNumberOfBytes(): void { - if (function_exists('random_bytes')) { - $rng = new CSRNGProvider(); - foreach ($this->rngTestLengths as $l) { - $this->assertSame($l, strlen($rng->getRandomBytes($l))); - } - } else { - $this->expectNotToPerformAssertions(); + $rng = new CSRNGProvider(); + foreach ($this->rngTestLengths as $l) { + $this->assertSame($l, strlen($rng->getRandomBytes($l))); } } } diff --git a/tests/Providers/Rng/IRNGProviderTest.php b/tests/Providers/Rng/IRNGProviderTest.php index 743774a..fd2c742 100644 --- a/tests/Providers/Rng/IRNGProviderTest.php +++ b/tests/Providers/Rng/IRNGProviderTest.php @@ -15,17 +15,4 @@ public function testCreateSecret(): void $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, null); $this->assertIsString($tfa->createSecret()); } - - public function testCreateSecretGeneratesDesiredAmountOfEntropy(): void - { - $rng = new TestRNGProvider(); - - $tfa = new TwoFactorAuth('Test', 6, 30, Algorithm::Sha1, null, $rng); - $this->assertSame('A', $tfa->createSecret(5)); - $this->assertSame('AB', $tfa->createSecret(6)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ', $tfa->createSecret(128)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(160)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567', $tfa->createSecret(320)); - $this->assertSame('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVWXYZ234567A', $tfa->createSecret(321)); - } } diff --git a/tests/Providers/Rng/TestRNGProvider.php b/tests/Providers/Rng/TestRNGProvider.php deleted file mode 100644 index fd5b591..0000000 --- a/tests/Providers/Rng/TestRNGProvider.php +++ /dev/null @@ -1,24 +0,0 @@ - Date: Tue, 16 Apr 2024 17:33:40 +0200 Subject: [PATCH 6/7] assing rng provider to class attribute this also aligns with other providers --- lib/TwoFactorAuth.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/TwoFactorAuth.php b/lib/TwoFactorAuth.php index 45efcbc..f2f6e98 100644 --- a/lib/TwoFactorAuth.php +++ b/lib/TwoFactorAuth.php @@ -169,7 +169,7 @@ public function getQrCodeProvider(): IQRCodeProvider */ public function getRngProvider(): IRNGProvider { - return $this->rngprovider ?? new CSRNGProvider(); + return $this->rngprovider ??= new CSRNGProvider(); } public function getTimeProvider(): ITimeProvider From ca921e41c88cd5ad0a6c451b0221ebf3a496b13f Mon Sep 17 00:00:00 2001 From: Nicolas CARPi Date: Tue, 16 Apr 2024 17:40:32 +0200 Subject: [PATCH 7/7] remove polyfill for hash_equals --- lib/TwoFactorAuth.php | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/lib/TwoFactorAuth.php b/lib/TwoFactorAuth.php index f2f6e98..0133448 100644 --- a/lib/TwoFactorAuth.php +++ b/lib/TwoFactorAuth.php @@ -4,6 +4,8 @@ namespace RobThree\Auth; +use function hash_equals; + use RobThree\Auth\Providers\Qr\IQRCodeProvider; use RobThree\Auth\Providers\Qr\QRServerProvider; use RobThree\Auth\Providers\Rng\CSRNGProvider; @@ -93,7 +95,7 @@ public function verifyCode(string $secret, string $code, int $discrepancy = 1, ? for ($i = -$discrepancy; $i <= $discrepancy; $i++) { $ts = $timestamp + ($i * $this->period); $slice = $this->getTimeSlice($ts); - $timeslice = $this->codeEquals($this->getCode($secret, $ts), $code) ? $slice : $timeslice; + $timeslice = hash_equals($this->getCode($secret, $ts), $code) ? $slice : $timeslice; } return $timeslice > 0; @@ -178,27 +180,6 @@ public function getTimeProvider(): ITimeProvider return $this->timeprovider ??= new LocalMachineTimeProvider(); } - /** - * Timing-attack safe comparison of 2 codes (see http://blog.ircmaxell.com/2014/11/its-all-about-time.html) - */ - private function codeEquals(string $safe, string $user): bool - { - if (function_exists('hash_equals')) { - return hash_equals($safe, $user); - } - // In general, it's not possible to prevent length leaks. So it's OK to leak the length. The important part is that - // we don't leak information about the difference of the two strings. - if (strlen($safe) === strlen($user)) { - $result = 0; - $strlen = strlen($safe); - for ($i = 0; $i < $strlen; $i++) { - $result |= (ord($safe[$i]) ^ ord($user[$i])); - } - return $result === 0; - } - return false; - } - private function getTime(?int $time = null): int { return $time ?? $this->getTimeProvider()->getTime();