SHA256 password hash generation and verification

Question

I would like to optimise this code. It seems a bit verbose, particularly with the 'elseif' that doesn't do anything.

This code either:

• takes a plain text password, generates a salt, and returns them both OR
• takes a plain text password and a salt with which to encrypt the password, and returns them both (even though the salt has already been supplied).

class Member extends ActiveRecord\Model {
private function process_password($password, $salt = '') {
 /**
 * Process a password to encrypt it for storage or verification.
 * @param string - plain text password for processing.
 * @access private
 * @return array Containing the encrypted password and the salt.
 */
 //Check if the salt has been supplied. If not, generate one.
 if (!$salt) {
 $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
 } elseif ($salt && strlen($salt) == 64) {
 //Do nothing, the salt has already been supplied here.
 } else {
 log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
 return false;
 }
 $hashed_password = hash('sha256', $password.$salt);
 return array('password' => $hashed_password, 'salt' => $salt);
}
}

• DaveRandom: If you want to get rid of that empty elseif, just change it to strlen($salt) != 64 and put the else code in the elseif block.
• B4NZ41: Your code looks like already optimized! Is it working? If so why change?
• Kieran: @B4NZ41 I've tried my best to optimise it, but I'm trying to learn to code a bit more 'profesionally'. Particularly by getting rid of the extra elseif! :)
• Paul DelRe: I'm a big fan of removing nesting whenever possible. See this for more information. Bad formatting since I can't post an answer on closed questions, so here's a pastebin.
• Paul DelRe: Please disregard the pastebin as it has an error. I am still a fan of reducing nesting, but I no longer think it makes a difference here.

Answer 1

How about the following:

if (!$salt) {
 $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
} elseif (strlen($salt) !== 64) {
 log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
 return false;
}
// if we're here, we have a valid salt

Improvements

• Removed $salt from second condition (already known to be TRUE)
• Flipped length logic, moved else code into second block
• Removed empty condition block

Answer 2

You could reduce your if/elseif/else to:

if (!$salt) {
 $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
} elseif (strlen($salt) !== 64) {
 log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.');
 return false;
}

You already know that $salt is truthy since you check in the if part, so all you care about is that it's valid (length 64).

A way to further improve this would be to use a standard password hashing library instead of using PHP's built in hash function. For example, yours has a major potential problem: It's way too fast. With enough hardware, an attacker could just brute force any short passwords.