From c11692c99c252bd18eeee1cf97cd23693bff5001 Mon Sep 17 00:00:00 2001 From: Nikolaos Dimopoulos Date: Fri, 5 Nov 2021 21:22:05 -0400 Subject: [PATCH 1/5] refactored hash to also use password_hash(); added getHashInformation() --- phalcon/Encryption/Crypt/PadFactory.zep.orig | 102 ---- phalcon/Encryption/Security.zep | 508 +++++++++++-------- 2 files changed, 305 insertions(+), 305 deletions(-) delete mode 100644 phalcon/Encryption/Crypt/PadFactory.zep.orig diff --git a/phalcon/Encryption/Crypt/PadFactory.zep.orig b/phalcon/Encryption/Crypt/PadFactory.zep.orig deleted file mode 100644 index 3d6d08cd683..00000000000 --- a/phalcon/Encryption/Crypt/PadFactory.zep.orig +++ /dev/null @@ -1,102 +0,0 @@ - -/** - * This file is part of the Phalcon Framework. - * - * (c) Phalcon Team - * - * For the full copyright and license information, please view the LICENSE.txt - * file that was distributed with this source code. - */ - -namespace Phalcon\Encryption\Crypt; - -use Phalcon\Encryption\Crypt; -use Phalcon\Encryption\Crypt\Padding\PadInterface; -use Phalcon\Factory\AbstractFactory; -use Phalcon\Helper\Arr; - -/** - * Class PadFactory - * - * @package Phalcon\Crypt - */ -class PadFactory extends AbstractFactory -{ - /** -<<<<<<< HEAD:phalcon/Crypt/PadFactory.zep -======= - * @var string - */ - protected exception = "Phalcon\\Encryption\\Crypt\\Exception\\Exception"; - - /** ->>>>>>> Moved Crypt and Security under encryption; adjusted tests:phalcon/Encryption/Crypt/PadFactory.zep - * AdapterFactory constructor. - */ - public function __construct(array! services = []) - { - this->init(services); - } - - /** - * Create a new instance of the adapter - */ - public function newInstance(string! name) -> - { - var definition; - - let definition = this->getService(name); - - return create_instance(definition); - } - - /** - * Gets a Crypt pad constant and returns the unique service name for the - * padding class - * - * @param int $number - * - * @return string - */ - public function padNumberToService(int number) -> string - { - array map; - - let map = [ - Crypt::PADDING_ANSI_X_923 : "ansi", - Crypt::PADDING_ISO_10126 : "iso10126", - Crypt::PADDING_ISO_IEC_7816_4 : "isoiek", - Crypt::PADDING_PKCS7 : "pjcs7", - Crypt::PADDING_SPACE : "space", - Crypt::PADDING_ZERO : "zero" - ]; - - return Arr::get(map, number, "noop"); - } - - /** - * @return string - */ - protected function getExceptionClass() -> string - { - return "Phalcon\\Crypt\\Exception\\Exception"; - } - - /** - * Returns the available adapters - * - * @return string[] - */ - protected function getServices() -> array - { - return [ - "ansi" : "Phalcon\\Encryption\\Crypt\\Padding\\Ansi", - "iso10126" : "Phalcon\\Encryption\\Crypt\\Padding\\Iso10126", - "isoiek" : "Phalcon\\Encryption\\Crypt\\Padding\\IsoIek", - "noop" : "Phalcon\\Encryption\\Crypt\\Padding\\Noop", - "pjcs7" : "Phalcon\\Encryption\\Crypt\\Padding\\Pkcs7", - "space" : "Phalcon\\Encryption\\Crypt\\Padding\\Space", - "zero" : "Phalcon\\Encryption\\Crypt\\Padding\\Zero" - ]; - } -} diff --git a/phalcon/Encryption/Security.zep b/phalcon/Encryption/Security.zep index 297f539cce6..a7c59bbc98d 100644 --- a/phalcon/Encryption/Security.zep +++ b/phalcon/Encryption/Security.zep @@ -36,6 +36,9 @@ use Phalcon\Session\ManagerInterface as SessionInterface; */ class Security extends AbstractInjectionAware { + const CRYPT_ARGON2I = 10; + const CRYPT_ARGON2ID = 11; + const CRYPT_BCRYPT = 0; const CRYPT_DEFAULT = 0; const CRYPT_BLOWFISH = 4; const CRYPT_BLOWFISH_A = 5; @@ -48,9 +51,9 @@ class Security extends AbstractInjectionAware const CRYPT_STD_DES = 1; /** - * @var int|null + * @var int */ - protected defaultHash = null; + protected defaultHash = self::CRYPT_DEFAULT; /** * @var int @@ -103,10 +106,15 @@ class Security extends AbstractInjectionAware private localRequest = null; /** - * Phalcon\Security constructor + * Security constructor. + * + * @param SessionInterface|null $session + * @param RequestInterface|null $request */ - public function __construct( session = null, request = null) - { + public function __construct( + session = null, + request = null + ) { let this->random = new Random(), this->localRequest = request, this->localSession = session; @@ -115,48 +123,43 @@ class Security extends AbstractInjectionAware /** * Checks a plain text password and its hash version to check if the * password matches + * + * @param string $password + * @param string $passwordHash + * @param int $maxPassLength + * + * @return bool */ - public function checkHash(string password, string passwordHash, int maxPassLength = 0) -> bool - { - char ch; - string cryptedHash; - int i, sum, cryptedLength, passwordLength; - + public function checkHash( + string password, + string passwordHash, + int maxPassLength = 0 + ) -> bool { if maxPassLength > 0 && strlen(password) > maxPassLength { return false; } - let cryptedHash = (string) crypt(password, passwordHash); - - let cryptedLength = strlen(cryptedHash), - passwordLength = strlen(passwordHash); - - let cryptedHash .= passwordHash; - - let sum = cryptedLength - passwordLength; - - for i, ch in passwordHash { - let sum = sum | (cryptedHash[i] ^ ch); - } - - return 0 === sum; + return password_verify(password, passwordHash); } /** * Check if the CSRF token sent in the request is the same that the current * in session + * + * @param string|null $tokenKey + * @param mixed|null $tokenValue + * @param bool $destroyIfValid + * + * @return bool */ - public function checkToken(var tokenKey = null, var tokenValue = null, bool destroyIfValid = true) -> bool - { - var session, request, equals, userToken, knownToken; + public function checkToken( + string tokenKey = null, + var tokenValue = null, + bool destroyIfValid = true + ) -> bool { + var equals, knownToken, userToken; - let session = this->getLocalSession(); - - if likely session && !tokenKey { - let tokenKey = session->get( - this->tokenKeySessionId - ); - } + let tokenKey = this->processTokenKey(tokenKey); /** * If tokenKey does not exist in session return false @@ -165,23 +168,12 @@ class Security extends AbstractInjectionAware return false; } - if !tokenValue { - let request = this->getLocalRequest(); - - /** - * We always check if the value is correct in post - */ - let userToken = request->getPost(tokenKey, "string"); - } else { - let userToken = tokenValue; - } - /** * The value is the same? */ - let knownToken = this->getRequestToken(); - - if null === knownToken { + let userToken = this->processUserToken(tokenKey, tokenValue), + knownToken = this->getRequestToken(); + if (null === knownToken || null === userToken) { return false; } @@ -199,9 +191,21 @@ class Security extends AbstractInjectionAware /** * Computes a HMAC + * + * @param string $data + * @param string $key + * @param string $algo + * @param bool $raw + * + * @return string + * @throws Exception */ - public function computeHmac(string data, string key, string algo, bool raw = false) -> string - { + public function computeHmac( + string data, + string key, + string algo, + bool raw = false + ) -> string { var hmac; let hmac = hash_hmac(algo, data, key, raw); @@ -225,7 +229,7 @@ class Security extends AbstractInjectionAware { var session; - let session = this->getLocalSession(); + let session = this->getLocalService("session", "localSession"); if likely session { session->remove(this->tokenKeySessionId); @@ -240,13 +244,27 @@ class Security extends AbstractInjectionAware } /** - * Returns the default hash - */ - public function getDefaultHash() -> int | null + * Returns the default hash + * + * @return int + */ + public function getDefaultHash() -> int { return this->defaultHash; } + /** + * Returns information regarding a hash + * + * @param string $hash + * + * @return array + */ + public function getHashInformation(string hash) -> array + { + return password_get_info(hash); + } + /** * Returns a secure random number generator instance */ @@ -278,12 +296,14 @@ class Security extends AbstractInjectionAware /** * Returns the value of the CSRF token in session + * + * @return string|null */ public function getSessionToken() -> string | null { var session; - let session = this->getLocalSession(); + let session = this->getLocalService("session", "localSession"); if likely session { return session->get(this->tokenValueSessionId); @@ -295,13 +315,18 @@ class Security extends AbstractInjectionAware /** * Generate a >22-length pseudo random string to be used as salt for * passwords + * + * @param int $numberBytes + * + * @return string + * @throws Exception */ public function getSaltBytes(int numberBytes = 0) -> string { var safeBytes; if !numberBytes { - let numberBytes = (int) this->numberBytes; + let numberBytes = this->numberBytes; } loop { @@ -318,19 +343,21 @@ class Security extends AbstractInjectionAware /** * Generates a pseudo random token value to be used as input's value in a * CSRF check + * + * @return string + * @throws Exception */ - public function getToken() -> string + public function getToken() -> string | null { var session; - if null === this->token { + if (null === this->token) { let this->requestToken = this->getSessionToken(), this->token = this->random->base64Safe(this->numberBytes); - - let session = this->getLocalSession(); - - if likely session { + /** @var SessionInterface|null $session */ + let session = this->getLocalService("session", "localSession"); + if (null !== session) { session->set( this->tokenValueSessionId, this->token @@ -344,15 +371,18 @@ class Security extends AbstractInjectionAware /** * Generates a pseudo random token key to be used as input's name in a CSRF * check + * + * @return string|null + * @throws Exception */ - public function getTokenKey() -> string + public function getTokenKey() -> string | null { var session; - if null === this->tokenKey { - let session = this->getLocalSession(); - - if likely session { + if (null === this->tokenKey) { + /** @var SessionInterface|null $session */ + let session = this->getLocalService("session", "localSession"); + if (null !== session) { let this->tokenKey = this->random->base64Safe(this->numberBytes); session->set( this->tokenKeySessionId, @@ -366,138 +396,93 @@ class Security extends AbstractInjectionAware /** * Creates a password hash using bcrypt with a pseudo random salt + * + * @param string $password + * @param array $options + * + * @return string */ - public function hash(string password, int workFactor = 0) -> string + public function hash(string password, array options = []) -> string { - int hash; - string variant; - var saltBytes; - - if !workFactor { - let workFactor = (int) this->workFactor; - } + var algorithm, arguments, cost, formatted, prefix, salt; + bool legacy; + int bytes; - let hash = (int) this->defaultHash; - - switch hash { - - case self::CRYPT_BLOWFISH_A: - let variant = "a"; - break; - - case self::CRYPT_BLOWFISH_X: - let variant = "x"; - break; - - case self::CRYPT_BLOWFISH_Y: - let variant = "y"; - break; + /** + * The `legacy` variable distinguishes between `password_hash` and + * non `password_hash` hashing. + */ + let cost = this->processCost(options), + formatted = sprintf("%02s", cost), + prefix = "", + bytes = 22, + legacy = true; + switch (this->defaultHash) { case self::CRYPT_MD5: - let variant = "1"; + /* + * MD5 hashing with a twelve character salt + * SHA-256/SHA-512 hash with a sixteen character salt. + */ + let prefix = "$1$", + bytes = 12; break; - case self::CRYPT_SHA256: - let variant = "5"; + let prefix = "$5$", + bytes = 16; break; - case self::CRYPT_SHA512: - let variant = "6"; + let prefix = "$6$", + bytes = 16; break; - - case self::CRYPT_DEFAULT: - default: - let variant = "y"; + /* + * Blowfish hashing with a salt as follows: "$2a$", "$2x$" or + * "$2y$", a two digit cost parameter, "$", and 22 characters + * from the alphabet "./0-9A-Za-z". Using characters outside + * this range in the salt will cause `crypt()` to return a + * zero-length string. The two digit cost parameter is the + * base-2 logarithm of the iteration count for the underlying + * Blowfish-based hashing algorithm and must be in range 04-31, + * values outside this range will cause crypt() to fail. + */ + case self::CRYPT_BLOWFISH_A: + let prefix = sprintf("$2a$%s$", formatted); break; - } - - switch hash { - - case self::CRYPT_STD_DES: - case self::CRYPT_EXT_DES: - - /** - * Standard DES-based hash with a two character salt from the - * alphabet "./0-9A-Za-z". - */ - - if hash == self::CRYPT_EXT_DES { - let saltBytes = "_" . this->getSaltBytes(8); - } else { - let saltBytes = this->getSaltBytes(2); - } - - if unlikely typeof saltBytes != "string" { - throw new Exception( - "Unable to get random bytes for the salt" - ); - } - - return crypt(password, saltBytes); - - case self::CRYPT_MD5: - case self::CRYPT_SHA256: - case self::CRYPT_SHA512: - - /** - * MD5 hashing with a twelve character salt - * SHA-256/SHA-512 hash with a sixteen character salt. - */ - - let saltBytes = this->getSaltBytes(hash == self::CRYPT_MD5 ? 12 : 16); - - if unlikely typeof saltBytes != "string" { - throw new Exception( - "Unable to get random bytes for the salt" - ); - } - - return crypt( - password, "$" . variant . "$" . saltBytes . "$" - ); - - case self::CRYPT_DEFAULT: - case self::CRYPT_BLOWFISH: case self::CRYPT_BLOWFISH_X: - case self::CRYPT_BLOWFISH_Y: + let prefix = sprintf("$2x$%s$", formatted); + break; default: + let legacy = false; + break; + } - /** - * Blowfish hashing with a salt as follows: "$2a$", "$2x$" or - * "$2y$", a two digit cost parameter, "$", and 22 characters - * from the alphabet "./0-9A-Za-z". Using characters outside of - * this range in the salt will cause `crypt()` to return a - * zero-length string. The two digit cost parameter is the - * base-2 logarithm of the iteration count for the underlying - * Blowfish-based hashing algorithm and must be in range 04-31, - * values outside this range will cause crypt() to fail. - */ - - let saltBytes = this->getSaltBytes(22); + if legacy { + let salt = prefix . this->getSaltBytes(bytes) . "$"; - if unlikely typeof saltBytes != "string" { - throw new Exception( - "Unable to get random bytes for the salt" - ); - } + return crypt(password, salt); + } - if workFactor < 4 { - let workFactor = 4; - } elseif workFactor > 31 { - let workFactor = 31; - } + /** + * This is using password_hash + * + * We will not provide a "salt" but let PHP calculate it. + */ + let options = [ + "cost" : cost + ]; - return crypt( - password, - "$2" . variant . "$" . sprintf("%02s", workFactor) . "$" . saltBytes . "$" - ); - } + let algorithm = this->processAlgorithm(), + arguments = this->processArgonOptions(options); - return ""; + return password_hash(password, algorithm, arguments); } /** * Checks if a password hash is a valid bcrypt's hash + * + * @param string $passwordHash + * + * @return bool */ public function isLegacyHash(string passwordHash) -> bool { @@ -506,6 +491,10 @@ class Security extends AbstractInjectionAware /** * Sets the default hash + * + * @param int $defaultHash + * + * @return Security */ public function setDefaultHash(int defaultHash) -> { @@ -517,6 +506,10 @@ class Security extends AbstractInjectionAware /** * Sets a number of bytes to be generated by the openssl pseudo random * generator + * + * @param int $randomBytes + * + * @return Security */ public function setRandomBytes(int! randomBytes) -> { @@ -527,6 +520,10 @@ class Security extends AbstractInjectionAware /** * Sets the work factor + * + * @param int $workFactor + * + * @return $this */ public function setWorkFactor(int workFactor) -> { @@ -535,47 +532,152 @@ class Security extends AbstractInjectionAware return this; } - private function getLocalRequest() -> | null + /** + * @param string $name + * @param string $property + * + * @return RequestInterface|SessionInterface|null + */ + protected function getLocalService(string name, string property) { - var container; - - if this->localRequest { - return this->localRequest; + if ( + null === this->{property} && + null !== this->container && + true === this->container->has(name) + ) { + let this->{property} = this->container->getShared(name); } - let container = this->container; - if unlikely typeof container != "object" { - throw new Exception( - Exception::containerServiceNotFound("the 'request' service") - ); + return this->{property}; + } + + /** + * Checks the algorithm for `password_hash`. If it is argon based, it + * returns the relevant constant + * + * @return string + */ + private function processAlgorithm() -> string + { + var algorithm; + + let algorithm = PASSWORD_BCRYPT; + + if (this->defaultHash === self::CRYPT_ARGON2I) { + let algorithm = PASSWORD_ARGON2I; + } elseif (this->defaultHash === self::CRYPT_ARGON2ID) { + let algorithm = PASSWORD_ARGON2ID; } - if likely container->has("request") { - return container->getShared("request"); + return algorithm; + } + + /** + * We check if the algorithm is Argon based. If yes, options are set for + * `password_hash` such as `memory_cost`, `time_cost` and `threads` + * + * @param array $options + * + * @return array + */ + private function processArgonOptions(array options) -> array + { + var value; + + if ( + this->defaultHash === self::CRYPT_ARGON2I || + this->defaultHash === self::CRYPT_ARGON2ID + ) { + if !fetch value, options["memory_cost"] { + let value = PASSWORD_ARGON2_DEFAULT_MEMORY_COST; + } + let options["memory_cost"] = value; + + if !fetch value, options["time_cost"] { + let value = PASSWORD_ARGON2_DEFAULT_TIME_COST; + } + let options["time_cost"] = value; + + if !fetch value, options["threads"] { + let value = PASSWORD_ARGON2_DEFAULT_THREADS; + } + let options["threads"] = value; } - return null; + return options; } - private function getLocalSession() -> | null + /** + * Checks the options array for `cost`. If not defined it is set to 10. + * It also checks the cost if it is between 4 and 31 + * + * @param array $options + * + * @return int + */ + private function processCost(array options = []) -> int { - var container; + var cost; - if this->localSession { - return this->localSession; + if !fetch cost, options["cost"] { + let cost = 10; } - let container = this->container; - if unlikely typeof container != "object" { - throw new Exception( - Exception::containerServiceNotFound("the 'session' service") - ); + if cost < 4 { + let cost = 4; } - if likely container->has("session") { - return container->getShared("session"); + if cost > 31 { + let cost = 31; } - return null; + return cost; + } + + /** + * @param string|null $tokenKey + * + * @return string|null + */ + private function processTokenKey(string tokenKey = null) -> string | null + { + var key, session; + + let key = tokenKey, + session = this->getLocalService("session", "localSession"); + if (null !== session && true === empty(key)) { + let key = session->get(this->tokenKeySessionId); + } + + return key; + } + + /** + * @param string $tokenKey + * @param string|null $tokenValue + * + * @return string|null + */ + private function processUserToken( + string tokenKey, + string tokenValue = null + ) -> string | null { + var request, userToken; + + let userToken = tokenValue; + if !tokenValue { + /** @var RequestInterface|null $request */ + let request = this->getLocalService("request", "localRequest"); + + /** + * We always check if the value is correct in post + */ + if (null !== request) { + /** @var string|null $userToken */ + let userToken = request->getPost(tokenKey, "string"); + } + } + + return userToken; } } From 280c1af1dbf9d4fd6c2bc18b766fb69dabd3b88d Mon Sep 17 00:00:00 2001 From: Nikolaos Dimopoulos Date: Fri, 5 Nov 2021 21:22:13 -0400 Subject: [PATCH 2/5] added more tests --- .../Encryption/Security/CheckHashCest.php | 18 +- .../Encryption/Security/CheckTokenCest.php | 84 +++++++- .../Security/GetHashInformationCest.php | 194 ++++++++++++++++++ .../Security/GetSetWorkFactorCest.php | 7 +- .../Encryption/Security/IsLegacyHashCest.php | 7 +- 5 files changed, 297 insertions(+), 13 deletions(-) create mode 100644 tests/unit/Encryption/Security/GetHashInformationCest.php diff --git a/tests/unit/Encryption/Security/CheckHashCest.php b/tests/unit/Encryption/Security/CheckHashCest.php index d8473d1dcba..2901116da0a 100644 --- a/tests/unit/Encryption/Security/CheckHashCest.php +++ b/tests/unit/Encryption/Security/CheckHashCest.php @@ -73,13 +73,17 @@ public function securityCheckHashFalse(UnitTester $I) private function getExamples(): array { return [ - ['CRYPT_DEFAULT', Security::CRYPT_DEFAULT], - ['CRYPT_BLOWFISH', Security::CRYPT_BLOWFISH], - ['CRYPT_BLOWFISH_A', Security::CRYPT_BLOWFISH_A], - ['CRYPT_BLOWFISH_X', Security::CRYPT_BLOWFISH_X], - ['CRYPT_BLOWFISH_Y', Security::CRYPT_BLOWFISH_Y], - ['CRYPT_SHA256', Security::CRYPT_SHA256], - ['CRYPT_SHA512', Security::CRYPT_SHA512], + ["CRYPT_ARGON2I", Security::CRYPT_ARGON2I], + ["CRYPT_ARGON2ID", Security::CRYPT_ARGON2ID], + ["CRYPT_BCRYPT", Security::CRYPT_BCRYPT], + ["CRYPT_DEFAULT", Security::CRYPT_DEFAULT], + ["CRYPT_BLOWFISH", Security::CRYPT_BLOWFISH], + ["CRYPT_BLOWFISH_A", Security::CRYPT_BLOWFISH_A], + ["CRYPT_BLOWFISH_X", Security::CRYPT_BLOWFISH_X], + ["CRYPT_BLOWFISH_Y", Security::CRYPT_BLOWFISH_Y], + ["CRYPT_MD5", Security::CRYPT_MD5], + ["CRYPT_SHA256", Security::CRYPT_SHA256], + ["CRYPT_SHA512", Security::CRYPT_SHA512], ]; } } diff --git a/tests/unit/Encryption/Security/CheckTokenCest.php b/tests/unit/Encryption/Security/CheckTokenCest.php index 7d851f52a92..b42f0066ecb 100644 --- a/tests/unit/Encryption/Security/CheckTokenCest.php +++ b/tests/unit/Encryption/Security/CheckTokenCest.php @@ -49,21 +49,101 @@ public function _before(UnitTester $I) public function securityCheckToken(UnitTester $I) { $I->wantToTest('Security - checkToken()'); - $I->skipTest('TODO: Enable when Request is done'); - $store = $_POST ?? []; + $tokenSessionId = '$PHALCON/CSRF/KEY$'; + $tokenValueSessionId = '$PHALCON/CSRF$'; /** @var Manager $session */ $session = $this->container->getShared('session'); + $session->start(); + + /** + * Just in case + */ + if (true === $session->has($tokenSessionId)) { + $session->remove($tokenSessionId); + } + if (true === $session->has($tokenValueSessionId)) { + $session->remove($tokenValueSessionId); + } + + $security = new Security(); + /** + * No session - checkToken returns empty + */ + $actual = $security->checkToken(); + $I->assertFalse($actual); + + $actual = $security->getTokenKey(); + $I->assertNull($actual); + + /** + * Enable the Session + */ + $security->setDI($this->container); + + // Random token and token key check + $token = $security->getToken(); + + $actual = $session->has($tokenValueSessionId); + $I->assertTrue($actual); + + $expected = $token; + $actual = $session->get($tokenValueSessionId); + $I->assertEquals($expected, $actual); + + $session->destroy(); + } + + /** + * Tests Phalcon\Security :: checkToken() and destroyToken() with Request + * + * @param UnitTester $I + * + * @author Phalcon Team + * @since 2020-09-09 + */ + public function securityCheckTokenWithRequest(UnitTester $I) + { + $I->wantToTest('Security - checkToken() - with Request'); + $I->skipTest("Enable when Request is ready"); + + /** @var Manager $session */ + $session = $this->container->getShared('session'); $session->start(); $security = new Security(); + + /** + * No session - checkToken returns empty + */ + $actual = $security->checkToken(); + $I->assertFalse($actual); + + $actual = $security->getTokenKey(); + $I->assertNull($actual); + + $actual = $security->getToken(); + $I->assertNull($actual); + + /** + * Enable the Session + */ $security->setDI($this->container); // Random token and token key check $tokenKey = $security->getTokenKey(); $token = $security->getToken(); + $actual = $session->has('$PHALCON/CSRF/KEY$'); + $I->assertTrue($actual); + + $session->destroy(); + + /** + * @todo When Request is done, enable the below + */ + $_POST = [ $tokenKey => $token, ]; diff --git a/tests/unit/Encryption/Security/GetHashInformationCest.php b/tests/unit/Encryption/Security/GetHashInformationCest.php new file mode 100644 index 00000000000..9240bc9ec3a --- /dev/null +++ b/tests/unit/Encryption/Security/GetHashInformationCest.php @@ -0,0 +1,194 @@ + + * + * For the full copyright and license information, please view the LICENSE.txt + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Phalcon\Tests\Unit\Encryption\Security; + +use Codeception\Example; +use Phalcon\Encryption\Security; +use UnitTester; + +use const PASSWORD_ARGON2_DEFAULT_MEMORY_COST; +use const PASSWORD_ARGON2_DEFAULT_THREADS; +use const PASSWORD_ARGON2_DEFAULT_TIME_COST; + +class GetHashInformationCest +{ + /** + * Tests Phalcon\Security :: getHashInformation() + * + * @dataProvider getExamples + * + * @param UnitTester $I + * @param Example $example + * + * @author Phalcon Team + * @since 2020-09-09 + */ + public function securityGetHashInformation(UnitTester $I, Example $example) + { + $I->wantToTest('Security - getHashInformation() ' . $example[0]); + + $security = new Security(); + $password = 'PhalconROCKS!'; + + $security->setDefaultHash($example[1]); + + $hash = $security->hash($password); + $expected = $example[2]; + $actual = $security->getHashInformation($hash); + $I->assertEquals($expected, $actual); + } + + /** + * Tests Phalcon\Security :: checkHash() - false + * + * @param UnitTester $I + * + * @author Phalcon Team + * @since 2020-09-09 + */ + public function securityCheckHashFalse(UnitTester $I) + { + $I->wantToTest('Security - checkHash() - false'); + + $security = new Security(); + $password = 'PhalconROCKS!'; + + $actual = $security->checkHash($password, $password, 2); + $I->assertFalse($actual); + } + + /** + * @return array[] + */ + private function getExamples(): array + { + return [ + [ + "CRYPT_ARGON2I", + Security::CRYPT_ARGON2I, + [ + "algo" => "argon2i", + "algoName" => "argon2i", + "options" => [ + "memory_cost" => PASSWORD_ARGON2_DEFAULT_MEMORY_COST, + "time_cost" => PASSWORD_ARGON2_DEFAULT_TIME_COST, + "threads" => PASSWORD_ARGON2_DEFAULT_THREADS, + ], + ], + ], + [ + "CRYPT_ARGON2ID", + Security::CRYPT_ARGON2ID, + [ + "algo" => "argon2id", + "algoName" => "argon2id", + "options" => [ + "memory_cost" => PASSWORD_ARGON2_DEFAULT_MEMORY_COST, + "time_cost" => PASSWORD_ARGON2_DEFAULT_TIME_COST, + "threads" => PASSWORD_ARGON2_DEFAULT_THREADS, + ], + ], + ], + [ + "CRYPT_BCRYPT", + Security::CRYPT_BCRYPT, + [ + "algo" => "2y", + "algoName" => "bcrypt", + "options" => [ + "cost" => 10, + ], + ], + ], + [ + "CRYPT_DEFAULT", + Security::CRYPT_DEFAULT, + [ + "algo" => "2y", + "algoName" => "bcrypt", + "options" => [ + "cost" => 10, + ], + ], + ], + [ + "CRYPT_BLOWFISH", + Security::CRYPT_BLOWFISH, + [ + "algo" => "2y", + "algoName" => "bcrypt", + "options" => [ + "cost" => 10, + ], + ], + ], + [ + "CRYPT_BLOWFISH_A", + Security::CRYPT_BLOWFISH_A, + [ + "algo" => null, + "algoName" => "unknown", + "options" => [], + ], + ], + [ + "CRYPT_BLOWFISH_X", + Security::CRYPT_BLOWFISH_X, + [ + "algo" => null, + "algoName" => "unknown", + "options" => [], + ], + ], + [ + "CRYPT_BLOWFISH_Y", + Security::CRYPT_BLOWFISH_Y, + [ + "algo" => "2y", + "algoName" => "bcrypt", + "options" => [ + "cost" => 10, + ], + ], + ], + [ + "CRYPT_MD5", + Security::CRYPT_MD5, + [ + "algo" => null, + "algoName" => "unknown", + "options" => [], + ], + ], + [ + "CRYPT_SHA256", + Security::CRYPT_SHA256, + [ + "algo" => null, + "algoName" => "unknown", + "options" => [], + ], + ], + [ + "CRYPT_SHA512", + Security::CRYPT_SHA512, + [ + "algo" => null, + "algoName" => "unknown", + "options" => [], + ], + ], + ]; + } +} diff --git a/tests/unit/Encryption/Security/GetSetWorkFactorCest.php b/tests/unit/Encryption/Security/GetSetWorkFactorCest.php index d6dfd760e3f..54a5c1e9971 100644 --- a/tests/unit/Encryption/Security/GetSetWorkFactorCest.php +++ b/tests/unit/Encryption/Security/GetSetWorkFactorCest.php @@ -32,11 +32,14 @@ public function securityGetSetWorkFactor(UnitTester $I) $security = new Security(); - $I->assertEquals(10, $security->getWorkFactor()); + $expected = 10; + $actual = $security->getWorkFactor(); + $I->assertEquals($expected, $actual); $expected = 31; $security->setWorkFactor($expected); - $I->assertEquals($expected, $security->getWorkFactor()); + $actual = $security->getWorkFactor(); + $I->assertEquals($expected, $actual); } } diff --git a/tests/unit/Encryption/Security/IsLegacyHashCest.php b/tests/unit/Encryption/Security/IsLegacyHashCest.php index 67e68763ce9..8180ddaadde 100644 --- a/tests/unit/Encryption/Security/IsLegacyHashCest.php +++ b/tests/unit/Encryption/Security/IsLegacyHashCest.php @@ -38,7 +38,10 @@ public function securityIsLegacyHash(UnitTester $I) $oldHash = '$2a$10$JnD9Za73U2dIIjd.Uvn1IuNVQhXNQpHIu13WzlL70q.WhfKT9Yuc2'; $security = new Security(); - $I->assertTrue($security->isLegacyHash($oldHash)); - $I->assertFalse($security->isLegacyHash('Phalcon')); + $actual = $security->isLegacyHash($oldHash); + $I->assertTrue($actual); + + $actual = $security->isLegacyHash('Phalcon'); + $I->assertFalse($actual); } } From 886794150f4cc513556307756027865262163118 Mon Sep 17 00:00:00 2001 From: Nikolaos Dimopoulos Date: Fri, 5 Nov 2021 21:24:24 -0400 Subject: [PATCH 3/5] updated changelog --- CHANGELOG-5.0.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md index 28339ae5c97..ee5af84ad49 100644 --- a/CHANGELOG-5.0.md +++ b/CHANGELOG-5.0.md @@ -70,6 +70,7 @@ - `escapeHtmlAttr()` becomes `attributes()` - `escapeUrl()` becomes `url()` - `setHtmlQuoteType()` becomes `setFlags()` [#15757](https://github.com/phalcon/cphalcon/issues/15757) +- Changed `Phalcon\Encryption\Security::hash()` to also use `password_hash()` and accept `ARGON2*` algorithms [#15731](https://github.com/phalcon/cphalcon/issues/15731) ## Added - Added more tests in the suite for additional code coverage [#15691](https://github.com/phalcon/cphalcon/issues/15691) @@ -89,6 +90,7 @@ - Added more tests increasing coverage [#15717](https://github.com/phalcon/cphalcon/issues/15717) - Added `Phalcon\Cache\Adapter\*::setForever()` and `Phalcon\Storage\Adapter\*::setForever()` to allow storing a key forever [#15485](https://github.com/phalcon/cphalcon/issues/15485) - Added `Phalcon\Forms\Form::getFilteredValue()` to get filtered value without providing entity [#15438](https://github.com/phalcon/cphalcon/issues/15438) +- Added `Phalcon\Encryption\Security::getHashInformation()` to return information for a hash [#15731](https://github.com/phalcon/cphalcon/issues/15731) ## Fixed - Fixed `Query::getExpression()` return type [#15553](https://github.com/phalcon/cphalcon/issues/15553) From 12686dc7f82f3ef7d89f2d5ce21f8958996ea12c Mon Sep 17 00:00:00 2001 From: Nikolaos Dimopoulos Date: Fri, 5 Nov 2021 21:26:18 -0400 Subject: [PATCH 4/5] updating changelog again --- CHANGELOG-5.0.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md index ee5af84ad49..5e6e4fc46cc 100644 --- a/CHANGELOG-5.0.md +++ b/CHANGELOG-5.0.md @@ -91,6 +91,7 @@ - Added `Phalcon\Cache\Adapter\*::setForever()` and `Phalcon\Storage\Adapter\*::setForever()` to allow storing a key forever [#15485](https://github.com/phalcon/cphalcon/issues/15485) - Added `Phalcon\Forms\Form::getFilteredValue()` to get filtered value without providing entity [#15438](https://github.com/phalcon/cphalcon/issues/15438) - Added `Phalcon\Encryption\Security::getHashInformation()` to return information for a hash [#15731](https://github.com/phalcon/cphalcon/issues/15731) +- Added constants `Phalcon\Encryption\Security::CRYPT_ARGON2I` and `Phalcon\Encryption\Security::CRYPT_ARGON2ID` [#15731](https://github.com/phalcon/cphalcon/issues/15731) ## Fixed - Fixed `Query::getExpression()` return type [#15553](https://github.com/phalcon/cphalcon/issues/15553) From 5c7338c700f90908534cf1212aed3fc8f61ce5d9 Mon Sep 17 00:00:00 2001 From: Nikolaos Dimopoulos Date: Fri, 5 Nov 2021 21:34:45 -0400 Subject: [PATCH 5/5] psalm correction --- phalcon/Encryption/Security.zep | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phalcon/Encryption/Security.zep b/phalcon/Encryption/Security.zep index a7c59bbc98d..b88531fea11 100644 --- a/phalcon/Encryption/Security.zep +++ b/phalcon/Encryption/Security.zep @@ -456,7 +456,7 @@ class Security extends AbstractInjectionAware break; } - if legacy { + if unlikely legacy { let salt = prefix . this->getSaltBytes(bytes) . "$"; return crypt(password, salt); @@ -523,7 +523,7 @@ class Security extends AbstractInjectionAware * * @param int $workFactor * - * @return $this + * @return Security */ public function setWorkFactor(int workFactor) -> {