Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am having some issue to use PHP5 password_hash() function. My server is not supporting it, so I am using a function to hash. Is this one secure?

public function Pass_Hash ($password) {

      $blowfish_salt = bin2hex(openssl_random_pseudo_bytes(22));
      $hash = md5(crypt($password, "X1;G^COU8U`Bo*A6@9<on5yQ6P87M]".$blowfish_salt)); 
      return $hash;

}

I read lot of articles regarding password hashing. Can anyone help me rewrite it more securely?

share|improve this question
2  
Why do you hash the crypt hash with md5? –  Sven Walter Feb 13 at 17:51
    
To make it look like Md5 only any issues? –  BANNA Feb 13 at 17:53
4  
I don't know about any issues, but I would rather use one strong hash algorithm, than mixing two. Mixing can only reduce the security. –  Sven Walter Feb 13 at 18:01
1  
Relevant article: crackstation.net/hashing-security.htm –  ntoskrnl Feb 13 at 22:42
    
The PHP5 password_hash() function isn't available everywhere, but there are some drop-in replacements for it. I recommend you use one of those; then you can upgrade to the real password_hash() function without difficulty when it becomes available. –  TRiG Feb 14 at 15:18
show 1 more comment

3 Answers 3

up vote 12 down vote accepted

NO NO NO NO NO!

Besides what others have pointed out, I find some serious flaws in your code.

  • You are not actually using blowfish. To use blowfish, your salt must begin with something like $2y$07$ see the PHP Documentation
  • Removing your $blowfish_salt variable from the code makes no difference! This will produce the same output. Therefore, we conclude that you are actually using a constant salt in your code.
  • The output of crypt is more secure than MD5. MD5 is an algorithm that is known to have a lot of duplicates. Many different inputs will produce the same results. That's not optimal. Using real blowfish algorithm is even more secure than what you currently have.

Here is some testing code I used to come to my conclusions:

<?php
$password = "bubu";
  $blowfish_salt = bin2hex(openssl_random_pseudo_bytes(22));

$blowfish_salt = "11b19e4e03d32f27f9db5646e56f63458113e9da56f7"; // Since this is the only randomness, I have replaced the randomness with this constant.

echo "salt is $blowfish_salt<br />";

$first = crypt($password, "X1;G^COU8U`Bo*A6@9<on5yQ6P87M]".$blowfish_salt);
echo $first . " ... constant with what you call blowfish<br />";

$hash = md5($first); 
echo $hash . " ... md5-ed version of above. This is the result of your current code<br />";
echo md5(crypt($password, "X1;G^COU8U`Bo*A6@9<on5yQ6P87M]")) . " ... and oops, that was exactly the same!<br />";

$first = crypt($password, "X1;G^COU8U`Bo*A6@9<on5yQ6P87M]");
echo $first . " ... constant salt<br />";

$first = crypt($password, $blowfish_salt);
echo $first . " ... only blowfish salt<br />";
echo "<br />";
echo "BELOW USING 2Y PADDING:<br />";
echo "<br />";
$first = crypt($password, "\$2y\$07\$X1;G^COU8U`Bo*A6@9<on5yQ6P87M]".$blowfish_salt);
echo $first . " ... a lot, with blowfish. Causes error because salt is too long<br />";

$first = crypt($password, "\$2y\$07$\X1;G^COU8U`Bo*A6@9<on5yQ6P87M]");
echo $first . " ... constant salt. Still causes error because salt is too long<br />";

$first = crypt($password, "\$2y\$07\$".$blowfish_salt);
echo $first . " ... only blowfish salt. And now THAT is blowfish!<br />";

echo "<br />";
echo "Now back to md5 with this result again:<br />";

$hash = md5($first); 
echo $hash;
?>

The output of this is:

salt is 11b19e4e03d32f27f9db5646e56f63458113e9da56f7
X154A2VqJVf4I ... constant with what you call blowfish
7b5f3730488255efcff9441a238d5c5d ... md5-ed version of above. This is the result of your current code
7b5f3730488255efcff9441a238d5c5d ... and oops, that was exactly the same!
X154A2VqJVf4I ... constant salt
11Xw1fe5HDjyo ... only blowfish salt

BELOW USING 2Y PADDING:

*0 ... a lot, with blowfish. Causes error because salt is too long
*0 ... constant salt. Still causes error because salt is too long
$2y$07$11b19e4e03d32f27f9db5uyeLJeUJh/VUoi1RFtAoDAE.eegj4bwO ... only blowfish salt. And now THAT is blowfish!

Now back to md5 with this result again:
e35cb490b47d3211dac9f85acfc2fb15

Please, take your time and study these results. Look at the code. Look at the results. Look at the code again. Look at the results again. Now look at the code one more time: Do you see the flaws? If you don't, I pointed them out at the beginning of my answer.

What you should do:

  1. Generate a salt, store that somewhere (as suggested by Chris). Get rid of that constant-salt you have.
  2. Use the real blowfish, and don't use MD5 afterwards

    public function Pass_Hash ($password, $blowfish_salt) {
          $hash = crypt($password, "\$2y\$07\$" . $blowfish_salt)); 
          return $hash;
    }
    $generated_salt = bin2hex(openssl_random_pseudo_bytes(22));
    Pass_Hash("a very secret password", $generated_salt);
    

Now we're talking.

However, I should add that I am not a security experts. There are likely several more aspects that one can think of. If you are interested in security and secure stuff, I recommend paying our StackExchange friends at Information Security a visit. But in the end I think that the question is: How secure do you want to be?

share|improve this answer
    
Thank you very much... I rewrite as per your suggestions and findings its now public function Pass_Hash ($password) { $generated_salt = bin2hex(openssl_random_pseudo_bytes(22)); $hash = crypt($password, "\$2y\$07\$" . $blowfish_salt); return $hash; } Is this ok now? –  BANNA Feb 14 at 5:42
    
I am planning to use crackstation.net/hashing-security.htm#phpsourcecode this library. But while i try to hash a password using $mypass = create_hash("Password123"); its generating sha256:1000:EzeYvaE4tVPzpWJ51l+etMvcj1qKZF26:G3GncF0rC4q86yWh7A0GTdrXLuciXGMw you mean i have to store this hash to DB? But in every refresh it changes why its changing? –  BANNA Feb 14 at 5:51
    
@BANNA It seems like you are lacking a lot of understanding about the available hashing methods. Please read the documentation for the functions that you are using. Whether your functions are "ok" or not really depends on one thing: Are you able to use them? Do they work for you? Can you compare if an entered password is the same as a stored password with them? If you are unable to do that, I suggest showing what you have tried and explaining what the problem is and ask on Stack Overflow. –  Simon André Forsberg Feb 14 at 9:35
    
bin2hex(openssl_random_pseudo_bytes(22)) is still clearly wrong. The salt bcrypt salt is 16 bytes encoded with a Base64 variant (not hex), producing 22 characters. –  CodesInChaos Feb 28 at 15:49
add comment

Cryptography is hard. Doing it yourself will only cause security issues. It is better to use a library made by experts.

You can take a look at http://stackoverflow.com/a/17073604/393157 to find a suitable library.

share|improve this answer
add comment

No.

Concern 1:

Most password hashing libraries will perform a hash recursively 2^n times. on Modern systems, they choose 2^8 or higher (resulting in 512 hashes).

While you are using a salt, you're only hashing the password once. This would allow a hacker to create a rainbow table with considerably less effort.

Concern 2:

Also, another concern, if you're creating a random salt everytime, you'll never be able to verify the password. I would pass the salt in (or return the salt with the hash). That way the hash is repeatable.

share|improve this answer
    
Actually, the salt isn't really random. Only the constant part is used over and over. See my answer. –  Simon André Forsberg Feb 13 at 19:39
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.