3

I'm upgrading my auth class, replacing md5 with crypt for storing passwords. Here's the approach I've taken:

function crypt_pass($pass, $userID) {
    $salt = $userID .'usesomesillystringforsalt';  // min 22 alphanumerics, dynamic
    $method = (version_compare('5.3.7',PHP_VERSION,'>=')) ? '2y' : '2a'; // PHP 5.3.7 fixed stuff
    if (CRYPT_BLOWFISH == 1) {
        $blowfish_salt = '$'. $method .'$07$'. substr($salt, 0, CRYPT_SALT_LENGTH) .'$';
        return crypt($pass, $blowfish_salt);
    }
    return sha1($pass . $salt);        
}

Making the salt unique per user adds a step, a db lookup for the supplied username's id ... I figure it's worth it. Am I wrong about that? Is there anything else I'm not considering here?

designosis
  • 5,182
  • 1
  • 38
  • 57
  • 1
    This is a better fit for http://codereview.stackexchange.com/ – Repox Aug 05 '12 at 09:28
  • 1
    Your php version magic is faulty. According to your logic, `5.2.17` > `6.0.0`. – dialer Aug 05 '12 at 09:28
  • @dialer true dat, thank you! i'll edit above when i find the best alternative. – designosis Aug 05 '12 at 09:29
  • The salt should be different for each password otherwise it would be just as bad as no salt. The use of a salt is to remove the possibility to brute force the whole list in one go. Another problem with your approach is that you only have two encryption steps crypt + sha1. To drive the computational cost of a brute force attack up you should run crypt or sha repeatedly on it's own result. Say atleast one thousand times. – Eelke Aug 05 '12 at 09:58
  • 1
    Eelke, actually, they shouldn't even attempt to manually create a hash. Better to use a library that gives them access to bcrypt or scrypt. – Joey Aug 05 '12 at 10:06
  • @Eelke Great idea to iterate crypt results. But your first point about different salts per password ... isn't that addressed above, where the salt includes `userID`, making it different per password? – designosis Aug 05 '12 at 10:24
  • I was just confirming that making it unique is what you should do. Thought all implementations I have seen use a purely random salt. – Eelke Aug 05 '12 at 11:15
  • 1
    HMAC + bcrypt is the way to go. Here are some good articles to get you started: [Let’s talk about password storage](https://blog.mozilla.org/webdev/2012/06/08/lets-talk-about-password-storage/) and [Password Storage – Basic Security Part 1](http://coffeeonthekeyboard.com/password-storage-basic-security-part-1-706/). The second link is part of a whole series on web security best practices. Definitely recommended reading. – rexmac Aug 05 '12 at 11:43

1 Answers1

0

The whole point of salting is to produce different encrypted strings from the same password. If you use the same salt every time, this will not happen, so you may as well not use salts at all. You should be creating a random new salt for every password, and then storing it in the database alongside the encrypted-and-salted string.

VettelS
  • 1,204
  • 1
  • 9
  • 17
  • I don't know enough to argue one way or the other, but it seems to me that using the database `id` of the user to generate the salt is pretty much the same as storing the salt within the same record. Each password gets a unique salt. What am I not seeing? – designosis Aug 05 '12 at 10:59
  • I didn't realise you're using the `id` in the salt as well- should have read it better :) – VettelS Aug 05 '12 at 13:11