Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

opmtimizations #853

Closed
wants to merge 4 commits into from

3 participants

@ceeram
Collaborator

Optimizing Security class

lib/Cake/Utility/Security.php
@@ -101,17 +101,15 @@ public static function hash($string, $type = null, $salt = false) {
return self::_crypt($string, $type, $salt);
}
if ($salt) {
- if (is_string($salt)) {
- $string = $salt . $string;
- } else {
- $string = Configure::read('Security.salt') . $string;
+ if (!is_string($salt)) {
+ $salt = Configure::read('Security.salt') . $string;
@ADmad Collaborator
ADmad added a note

Shouldn't this be $salt = Configure::read('Security.salt'); ?

@ADmad Collaborator
ADmad added a note

Nevermind, I misread.

@ADmad Collaborator
ADmad added a note

Doh I was right the first time, diffs can be confusing sometimes. With your patch if $salt is true the lines executed will be

$salt = Configure::read('Security.salt') . $string;
$string = $salt . $string;

and $string will effectively become $string = Configure::read('Security.salt') . $string . $string;

@ceeram Collaborator
ceeram added a note

right, missed that, thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ADmad
Collaborator

Can you please add some tests too for the case I highlighted? If your previous patch did not cause any failing tests it means the test cases are inadequate.

@markstory
Owner

I think this change looks pretty good.

@ceeram
Collaborator

i think can do some more, as new _crypt() method is only used with blowfish, so the $type param doesnt do anything here and should be removed imo

@ceeram
Collaborator

What is your opinion about _crypt(), it accepts $type parameter, but its currently only used with blowfish, also the $options are hardcoded within the method, and no way to change those, i think the method can be simplified for blowfish only use, or expanded somehow to be able to be used with other types.

I think its a blowfosh only helper method, so it can be reduced to only handle that

@markstory
Owner

@ceeram I think simplifying is the best option for now. Until we decide to add support for more cipher types, we shouldn't take on any complexity we don't need.

@ceeram
Collaborator

removed unneeded complexity now as well

@markstory
Owner

@Ceeram This looks good to me :beers:

@ceeram ceeram closed this
@ceeram
Collaborator

Merged as single commit: 0196c6f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
13 lib/Cake/Test/Case/Utility/SecurityTest.php
@@ -100,7 +100,6 @@ public function testHashYetAnotherInvalidSalt() {
*/
public function testHashInvalidCost() {
Security::setCost(1000);
- $result = Security::hash('somekey', 'blowfish', false);
}
/**
* testHash method
@@ -179,6 +178,18 @@ public function testHashBlowfish() {
$hashedPassword = Security::hash($submittedPassword, null, $storedPassword);
$this->assertNotSame($storedPassword, $hashedPassword);
+ $expected = sha1('customsaltsomevalue');
+ $result = Security::hash('somevalue', 'sha1', 'customsalt');
+ $this->assertSame($expected, $result);
+
+ $oldSalt = Configure::read('Security.salt');
+ Configure::write('Security.salt', 'customsalt');
+
+ $expected = sha1('customsaltsomevalue');
+ $result = Security::hash('somevalue', 'sha1', true);
+ $this->assertSame($expected, $result);
+
+ Configure::write('Security.salt', $oldSalt);
Security::setHash($_hashType);
}
View
77 lib/Cake/Utility/Security.php
@@ -98,20 +98,18 @@ public static function hash($string, $type = null, $salt = false) {
$type = strtolower($type);
if ($type === 'blowfish') {
- return self::_crypt($string, $type, $salt);
+ return self::_crypt($string, $salt);
}
if ($salt) {
- if (is_string($salt)) {
- $string = $salt . $string;
- } else {
- $string = Configure::read('Security.salt') . $string;
+ if (!is_string($salt)) {
+ $salt = Configure::read('Security.salt');
}
+ $string = $salt . $string;
}
if (!$type || $type == 'sha1') {
if (function_exists('sha1')) {
- $return = sha1($string);
- return $return;
+ return sha1($string);
}
$type = 'sha256';
}
@@ -145,6 +143,14 @@ public static function setHash($hash) {
* @return void
*/
public static function setCost($cost) {
+ if ($cost < 4 || $cost > 31) {
+ trigger_error(__d(
+ 'cake_dev',
+ 'Invalid value, cost must be between %s and %s',
+ array(4, 31)
+ ), E_USER_WARNING);
+ return null;
+ }
self::$hashCost = $cost;
}
@@ -201,13 +207,11 @@ public static function rijndael($text, $key, $operation) {
$mode = 'cbc';
$cryptKey = substr($key, 0, 32);
$iv = substr($key, strlen($key) - 32, 32);
- $out = '';
+
if ($operation === 'encrypt') {
- $out .= mcrypt_encrypt($algorithm, $cryptKey, $text, $mode, $iv);
- } elseif ($operation === 'decrypt') {
- $out .= rtrim(mcrypt_decrypt($algorithm, $cryptKey, $text, $mode, $iv), "\0");
+ return mcrypt_encrypt($algorithm, $cryptKey, $text, $mode, $iv);
}
- return $out;
+ return rtrim(mcrypt_decrypt($algorithm, $cryptKey, $text, $mode, $iv), "\0");
}
/**
@@ -228,55 +232,22 @@ public static function salt($length = 22) {
}
/**
- * One way encryption using php's crypt() function.
+ * One way encryption using php's crypt() function, used with type blowfish in ``Security::hash()``
*
* @param string $password The string to be encrypted.
- * @param string $type The encryption method to use (blowfish)
* @param mixed $salt false to generate a new salt or an existing salt.
*/
- protected static function _crypt($password, $type = null, $salt = false) {
- $options = array(
- 'saltFormat' => array(
- 'blowfish' => '$2a$%02d$%s',
- ),
- 'saltLength' => array(
- 'blowfish' => 22,
- ),
- 'costLimits' => array(
- 'blowfish' => array(4, 31),
- )
- );
- extract($options);
- if ($type === null) {
- $hashType = self::$hashType;
- } else {
- $hashType = $type;
- }
- $cost = self::$hashCost;
+ protected static function _crypt($password, $salt = false) {
if ($salt === false) {
- if (isset($costLimits[$hashType]) && ($cost < $costLimits[$hashType][0] || $cost > $costLimits[$hashType][1])) {
- trigger_error(__d(
- 'cake_dev',
- 'When using %s you must specify a cost between %s and %s',
- array(
- $hashType,
- $costLimits[$hashType][0],
- $costLimits[$hashType][1]
- )
- ), E_USER_WARNING);
- return '';
- }
- $salt = self::salt($saltLength[$hashType]);
- $vspArgs = array(
- $cost,
- $salt,
- );
- $salt = vsprintf($saltFormat[$hashType], $vspArgs);
- } elseif ($salt === true || strpos($salt, '$2a$') !== 0 || strlen($salt) < 29) {
+ $salt = self::salt(22);
+ $salt = vsprintf('$2a$%02d$%s', array(self::$hashCost, $salt));
+ }
+
+ if ($salt === true || strpos($salt, '$2a$') !== 0 || strlen($salt) < 29) {
trigger_error(__d(
'cake_dev',
'Invalid salt: %s for %s Please visit http://www.php.net/crypt and read the appropriate section for building %s salts.',
- array($salt, $hashType, $hashType)
+ array($salt, 'blowfish', 'blowfish')
), E_USER_WARNING);
return '';
}
Something went wrong with that request. Please try again.