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've created a MD5 shuffler with a defined number pattern. Does this make sense? Will this make storing passwords more secure? Is it efficient?

    <?php
echo "<pre>";
$md5 = "e2fc714c4727ee9395f324cd2e7f331f";
echo "Old hash: " . $md5 . "<br />";
$hash = str_split($md5, 1);
$shuffle = explode(",", "24,29,21,23,2,30,10,22,6,28,26,11,8,19,9,20,16,3,0,14,18,15,12,25,5,4,31,1,7,27,13,17");
$newhash = "";
for ($i = 0; $i < 32; $i++) {
$s = $shuffle[$i];
$newhash .= $hash[$s];
}
echo "New Hash: " . $newhash . "<br />";
$newhash = str_split($newhash, 1);
$reverse = "";
for ($i = 0; $i < 32; $i++) {
$s = $shuffle[$i];
$reverse[$s]= $newhash[$i];
}
ksort($reverse);
$reversehash = "";
foreach ($reverse as $value) {
$reversehash .= $value;
}
echo"Reversed hash: " . $reversehash . "<br />";
echo "</pre>";
?>
share|improve this question

2 Answers 2

up vote 6 down vote accepted

Security

In addition to what @Kid Diamond said (don't use md5, don't write your own hashing):

You also shouldn't 'improve' existing hashing functions yourself (in this case, shuffling doesn't make it more unsecure, but in general, it's not a good idea to change this kind of stuff yourself).

Shuffling doesn't really add that much security, as an attacker only has to look at your script to know how to shuffle themselves (it's security through obscurity). See also this discussion at security.stackechange.

Performance

Why are you doing this:

$shuffle = explode(",", "24,29,21,23,2,30,10,22,6,28,26,11,8,19,9,20,16,3,0,14,18,15,12,25,5,4,31,1,7,27,13,17");

instead of this:

$shuffle = [24,29,21,23,2,30,10,22,6,28,26,11,8,19,9,20,16,3,0,14,18,15,12,25,5,4,31,1,7,27,13,17];

The second one will definitely perform better.

You might also try to change

$reversehash = "";
foreach ($reverse as $value) {
    $reversehash .= $value;
}

to

$reversehash = implode("", $reverse);

It is definitely easier to read, and it might perform better.

Other

  • please use correct indentation. Your code is hard to read if you don't.
  • naming: $reversehash sounds like you are reversing the hash (eg 123 -> 321), but it is in fact the original hash, so use something like $originalHash.
  • magic numbers: why hardcode 32 if you can just use the length of the string? If you do this, nobody will wonder if 32 is correct, and you can reuse your code for different hash functions.
  • functions: I know that this is just a proof of concept, but still functions would be nice (like shuffle() and unshuffle() or similar).
share|improve this answer

Will this make storing passwords more secure?

Using MD5 for passwords is a bad idea, not because of its cryptographic weaknesses, but because it's fast. This means that an attacker can try billions of candidate passwords per second on a single GPU.

I wouldn't even bother creating my own secure hashing functions if I'm not an expert at it. There are libraries out from people who have already done all the hard work for you and have tested them thoroughly for security. PHPass is one of them.

Matter of fact, if you run PHP 5.5 or greater, you can use its native function: password_hash().

share|improve this answer
1  
If you haven't got PHP 5.5+ but have got PHP >= v5.3.7, you can use the plain PHP version of that function (and other related functions) from github.com/ircmaxell/password_compat It will work exactly the same, just a little slower (which, in itself is not a bad thing for security) –  Alister Bulman Sep 1 at 13:44

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.