From c67238c5160e4aa16f97283367fae9a49803f4c6 Mon Sep 17 00:00:00 2001 From: maker Date: Sat, 9 Apr 2016 18:17:43 +0200 Subject: [PATCH] - Phalcon\Security is using now Phalcon\Security\Random - Enforced that Phalcon\Security::getToken() and Phalcon\Security::getTokenKey() return a random value per request not per call - Phalcon\Security::getToken() and Phalcon\Security::getTokenKey() are using now Phalcon\Security::_numberBytes instead of passed as a argument or hardcoded value - Phalcon\Security::hash() corrected not working CRYPT_STD_DES, CRYPT_EXT_DES, MD5, CRYPT_SHA256 - Phalcon\Security::hash() CRYPT_SHA512 fixed wrong salt length - Added missing unit-tests for Phalcon\Security --- CHANGELOG.md | 6 ++ phalcon/security.zep | 148 ++++++++++++++++----------- tests/unit/SecurityTest.php | 199 ++++++++++++++++++++++++++++++++++++ 3 files changed, 292 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2dd3cad784..6ec3365b5af 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,12 @@ - Default encrypt algorithm in `Phalcon\Crypt` is now changed to `AES-256-CFB` - Removed methods setMode(), getMode(), getAvailableModes() in `Phalcon\CryptInterface` - Added `Phalcon\Assets\Manager::exists()` to check if collection exists +- Phalcon\Security is using now Phalcon\Security\Random +- Enforced that Phalcon\Security::getToken() and Phalcon\Security::getTokenKey() return a random value per request not per call +- Phalcon\Security::getToken() and Phalcon\Security::getTokenKey() are using now Phalcon\Security::_numberBytes instead of passed as a argument or hardcoded value +- Phalcon\Security::hash() corrected not working CRYPT_STD_DES, CRYPT_EXT_DES, MD5, CRYPT_SHA256 +- Phalcon\Security::hash() CRYPT_SHA512 fixed wrong salt length +- Added missing unit-tests for Phalcon\Security # [2.0.11](https://github.com/phalcon/cphalcon/releases/tag/phalcon-v2.0.11) (????-??-??) - Fix Model magic set functionality to maintain variable visibility and utilize setter methods.[#11286](https://github.com/phalcon/cphalcon/issues/11286) diff --git a/phalcon/security.zep b/phalcon/security.zep index 5dece576474..bde4ab3fe57 100644 --- a/phalcon/security.zep +++ b/phalcon/security.zep @@ -20,6 +20,7 @@ namespace Phalcon; use Phalcon\DiInterface; +use Phalcon\Security\Random; use Phalcon\Security\Exception; use Phalcon\Di\InjectionAwareInterface; use Phalcon\Session\AdapterInterface as SessionInterface; @@ -54,7 +55,11 @@ class Security implements InjectionAwareInterface protected _tokenValueSessionID = "$PHALCON/CSRF$"; - protected _csrf; + protected _token; + + protected _tokenKey; + + protected _random; protected _defaultHash; @@ -78,6 +83,14 @@ class Security implements InjectionAwareInterface const CRYPT_SHA512 = 9; + /** + * Phalcon\Security constructor + */ + public function __construct() + { + let this->_random = new Random(); + } + /** * Sets the dependency injector */ @@ -96,10 +109,13 @@ class Security implements InjectionAwareInterface /** * Sets a number of bytes to be generated by the openssl pseudo random generator + * @return Phalcon\Security */ - public function setRandomBytes(long! randomBytes) -> void + public function setRandomBytes(long! randomBytes) -> { let this->_numberBytes = randomBytes; + + return this; } /** @@ -110,6 +126,15 @@ class Security implements InjectionAwareInterface return this->_numberBytes; } + /** + * Returns a secure random number generator instance + * @return Phalcon\Security\Random + */ + public function getRandom() -> + { + return this->_random; + } + /** * Generate a >22-length pseudo random string to be used as salt for passwords */ @@ -117,27 +142,14 @@ class Security implements InjectionAwareInterface { var safeBytes; - if !function_exists("openssl_random_pseudo_bytes") { - throw new Exception("Openssl extension must be loaded"); - } - if !numberBytes { let numberBytes = (int) this->_numberBytes; } loop { + let safeBytes = this->_random->base64Safe(numberBytes); - /** - * Produce random bytes using openssl - * Filter alpha numeric characters - */ - let safeBytes = phalcon_filter_alphanum(base64_encode(openssl_random_pseudo_bytes(numberBytes))); - - if !safeBytes { - continue; - } - - if strlen(safeBytes) < numberBytes { + if !safeBytes || strlen(safeBytes) < numberBytes { continue; } @@ -176,6 +188,10 @@ class Security implements InjectionAwareInterface let variant = "y"; break; + case self::CRYPT_MD5: + let variant = "1"; + break; + case self::CRYPT_SHA256: let variant = "5"; break; @@ -193,22 +209,38 @@ class Security implements InjectionAwareInterface 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". */ - let saltBytes = this->getSaltBytes(2); + if (hash == self::CRYPT_EXT_DES) { + let saltBytes = "_".this->getSaltBytes(8); + } else { + let saltBytes = this->getSaltBytes(2); + } + if typeof saltBytes != "string" { throw new Exception("Unable to get random bytes for the salt"); } - break; + return crypt(password, saltBytes); + case self::CRYPT_MD5: + case self::CRYPT_SHA256: case self::CRYPT_SHA512: - let saltBytes = this->getSaltBytes(8); + + /* + * 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 typeof saltBytes != "string" { throw new Exception("Unable to get random bytes for the salt"); } - return crypt(password, "$" . variant . "$" . saltBytes); + + return crypt(password, "$" . variant . "$" . saltBytes . "$"); case self::CRYPT_DEFAULT: case self::CRYPT_BLOWFISH: @@ -239,7 +271,7 @@ class Security implements InjectionAwareInterface } } - return crypt(password, "$2" . variant . "$" . sprintf("%02s", workFactor) . "$" . saltBytes); + return crypt(password, "$2" . variant . "$" . sprintf("%02s", workFactor) . "$" . saltBytes . "$"); } return ""; @@ -286,59 +318,45 @@ class Security implements InjectionAwareInterface /** * Generates a pseudo random token key to be used as input's name in a CSRF check */ - public function getTokenKey(int numberBytes = null) -> string + public function getTokenKey() -> string { - var safeBytes, dependencyInjector, session; - - if !numberBytes { - let numberBytes = 12; - } + var dependencyInjector, session; - if !function_exists("openssl_random_pseudo_bytes") { - throw new Exception("Openssl extension must be loaded"); - } + if null === this->_tokenKey { + let dependencyInjector = this->_dependencyInjector; + if typeof dependencyInjector != "object" { + throw new Exception("A dependency injection container is required to access the 'session' service"); + } - let dependencyInjector = this->_dependencyInjector; - if typeof dependencyInjector != "object" { - throw new Exception("A dependency injection container is required to access the 'session' service"); + let this->_tokenKey = this->_random->base64Safe(this->_numberBytes); + let session = dependencyInjector->getShared("session"); + session->set(this->_tokenKeySessionID, this->_tokenKey); } - let safeBytes = phalcon_filter_alphanum(base64_encode(openssl_random_pseudo_bytes(numberBytes))); - let session = dependencyInjector->getShared("session"); - session->set(this->_tokenKeySessionID, safeBytes); - - return safeBytes; + return this->_tokenKey; } /** * Generates a pseudo random token value to be used as input's value in a CSRF check */ - public function getToken(int numberBytes = null) -> string + public function getToken() -> string { - var token, dependencyInjector, session; - - if !numberBytes { - let numberBytes = 12; - } + var dependencyInjector, session; - if !function_exists("openssl_random_pseudo_bytes") { - throw new Exception("Openssl extension must be loaded"); - } + if null === this->_token { + let this->_token = this->_random->base64Safe(this->_numberBytes); - let token = openssl_random_pseudo_bytes(numberBytes); - let token = base64_encode(token); - let token = phalcon_filter_alphanum(token); + let dependencyInjector = this->_dependencyInjector; - let dependencyInjector = this->_dependencyInjector; + if typeof dependencyInjector != "object" { + throw new Exception("A dependency injection container is required to access the 'session' service"); + } - if typeof dependencyInjector != "object" { - throw new Exception("A dependency injection container is required to access the 'session' service"); + let session = dependencyInjector->getShared("session"); + session->set(this->_tokenValueSessionID, this->_token); } - let session = dependencyInjector->getShared("session"); - session->set(this->_tokenValueSessionID, token); - - return token; + return this->_token; } /** @@ -387,8 +405,7 @@ class Security implements InjectionAwareInterface * Remove the key and value of the CSRF token in session */ if returnValue && destroyIfValid { - session->remove(this->_tokenKeySessionID); - session->remove(this->_tokenValueSessionID); + this->destroyToken(); } return returnValue; @@ -408,13 +425,14 @@ class Security implements InjectionAwareInterface } let session = dependencyInjector->getShared("session"); + return session->get(this->_tokenValueSessionID); } /** * Removes the value of the CSRF token and key from session */ - public function destroyToken() + public function destroyToken() -> { var dependencyInjector, session; @@ -428,6 +446,11 @@ class Security implements InjectionAwareInterface session->remove(this->_tokenKeySessionID); session->remove(this->_tokenValueSessionID); + + let this->_token = null; + let this->_tokenKey = null; + + return this; } /** @@ -447,10 +470,13 @@ class Security implements InjectionAwareInterface /** * Sets the default hash + * @return Phalcon\Security */ public function setDefaultHash(int defaultHash) { let this->_defaultHash = defaultHash; + + return this; } /** diff --git a/tests/unit/SecurityTest.php b/tests/unit/SecurityTest.php index f4d330a6ed8..4639d1d7f1c 100644 --- a/tests/unit/SecurityTest.php +++ b/tests/unit/SecurityTest.php @@ -2,8 +2,11 @@ namespace Phalcon\Test\Unit; +use Phalcon\Di; use Phalcon\Test\Module\UnitTest; use Phalcon\Test\Proxy\Security; +use Phalcon\Test\Proxy\Http\Request; +use Codeception\Lib\Connector\PhalconMemorySession; /** * \Phalcon\Test\Unit\SecurityTest @@ -94,4 +97,200 @@ function () { } ); } + + /** + * Tests the security defaults + */ + public function testSecurityDefaults() + { + $this->specify( + "Security defaults are not correct", + function () { + $s = new Security(); + expect($s->getDefaultHash())->equals(null); + expect($s->getRandomBytes())->equals(16); + + $s->setDefaultHash(1); + expect($s->getDefaultHash())->equals(1); + + $s->setRandomBytes(22); + expect($s->getRandomBytes())->equals(22); + } + ); + } + + /** + * Tests getToken() and getTokenKey() for generating only one token per request + */ + public function testOneTokenPerRequest() + { + $this->specify( + "The getToken() and TokenKey() must return only one token per request", + function () { + $di = $this->setupDI(); + + $s = new Security(); + $s->setDI($di); + + $tokenKey = $s->getTokenKey(); + $token = $s->getToken(); + + expect($tokenKey)->equals($s->getTokenKey()); + expect($token)->equals($s->getToken()); + expect($token)->equals($s->getSessionToken()); + + $s->destroyToken(); + + expect($tokenKey)->notEquals($s->getTokenKey()); + expect($token)->notEquals($s->getToken()); + expect($token)->notEquals($s->getSessionToken()); + + $s->destroyToken(); + } + ); + } + + /** + * Tests checkToken() method + */ + public function testCheckToken() + { + $this->specify( + "The checkToken() not working correct", + function () { + $di = $this->setupDI(); + + $s = new Security(); + $s->setDI($di); + + // Random token and token key check + $tokenKey = $s->getTokenKey(); + $token = $s->getToken(); + + $_POST = array($tokenKey => $token); + + expect($s->checkToken(null, null, false))->true(); + expect($s->checkToken())->true(); + expect($s->checkToken())->false(); + + // Destroy token check + $tokenKey = $s->getTokenKey(); + $token = $s->getToken(); + + $s->destroyToken(); + + $_POST = array($tokenKey => $token); + + expect($s->checkToken())->false(); + + // Custom token key check + $token = $s->getToken(); + + $_POST = array('custom_key' => $token); + + expect($s->checkToken(null, null, false))->false(); + expect($s->checkToken('other_custom_key', null, false))->false(); + expect($s->checkToken('custom_key'))->true(); + + // Custom token value check + $token = $s->getToken(); + + $_POST = array(); + + expect($s->checkToken(null, null, false))->false(); + expect($s->checkToken('some_random_key', 'some_random_value', false))->false(); + expect($s->checkToken('custom_key', $token))->true(); + } + ); + } + + /** + * Tests getSaltBytes() method + */ + public function testGetSaltBytes() + { + $this->specify( + "The getSaltBytes() not working correct", + function () { + $s = new Security(); + + $salt = $s->getSaltBytes(); + + expect(strlen($salt))->greaterOrEquals(16); + + $salt = $s->getSaltBytes(22); + + expect(strlen($salt))->greaterOrEquals(22); + } + ); + } + + /** + * Tests password hash + */ + public function testHash() + { + $this->specify( + "The hash() not working correct", + function () { + $s = new Security(); + + $password = 'SomePasswordValue'; + + $s->setDefaultHash(Security::CRYPT_DEFAULT); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_STD_DES); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_EXT_DES); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_BLOWFISH); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_BLOWFISH_A); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_BLOWFISH_X); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_BLOWFISH_Y); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_SHA256); + expect($s->checkHash($password, $s->hash($password)))->true(); + + $s->setDefaultHash(Security::CRYPT_SHA512); + expect($s->checkHash($password, $s->hash($password)))->true(); + } + ); + } + + /** + * Sets the environment + */ + private function setupDI() + { + Di::reset(); + $di = new Di(); + + $di->set( + 'session', + function() { + return new PhalconMemorySession(); + }, + true + ); + + $di->set( + 'request', + function() { + return new Request(); + }, + true + ); + + return $di; + } }