Merge branch '1.1.16-branch'

Conflicts:
	CHANGELOG
	UPGRADE
	framework/YiiBase.php
	framework/yiilite.php

CHANGELOG

@@ -62,6 +62,7 @@ Version 1.1.16 under development
 - Bug: Fixed the bug that backslashes are not escaped by CDbCommandBuilder::buildSearchCondition() (qiangxue)
 - Bug: Fixed URL parsing so it's now properly giving 404 for URLs like "http://example.com//////site/about/////" (samdark)
 - Bug: Fixed an issue with CFilehelper and not accessable directories which resulted in endless loop (cebe)
+- Bug: CSecurityManager encryption and string comparison were enhanced (sarciszewski, Jan Ewald, tom--, ircmaxell, qiangxue, samdark)
 - Enh: Public method CFileHelper::createDirectory() has been added (klimov-paul)
 - Enh: Added proper handling and support of the symlinked directories in CFileHelper::removeDirectory(), added $options parameter in CFileHelper::removeDirectory() (resurtm)
 - Enh #89: Support for SOAP headers in WSDL generator (nineinchnick)

UPGRADE

@@ -43,6 +43,18 @@ Upgrading from v1.1.15
   There were breaking changes in the jQuery API which you can find in the jQuery blog:
   <http://jquery.com/upgrade-guide/1.9/#changes-of-note-in-jquery-1-9>
 
+- Use CSecurityManager::legacyDecrypt() and CSecurityManager::encrypt() to convert existing encrypted data if any.
+
+- If there are exceptions about encryption key length you need to:
+
+  1. Decrypt all the data.
+  2. Change key via application config (encryptionKey property of securityManager component) to the one that conforms
+  to recommendations.
+  3. Encrypt all the data.
+
+  You can disable key validation by setting validateEncryptionKey property of securityManager component to false but
+  if it strongly not recommended.
+
 Upgrading from v1.1.14
 ----------------------
 

framework/base/CSecurityManager.php

@@ -48,6 +48,20 @@ class CSecurityManager extends CApplicationComponent
 	const STATE_VALIDATION_KEY='Yii.CSecurityManager.validationkey';
 	const STATE_ENCRYPTION_KEY='Yii.CSecurityManager.encryptionkey';
 
+	/**
+	 * @var array known minimum lengths per encryption algorithm
+	 */
+	protected static $encryptionKeyMinimumLengths=array(
+		'blowfish'=>4,
+		'arcfour'=>5,
+		'rc2'=>5,
+	);
+
+	/**
+	 * @var boolean if encryption key should be validated
+	 */
+	public $validateEncryptionKey=true;
+
 	/**
 	 * @var string the name of the hashing algorithm to be used by {@link computeHMAC}.
 	 * See {@link http://php.net/manual/en/function.hash-algos.php hash-algos} for the list of possible
@@ -62,12 +76,20 @@ class CSecurityManager extends CApplicationComponent
 	 * This will be passed as the first parameter to {@link http://php.net/manual/en/function.mcrypt-module-open.php mcrypt_module_open}.
 	 *
 	 * This property can also be configured as an array. In this case, the array elements will be passed in order
-	 * as parameters to mcrypt_module_open. For example, <code>array('rijndael-256', '', 'ofb', '')</code>.
+	 * as parameters to mcrypt_module_open. For example, <code>array('rijndael-128', '', 'ofb', '')</code>.
+	 *
+	 * Defaults to AES
+	 *
+	 * Note: MCRYPT_RIJNDAEL_192 and MCRYPT_RIJNDAEL_256 are *not* AES-192 and AES-256. The numbers of the MCRYPT_RIJNDAEL
+	 * constants refer to the block size, whereas the numbers of the AES variants refer to the key length. AES is Rijndael
+	 * with a block size of 128 bits and a key length of 128 bits, 192 bits or 256 bits. So to use AES in Mcrypt, you need
+	 * MCRYPT_RIJNDAEL_128 and a key with 16 bytes (AES-128), 24 bytes (AES-192) or 32 bytes (AES-256). The other two
+	 * Rijndael variants in Mcrypt should be avoided, because they're not standardized and have been analyzed much less
+	 * than AES.
 	 *
-	 * Defaults to 'des', meaning using DES crypt algorithm.
 	 * @since 1.1.3
 	 */
-	public $cryptAlgorithm='des';
+	public $cryptAlgorithm='rijndael-128';
 
 	private $_validationKey;
 	private $_encryptionKey;
@@ -158,10 +180,8 @@ class CSecurityManager extends CApplicationComponent
 	 */
 	public function setEncryptionKey($value)
 	{
-		if(!empty($value))
-			$this->_encryptionKey=$value;
-		else
-			throw new CException(Yii::t('yii','CSecurityManager.encryptionKey cannot be empty.'));
+		$this->validateEncryptionKey($value);
+		$this->_encryptionKey=$value;
 	}
 
 	/**
@@ -191,12 +211,14 @@ class CSecurityManager extends CApplicationComponent
 	 * @param string $data data to be encrypted.
 	 * @param string $key the decryption key. This defaults to null, meaning using {@link getEncryptionKey EncryptionKey}.
 	 * @return string the encrypted data
-	 * @throws CException if PHP Mcrypt extension is not loaded
+	 * @throws CException if PHP Mcrypt extension is not loaded or key is invalid
 	 */
 	public function encrypt($data,$key=null)
 	{
+		if($key===null)
+			$key=$this->getEncryptionKey();
+		$this->validateEncryptionKey($key);
 		$module=$this->openCryptModule();
-		$key=$this->substr($key===null ? md5($this->getEncryptionKey()) : $key,0,mcrypt_enc_get_key_size($module));
 		srand();
 		$iv=mcrypt_create_iv(mcrypt_enc_get_iv_size($module), MCRYPT_RAND);
 		mcrypt_generic_init($module,$key,$iv);
@@ -211,12 +233,14 @@ class CSecurityManager extends CApplicationComponent
 	 * @param string $data data to be decrypted.
 	 * @param string $key the decryption key. This defaults to null, meaning using {@link getEncryptionKey EncryptionKey}.
 	 * @return string the decrypted data
-	 * @throws CException if PHP Mcrypt extension is not loaded
+	 * @throws CException if PHP Mcrypt extension is not loaded or key is invalid
 	 */
 	public function decrypt($data,$key=null)
 	{
+		if($key===null)
+			$key=$this->getEncryptionKey();
+		$this->validateEncryptionKey($key);
 		$module=$this->openCryptModule();
-		$key=$this->substr($key===null ? md5($this->getEncryptionKey()) : $key,0,mcrypt_enc_get_key_size($module));
 		$ivSize=mcrypt_enc_get_iv_size($module);
 		$iv=$this->substr($data,0,$ivSize);
 		mcrypt_generic_init($module,$key,$iv);
@@ -279,7 +303,7 @@ class CSecurityManager extends CApplicationComponent
 		{
 			$hmac=$this->substr($data,0,$len);
 			$data2=$this->substr($data,$len,$this->strlen($data));
-			return $hmac===$this->computeHMAC($data2,$key)?$data2:false;
+			return $this->compareString($hmac,$this->computeHMAC($data2,$key))?$data2:false;
 		}
 		else
 			return false;
@@ -492,4 +516,97 @@ class CSecurityManager extends CApplicationComponent
 	{
 		return $this->_mbstring ? mb_substr($string,$start,$length,'8bit') : substr($string,$start,$length);
 	}
+    
+	/**
+	 * Checks if a key is valid for {@link cryptAlgorithm}.
+	 * @param string $key the key to check
+	 * @return boolean the validation result
+	 * @throws CException if the supported key lengths of the cipher are unknown
+	 */
+	protected function validateEncryptionKey($key)
+	{
+		if(is_string($key))
+		{
+			$supportedKeyLengths=mcrypt_module_get_supported_key_sizes($this->cryptAlgorithm);
+
+			if($supportedKeyLengths)
+			{
+				if(!in_array($this->strlen($key),$supportedKeyLengths)) {
+					throw new CException(Yii::t('yii','Encryption key length can be '.implode(',',$supportedKeyLengths).'.'));
+				}
+			}
+			elseif(isset(self::$encryptionKeyMinimumLengths[$this->cryptAlgorithm]))
+			{
+				$minLength=self::$encryptionKeyMinimumLengths[$this->cryptAlgorithm];
+				$maxLength=mcrypt_module_get_algo_key_size($this->cryptAlgorithm);
+				if($this->strlen($key)<$minLength || $this->strlen($key)>$maxLength)
+					throw new CException(Yii::t('yii','Encryption key length must be between '.$minLength.' and '.$maxLength.'.'));
+			}
+			else
+				throw new CException(Yii::t('yii','Failed to validate key. Supported key lengths of cipher not known.'));
+		}
+		else
+			throw new CException(Yii::t('yii','Encryption key should be a string.'));
+	}
+    
+	/**
+	 * Decrypts legacy ciphertext which was produced by the old, broken implementation of encrypt().
+	 * @deprecated use only to convert data encrypted prior to 1.1.16
+	 * @param string $data data to be decrypted.
+	 * @param string $key the decryption key. This defaults to null, meaning the key should be loaded from persistent storage.
+	 * @param string|array $cipher the algorithm to be used
+	 * @return string the decrypted data
+	 * @throws CException if PHP Mcrypt extension is not loaded
+	 * @throws CException if the key is missing
+	 */
+	public function legacyDecrypt($data,$key=null,$cipher='des')
+	{
+		if (!$key)
+		{
+			$key=Yii::app()->getGlobalState(self::STATE_ENCRYPTION_KEY);
+			if(!$key)
+				throw new CException(Yii::t('yii','No encryption key specified.'));
+		}
+
+		if(extension_loaded('mcrypt'))
+		{
+			if(is_array($cipher))
+				$module=@call_user_func_array('mcrypt_module_open',$cipher);
+			else
+				$module=@mcrypt_module_open($cipher,'', MCRYPT_MODE_CBC,'');
+
+			if($module===false)
+				throw new CException(Yii::t('yii','Failed to initialize the mcrypt module.'));
+		}
+		else
+			throw new CException(Yii::t('yii','CSecurityManager requires PHP mcrypt extension to be loaded in order to use data encryption feature.'));
+
+		$derivedKey=$this->substr(md5($key),0,mcrypt_enc_get_key_size($module));
+		$ivSize=mcrypt_enc_get_iv_size($module);
+		$iv=$this->substr($data,0,$ivSize);
+		mcrypt_generic_init($module,$derivedKey,$iv);
+		$decrypted=mdecrypt_generic($module,$this->substr($data,$ivSize,$this->strlen($data)));
+		mcrypt_generic_deinit($module);
+		mcrypt_module_close($module);
+		return rtrim($decrypted,"\0");
+	}
+
+	/**
+	 * Performs string comparison using timing attack resistant approach.
+	 * @see http://codereview.stackexchange.com/questions/13512
+	 * @param string $expected string to compare.
+	 * @param string $actual user-supplied string.
+	 * @return boolean whether strings are equal.
+	 */
+	public function compareString($expected,$actual)
+	{
+		$expected.="\0";
+		$actual.="\0";
+		$expectedLength=$this->strlen($expected);
+		$actualLength=$this->strlen($actual);
+		$diff=$expectedLength-$actualLength;
+		for($i=0;$i<$actualLength;$i++)
+			$diff|=(ord($actual[$i])^ord($expected[$i%$expectedLength]));
+		return $diff===0;
+	}
 }

tests/framework/base/CSecurityManagerTest.php

@@ -26,15 +26,50 @@ class CSecurityManagerTest extends CTestCase
 	public function testEncryptionKey()
 	{
 		$sm=new CSecurityManager;
-		$key='123456';
-		$sm->encryptionKey=$key;
-		$this->assertEquals($key,$sm->encryptionKey);
+		$sm->cryptAlgorithm='des';
+		$key="\xA5\x94\x72\x26\x1F\xA3\x8A\x5B";
+		$sm->setEncryptionKey($key);
+		$this->assertEquals($key,$sm->getEncryptionKey());
+	}
 
-		$app=new TestApplication;
-		$key=$app->securityManager->encryptionKey;
-		$app->saveGlobalState();
-		$app2=new TestApplication;
-		$this->assertEquals($app2->securityManager->encryptionKey,$key);
+	/**
+	 * @expectedException CException
+	 */
+	public function testUndersizedGlobalKey()
+	{
+		$sm=new CSecurityManager;
+		$sm->cryptAlgorithm='des';
+		$sm->setEncryptionKey('1');
+	}
+
+	/**
+	 * @expectedException CException
+	 */
+	public function testUndersizedKey()
+	{
+		$sm=new CSecurityManager;
+		$sm->cryptAlgorithm='des';
+		$sm->encrypt('some data', '1');
+	}
+
+	/**
+	 * @expectedException CException
+	 */
+	public function testOversizedGlobalKey()
+	{
+		$sm=new CSecurityManager;
+		$sm->cryptAlgorithm='des';
+		$sm->setEncryptionKey('123456789');
+	}
+
+	/**
+	 * @expectedException CException
+	 */
+	public function testOversizedKey()
+	{
+		$sm=new CSecurityManager;
+		$sm->cryptAlgorithm='des';
+		$sm->encrypt('some data', '123456789');
 	}
 
 	public function testValidation()
@@ -69,7 +104,8 @@ class CSecurityManagerTest extends CTestCase
 		if(!extension_loaded('mcrypt'))
 			$this->markTestSkipped('mcrypt extension is required to test encrypt feature.');
 		$sm=new CSecurityManager;
-		$sm->encryptionKey='123456';
+		$sm->cryptAlgorithm='des';
+		$sm->setEncryptionKey("\xAF\x84\x8F\xF2\xEE\x92\xDF\xA8");
 		$data='this is raw data';
 		$encryptedData=$sm->encrypt($data);
 		$this->assertTrue($data!==$encryptedData);
@@ -174,4 +210,42 @@ class CSecurityManagerTest extends CTestCase
 			$this->assertEquals($i, $mbStrlen ? mb_strlen($ran, '8bit') : strlen($ran));
 		}
 	}
+
+	public function dataProviderCompareStrings()
+	{
+		return array(
+			array("",""),
+			array(false,""),
+			array(null,""),
+			array(0,""),
+			array(0.00,""),
+			array("",null),
+			array("",false),
+			array("",0),
+			array("","\0"),
+			array("\0",""),
+			array("\0","\0"),
+			array("0","\0"),
+			array(0,"\0"),
+			array("user","User"),
+			array("password","password"),
+			array("password","passwordpassword"),
+			array("password1","password"),
+			array("password","password2"),
+			array("","password"),
+			array("password",""),
+		);
+	}
+
+	/**
+	 * @dataProvider dataProviderCompareStrings
+	 *
+	 * @param $expected
+	 * @param $actual
+	 */
+	public function testCompareStrings($expected, $actual)
+	{
+		$sm=new CSecurityManager;
+		$this->assertEquals(strcmp($expected,$actual)===0,$sm->compareString($expected,$actual));
+	}
 }