remove insecure rng providers and remove polyfill for hash_equals (#122)

* 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

* remove the isSecure property of the test rng class

* remove pointless test rng class

we were testing a test class, which didn't make a lot of sense.

* Revert "remove pointless test rng class"

This reverts commit f6da6bee6d.

* Reapply "remove pointless test rng class"

This reverts commit 06220d4a54.

* assing rng provider to class attribute

this also aligns with other providers

* remove polyfill for hash_equals
This commit is contained in:
Nicolas CARPi
2024-04-16 17:52:51 +02:00
committed by GitHub
parent 323053bd52
commit 194ecc28eb
12 changed files with 12 additions and 279 deletions
+5 -41
View File
@@ -4,12 +4,12 @@ declare(strict_types=1);
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;
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 +51,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
@@ -98,7 +95,7 @@ class TwoFactorAuth
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;
@@ -174,19 +171,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
@@ -195,27 +180,6 @@ class TwoFactorAuth
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();