Merge pull request #1464 from markstory/3.0-security-improvements

3.0 security improvements Remove insecure features and backwards compatibility shims.
cakephp · Aug 3, 2013 · a8d5052 · a8d5052
2 parents 9b00278 + 3ba064a
commit a8d5052
Show file tree

Hide file tree

Showing 12 changed files with 37 additions and 197 deletions.
diff --git a/App/Config/app.php b/App/Config/app.php
@@ -76,13 +76,10 @@
  * The level of CakePHP security.
  *
  * - salt - A random string used in security hashing methods.
- * - cipherSeed - A random numeric string (digits only) used to seed 
- *   the xor cipher functions in Security.
+ *   The salt value is also used as the encryption key. You should treat it
+ *   as extremely sensitive data.
  */
-	Configure::write('Security', [
-		'salt' => 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi',
-		'cipherSeed' => '76859309657453542496749683645',
-	]);
+	Configure::write('Security.salt', 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi');
 
 /**
  * Apply timestamps with the last modified time to static assets (js, css, images).

diff --git a/lib/Cake/Console/Command/Task/ProjectTask.php b/lib/Cake/Console/Command/Task/ProjectTask.php
@@ -98,13 +98,6 @@ public function execute() {
 				$success = false;
 			}
 
-			if ($this->securityCipherSeed($path) === true) {
-				$this->out(__d('cake_console', ' * Random seed created for \'Security.cipherSeed\''));
-			} else {
-				$this->err(__d('cake_console', 'Unable to generate random seed for \'Security.cipherSeed\', you should change it in %s', APP . 'Config' . DS . 'app.php'));
-				$success = false;
-			}
-
 			if ($this->cachePrefix($path)) {
 				$this->out(__d('cake_console', ' * Cache prefix set'));
 			} else {
@@ -287,7 +280,7 @@ public function securitySalt($path) {
 		$contents = $File->read();
 		$newSalt = Security::generateAuthKey();
 		$contents = preg_replace(
-			"/^(\s+'salt'\s+\=\>\s+')([^']+)(',)/m",
+			"/('Security.salt',\s+')([^']+)(')/m",
 			'${1}' . $newSalt . '\\3',
 			$contents,
 			-1,
@@ -299,29 +292,6 @@ public function securitySalt($path) {
 		return false;
 	}
 
-/**
- * Generates and writes 'Security.cipherSeed'
- *
- * @param string $path Project path
- * @return boolean Success
- */
-	public function securityCipherSeed($path) {
-		$File = new File($path . 'Config/app.php');
-		$contents = $File->read();
-		$newCipher = substr(bin2hex(Security::generateAuthKey()), 0, 30);
-		$contents = preg_replace(
-			"/^(\s+'cipherSeed'\s+\=\>\s+')([^']+)(',)/m",
-			'${1}' . $newCipher . '\\3',
-			$contents,
-			-1,
-			$count
-		);
-		if ($count && $File->write($contents)) {
-			return true;
-		}
-		return false;
-	}
-
 /**
  * Writes cache prefix using app's name
  *

diff --git a/lib/Cake/Console/Templates/skel/Config/app.php b/lib/Cake/Console/Templates/skel/Config/app.php
@@ -76,13 +76,10 @@
  * The level of CakePHP security.
  *
  * - salt - A random string used in security hashing methods.
- * - cipherSeed - A random numeric string (digits only) used to seed
- *   the xor cipher functions in Security.
+ *   The salt value is also used as the encryption key. You should treat it
+ *   as extremely sensitive data.
  */
-	Configure::write('Security', [
-		'salt' => 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi',
-		'cipherSeed' => '76859309657453542496749683645',
-	]);
+	Configure::write('Security.salt', 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi');
 
 /**
  * Apply timestamps with the last modified time to static assets (js, css, images).

diff --git a/lib/Cake/Console/Templates/skel/Config/core.php b/lib/Cake/Console/Templates/skel/Config/core.php
@@ -199,11 +199,6 @@
  */
 	Configure::write('Security.salt', 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi');
 
-/**
- * A random numeric string (digits only) used to encrypt/decrypt strings.
- */
-	Configure::write('Security.cipherSeed', '76859309657453542496749683645');
-
 /**
  * Apply timestamps with the last modified time to static assets (js, css, images).
  * Will append a query string parameter containing the time the file was modified. This is

diff --git a/lib/Cake/Controller/Component/CookieComponent.php b/lib/Cake/Controller/Component/CookieComponent.php
@@ -18,6 +18,7 @@
 use Cake\Controller\ComponentRegistry;
 use Cake\Controller\Controller;
 use Cake\Core\Configure;
+use Cake\Error;
 use Cake\Event\Event;
 use Cake\Network\Request;
 use Cake\Network\Response;
@@ -132,12 +133,11 @@ class CookieComponent extends Component {
 /**
  * Type of encryption to use.
  *
- * Currently two methods are available: cipher and rijndael
- * Defaults to Security::cipher();
+ * Defaults to Security::rijndael();
  *
  * @var string
  */
-	protected $_type = 'cipher';
+	protected $_type = 'rijndael';
 
 /**
  * Used to reset cookie time if $expire is passed to CookieComponent::write()
@@ -375,15 +375,14 @@ public function destroy() {
  *
  * @param string $type Encryption method
  * @return void
+ * @throws Cake\Error\Exception When an unknown type is used.
  */
-	public function type($type = 'cipher') {
-		$availableTypes = array(
-			'cipher',
+	public function type($type = 'rijndael') {
+		$availableTypes = [
 			'rijndael'
-		);
+		];
 		if (!in_array($type, $availableTypes)) {
-			trigger_error(__d('cake_dev', 'You must use cipher or rijndael for cookie encryption type'), E_USER_WARNING);
-			$type = 'cipher';
+			throw new Error\Exception(__d('cake_dev', 'You must use rijndael for cookie encryption type'));
 		}
 		$this->_type = $type;
 	}

diff --git a/lib/Cake/Test/TestCase/Console/Command/Task/ProjectTaskTest.php b/lib/Cake/Test/TestCase/Console/Command/Task/ProjectTaskTest.php
@@ -248,23 +248,6 @@ public function testSecuritySaltGeneration() {
 		$this->assertNotRegExp('/DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi/', $contents, 'Default Salt left behind. %s');
 	}
 
-/**
- * test generation of Security.cipherSeed
- *
- * @return void
- */
-	public function testSecurityCipherSeedGeneration() {
-		$this->_setupTestProject();
-
-		$path = $this->Task->path . 'BakeTestApp/';
-		$result = $this->Task->securityCipherSeed($path);
-		$this->assertTrue($result);
-
-		$File = new File($path . 'Config/app.php');
-		$contents = $File->read();
-		$this->assertNotRegExp('/76859309657453542496749683645/', $contents, 'Default CipherSeed left behind. %s');
-	}
-
 /**
  * test generation of cache prefix
  *

diff --git a/lib/Cake/Test/TestCase/Controller/Component/AuthComponentTest.php b/lib/Cake/Test/TestCase/Controller/Component/AuthComponentTest.php
@@ -72,7 +72,6 @@ public function setUp() {
 		$this->markTestIncomplete('Need to revisit once models work again.');
 
 		Configure::write('Security.salt', 'YJfIxfs2guVoUubWDYhG93b0qyJfIxfs2guwvniR2G0FgaC9mi');
-		Configure::write('Security.cipherSeed', 770011223369876);
 		Configure::write('App.namespace', 'TestApp');
 
 		$request = new Request();

diff --git a/lib/Cake/Test/TestCase/Controller/Component/CookieComponentTest.php b/lib/Cake/Test/TestCase/Controller/Component/CookieComponentTest.php
@@ -50,7 +50,7 @@ public function setUp() {
 		$this->Cookie->path = '/';
 		$this->Cookie->domain = '';
 		$this->Cookie->secure = false;
-		$this->Cookie->key = 'somerandomhaskey';
+		$this->Cookie->key = 'somerandomhaskeysomerandomhaskey';
 
 		$event = new Event('Controller.startup', $this->Controller);
 		$this->Cookie->startup($event);
@@ -647,7 +647,7 @@ protected function _encrypt($value) {
 		if (is_array($value)) {
 			$value = $this->_implode($value);
 		}
-		return "Q2FrZQ==." . base64_encode(Security::cipher($value, $this->Cookie->key));
+		return "Q2FrZQ==." . base64_encode(Security::rijndael($value, $this->Cookie->key, 'encrypt'));
 	}
 
 }
diff --git a/lib/Cake/Test/TestCase/Core/ObjectTest.php b/lib/Cake/Test/TestCase/Core/ObjectTest.php
@@ -198,7 +198,6 @@ public function setUp() {
 		$this->object = new TestObject();
 		Configure::write('App.namespace', 'TestApp');
 		Configure::write('Security.salt', 'not-the-default');
-		Configure::write('Security.cipherSeed', '123456');
 		Log::disable('stdout');
 		Log::disable('stderr');
 	}

diff --git a/lib/Cake/Test/TestCase/Utility/SecurityTest.php b/lib/Cake/Test/TestCase/Utility/SecurityTest.php
@@ -70,7 +70,7 @@ public function testValidateAuthKey() {
 /**
  * testHashInvalidSalt method
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testHashInvalidSalt() {
@@ -80,7 +80,7 @@ public function testHashInvalidSalt() {
 /**
  * testHashAnotherInvalidSalt
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testHashAnotherInvalidSalt() {
@@ -90,7 +90,7 @@ public function testHashAnotherInvalidSalt() {
 /**
  * testHashYetAnotherInvalidSalt
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testHashYetAnotherInvalidSalt() {
@@ -100,7 +100,7 @@ public function testHashYetAnotherInvalidSalt() {
 /**
  * testHashInvalidCost method
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testHashInvalidCost() {
@@ -198,49 +198,6 @@ public function testHashBlowfish() {
 		Security::setHash($_hashType);
 	}
 
-/**
- * testCipher method
- *
- * @return void
- */
-	public function testCipher() {
-		$length = 10;
-		$txt = '';
-		for ($i = 0; $i < $length; $i++) {
-			$txt .= mt_rand(0, 255);
-		}
-		$key = 'my_key';
-		$result = Security::cipher($txt, $key);
-		$this->assertEquals($txt, Security::cipher($result, $key));
-
-		$txt = '';
-		$key = 'my_key';
-		$result = Security::cipher($txt, $key);
-		$this->assertEquals($txt, Security::cipher($result, $key));
-
-		$txt = 123456;
-		$key = 'my_key';
-		$result = Security::cipher($txt, $key);
-		$this->assertEquals($txt, Security::cipher($result, $key));
-
-		$txt = '123456';
-		$key = 'my_key';
-		$result = Security::cipher($txt, $key);
-		$this->assertEquals($txt, Security::cipher($result, $key));
-	}
-
-/**
- * testCipherEmptyKey method
- *
- * @expectedException PHPUnit_Framework_Error
- * @return void
- */
-	public function testCipherEmptyKey() {
-		$txt = 'some_text';
-		$key = '';
-		Security::cipher($txt, $key);
-	}
-
 /**
  * testRijndael method
  *
@@ -265,26 +222,10 @@ public function testRijndael() {
 		$this->assertEquals($txt, Security::rijndael($result, $key, 'decrypt'));
 	}
 
-/**
- * Test that rijndael() can still decrypt values with a fixed iv.
- *
- * @return void
- */
-	public function testRijndaelBackwardCompatibility() {
-		$this->skipIf(!function_exists('mcrypt_encrypt'));
-
-		$txt = 'The quick brown fox jumped over the lazy dog.';
-		$key = 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi';
-
-		// Encrypted before random iv
-		$value = base64_decode('1WPjnq96LMzLGwNgmudHF+cAIqVUN5DaUZEpf5tm1EzSgt5iYY9o3d66iRI/fKJLTlTVGsa8HzW0jDNitmVXoQ==');
-		$this->assertEquals($txt, Security::rijndael($value, $key, 'decrypt'));
-	}
-
 /**
  * testRijndaelInvalidOperation method
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testRijndaelInvalidOperation() {
@@ -296,7 +237,7 @@ public function testRijndaelInvalidOperation() {
 /**
  * testRijndaelInvalidKey method
  *
- * @expectedException PHPUnit_Framework_Error
+ * @expectedException Cake\Error\Exception
  * @return void
  */
 	public function testRijndaelInvalidKey() {

diff --git a/lib/Cake/Utility/Debugger.php b/lib/Cake/Utility/Debugger.php
@@ -850,10 +850,6 @@ public static function checkSecurityKeys() {
 		if (Configure::read('Security.salt') === 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi') {
 			trigger_error(__d('cake_dev', 'Please change the value of \'Security.salt\' in App/Config/app.php to a salt value specific to your application'), E_USER_NOTICE);
 		}
-
-		if (Configure::read('Security.cipherSeed') === '76859309657453542496749683645') {
-			trigger_error(__d('cake_dev', 'Please change the value of \'Security.cipherSeed\' in app/Config/app.php to a numeric (digits only) seed value specific to your application'), E_USER_NOTICE);
-		}
 	}
 
 }