From d1792f07d15538115b3523b6d3d20ff63ca27c67 Mon Sep 17 00:00:00 2001 From: Nicolas CARPi Date: Mon, 15 Apr 2024 23:28:45 +0200 Subject: [PATCH] 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 @@ class CSRNGProvider implements IRNGProvider { 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 @@ namespace RobThree\Auth\Providers\Rng; 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 @@ namespace RobThree\Auth; 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 @@ class TwoFactorAuth /** * 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 @@ class TwoFactorAuth */ 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 @@ class CSRNGProviderTest extends TestCase 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 @@ namespace Tests\Providers\Rng; 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 @@ class TestRNGProvider implements IRNGProvider return $result; } - - /** - * {@inheritdoc} - */ - public function isCryptographicallySecure(): bool - { - return $this->isSecure; - } }