From f57277aa96ce91a0e6761b04937447b9a2a9a679 Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Mon, 20 Nov 2017 20:17:06 +0100 Subject: [PATCH 1/2] Introduce PasswordHelper for safer passwords refs #2954 --- .../Icinga/Authentication/PasswordHelper.php | 109 ++++++++++++++++++ .../Authentication/PasswordHelperTest.php | 92 +++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 library/Icinga/Authentication/PasswordHelper.php create mode 100644 test/php/library/Icinga/Authentication/PasswordHelperTest.php diff --git a/library/Icinga/Authentication/PasswordHelper.php b/library/Icinga/Authentication/PasswordHelper.php new file mode 100644 index 000000000..0f5448b70 --- /dev/null +++ b/library/Icinga/Authentication/PasswordHelper.php @@ -0,0 +1,109 @@ +='); + } + + /** + * Hash a password with password_hash() or crypt() + * + * @param string $password + * @param int $algo + * + * @return string + * @throws AuthenticationException + */ + public static function hash($password, $algo = null) + { + if (static::supportsModernAPI() and $algo !== self::PASSWORD_ALGO_FALLBACK) { + if ($algo === null) { + $algo = PASSWORD_DEFAULT; + } + $p = password_hash($password, $algo); + if ($p === false) { + throw new AuthenticationException('Could not hash password, password_hash() returned false!'); + } + } else { + $p = crypt($password, self::COMPAT_HASH . static::generateSalt()); + if (strlen($p) < 13) { + throw new AuthenticationException('Hash generated by crypt() seems too small, this suggests an error!'); + } + } + + return $p; + } + + /** + * Verify a password with either password_verify() or crypt() + * + * @param string $password + * @param string $hash + * + * @return bool + */ + public static function verify($password, $hash) + { + if (static::supportsModernAPI()) { + return password_verify($password, $hash); + } else { + return crypt($password, $hash) === $hash; + } + } + + /** + * Shorthand to generate a salt to use with crypt() + * + * @return string + */ + public static function generateSalt() + { + return bin2hex(openssl_random_pseudo_bytes(self::COMPAT_SALT_LENGTH / 2)); + } +} diff --git a/test/php/library/Icinga/Authentication/PasswordHelperTest.php b/test/php/library/Icinga/Authentication/PasswordHelperTest.php new file mode 100644 index 000000000..7c97edda3 --- /dev/null +++ b/test/php/library/Icinga/Authentication/PasswordHelperTest.php @@ -0,0 +1,92 @@ +assertRegExp( + '~^[a-f0-9]{16}$~i', + PasswordHelper::generateSalt(), + 'A hex based salt with 16 chars must be returned' + ); + } + + public function testHash() + { + foreach (array(self::TEST_PASSWORD, self::TEST_PASSWORD_LONG) as $pw) { + $hashed = PasswordHelper::hash($pw); + + $this->assertRegExp( + '~^\$\d\w*\$(?:rounds=\d+\$)?~', + $hashed, + 'Hash output must look like a hash: ' . $hashed + ); + + $this->assertEquals( + crypt($pw, $hashed), + $hashed, + 'New hashed password must validate via crypt: ' . $hashed + ); + } + } + + public function testHashFallback() + { + $hashed = PasswordHelper::hash(self::TEST_PASSWORD, PasswordHelper::PASSWORD_ALGO_FALLBACK); + + $this->assertRegExp( + '~^\$6\$rounds=\d+\$?~', + $hashed, + 'Hash output must look like a SHA-512 hash: ' . $hashed + ); + + $this->assertEquals( + crypt(self::TEST_PASSWORD, $hashed), + $hashed, + 'New hashed password must validate via crypt: ' . $hashed + ); + } + + public function testVerify() + { + $pws = array( + self::TEST_PASSWORD_HASHED_BLOWFISH_1 => self::TEST_PASSWORD, + self::TEST_PASSWORD_HASHED_BLOWFISH_2 => self::TEST_PASSWORD, + self::TEST_PASSWORD_HASHED_BLOWFISH_LONG => self::TEST_PASSWORD_LONG, + self::TEST_PASSWORD_HASHED_SHA256 => self::TEST_PASSWORD, + pack('H*', self::TEST_PASSWORD_OLD_MD5) => self::TEST_PASSWORD, + ); + + foreach ($pws as $hash => $pw) { + $this->assertTrue( + PasswordHelper::verify($pw, $hash), + 'Password must be validated against its hash' + ); + } + } +} From 1aae1eab2318df12b0871c937615dfcdd81517b1 Mon Sep 17 00:00:00 2001 From: Markus Frosch Date: Mon, 20 Nov 2017 20:19:28 +0100 Subject: [PATCH 2/2] DBUserBackend: Replace internal crypt handling with PasswordHelper refs #2954 --- .../Authentication/User/DbUserBackend.php | 56 ++++--------------- 1 file changed, 11 insertions(+), 45 deletions(-) diff --git a/library/Icinga/Authentication/User/DbUserBackend.php b/library/Icinga/Authentication/User/DbUserBackend.php index d81319b60..6a2e2f646 100644 --- a/library/Icinga/Authentication/User/DbUserBackend.php +++ b/library/Icinga/Authentication/User/DbUserBackend.php @@ -4,30 +4,17 @@ namespace Icinga\Authentication\User; use Exception; +use Icinga\Authentication\PasswordHelper; use Icinga\Data\Inspectable; use Icinga\Data\Inspection; -use PDO; use Icinga\Data\Filter\Filter; use Icinga\Exception\AuthenticationException; use Icinga\Repository\DbRepository; use Icinga\User; +use PDO; class DbUserBackend extends DbRepository implements UserBackendInterface, Inspectable { - /** - * The algorithm to use when hashing passwords - * - * @var string - */ - const HASH_ALGORITHM = '$1$'; // MD5 - - /** - * The length of the salt to use when hashing a password - * - * @var int - */ - const SALT_LENGTH = 12; // 12 is required by MD5 - /** * The query columns being provided * @@ -125,8 +112,10 @@ class DbUserBackend extends DbRepository implements UserBackendInterface, Inspec /** * Insert a table row with the given data * - * @param string $table - * @param array $bind + * @param string $table + * @param array $bind + * + * @return void */ public function insert($table, array $bind) { @@ -177,7 +166,7 @@ class DbUserBackend extends DbRepository implements UserBackendInterface, Inspec */ protected function persistPassword($value) { - return $this->hashPassword($value); + return PasswordHelper::hash($value); } /** @@ -224,8 +213,10 @@ class DbUserBackend extends DbRepository implements UserBackendInterface, Inspec public function authenticate(User $user, $password) { try { - $passwordHash = $this->getPasswordHash($user->getUsername()); - return crypt($password, $passwordHash) === $passwordHash; + return PasswordHelper::verify( + $password, + $this->getPasswordHash($user->getUsername()) + ); } catch (Exception $e) { throw new AuthenticationException( 'Failed to authenticate user "%s" against backend "%s". An exception was thrown:', @@ -236,31 +227,6 @@ class DbUserBackend extends DbRepository implements UserBackendInterface, Inspec } } - /** - * Return a random salt - * - * The returned salt is safe to be used for hashing a user's password - * - * @return string - */ - protected function generateSalt() - { - return openssl_random_pseudo_bytes(self::SALT_LENGTH); - } - - /** - * Hash a password - * - * @param string $password - * @param string $salt - * - * @return string - */ - protected function hashPassword($password, $salt = null) - { - return crypt($password, self::HASH_ALGORITHM . ($salt !== null ? $salt : $this->generateSalt())); - } - /** * Inspect this object to gain extended information about its health *