- 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
phalcon · Apr 13, 2016 · 1a2a9e5 · 1a2a9e5
1 parent b024e3d
commit 1a2a9e5
Show file tree

Hide file tree

Showing 3 changed files with 292 additions and 61 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -66,6 +66,12 @@
 - `Phalcon\Mvc\Model\Manager::load()` now can load models from aliased namespaces
 - `Phalcon\Mvc\Model\Transaction\Manager` now correctly keeps account of transactions [#11554](https://github.com/phalcon/cphalcon/issues/11554)
 - `Phalcon\Db\Dialect\Sqlite` now maps additional column types to SQLite columns equivalents.
+- `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
@@ -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) -> <Security>
 	{
 		let this->_numberBytes = randomBytes;
+
+		return this;
 	}
 
 	/**
@@ -110,34 +126,30 @@ class Security implements InjectionAwareInterface
 		return this->_numberBytes;
 	}
 
+	/**
+	 * Returns a secure random number generator instance
+	 * @return Phalcon\Security\Random
+	 */	
+	public function getRandom() -> <Random>
+	{
+		return this->_random;
+	}
+
 	/**
 	 * Generate a >22-length pseudo random string to be used as salt for passwords
 	 */
 	public function getSaltBytes(int numberBytes = 0) -> string
 	{
 		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 = <DiInterface> this->_dependencyInjector;
+			if typeof dependencyInjector != "object" {
+				throw new Exception("A dependency injection container is required to access the 'session' service");
+			}
 
-		let dependencyInjector = <DiInterface> 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 = <SessionInterface> dependencyInjector->getShared("session");
+			session->set(this->_tokenKeySessionID, this->_tokenKey);
 		}
 
-		let safeBytes = phalcon_filter_alphanum(base64_encode(openssl_random_pseudo_bytes(numberBytes)));
-		let session = <SessionInterface> 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 = <DiInterface> this->_dependencyInjector;
 
-		let dependencyInjector = <DiInterface> 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 = <SessionInterface> dependencyInjector->getShared("session");
+			session->set(this->_tokenValueSessionID, this->_token);
 		}
 
-		let session = <SessionInterface> 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 = <SessionInterface> 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() -> <Security>
 	{
 		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;
 	}
 
 	/**