2

For the record, this was purely for learning purposes, I've been told don't even use this for a personal website that I don't expect anyone to visit, so this post is unfortunately the last I'll see of this code. However, I've done all the things I read about and it seems decent to me (but I'm no expert), so I'm just curious as to which parts could potentially be bad enough that the recommendation to everyone is to never ever use a custom solution?
Not naming anyone, but I'm pretty sure I've already done a miles better job than the free webhost that begins with a triple number haha, my plaintext password was stolen from there.

Here's the basic idea of what I did:

Salt and IV
Each salt is generated by a random but not cryptographically secure function in PHP (it's better than the default random though). It is then encrypted with blowfish using a key defined in the code, and an IV defined in an actual file (which is protected by htaccess). The encrypted value is stored, so it's very easy to change the IV or salt key to invalidate all passwords. Also if the database is compromised, the salts still can't be decrypted without getting access to the file system.

Hash
It uses key stretching and sha512. The amount of key stretching is defined by a modulus of a crc32 value of the password and salt, so each user has a different amount, and you can change the value to allow longer processing times. Each repeat hash uses a few hashes inside it in an attempt to keep it pretty random.

Here's the actual code I have, if you don't know PHP it's still fairly easy to see what's going on line by line:

$salt_key = 'íSgüý[Îaí²zÿCV™F÷œDDa©^fU©êm‹“.Ù   ®Kâ³ÃX}rÿ,¢\ˆÿ7L³¢QÆH’YeTsÙõŸ¥éÙí#W—c´VÆ';

function get_iv(){
    // It stores it as a file, but there's no point showing that part so here's the actual code bit
    return mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_BLOWFISH, MCRYPT_MODE_CBC), MCRYPT_RAND);
}

function generate_salt(){
    global $salt_key;
    $salt = substr(uniqid(mt_rand(), true), 0, 32);
    $key = hash('sha256', $salt_key, true);
    $iv = get_iv();
    $encrypt = mcrypt_encrypt(MCRYPT_BLOWFISH, $key, $salt, MCRYPT_MODE_CFB, $iv);
    return array($salt, $encrypt);
}

function decrypt_salt($encrypted){
    global $salt_key;
    $key = hash('sha256', $salt_key, true);
    $iv = get_iv();
    return mcrypt_decrypt(MCRYPT_BLOWFISH, $key, $encrypted, MCRYPT_MODE_CFB, $iv);
}

function hash_password($password, $salt){
    #return hash('sha512', $password.$salt, true);
    $hash = $salt;
    for($i=0; $i<(crc32($salt.$password) % 129197); $i++){
        $hash = hash('sha512', $hash.$password.$salt.hash('sha512', $salt.$hash.$password).md5($salt.md5($password.$hash)), true);
    }
    return $hash;
}

The part to check if the password is valid basically just checks the new hash against the old one so I left it out. The only drawback based on the things I've read a good algorithm should do, is it doesn't use a lot of memory.

  • 3
    The question you are going to get is, "why use a custom solution instead of the standard libraries that do it right?" If you want a review of certain elements of your scheme for learning purposes, that's fine, but just know that the way you phrased your intro paragraph, you are going to get flooded with "just use bcrypt/PBKDF2". – schroeder Oct 13 '16 at 16:22
  • 2
    I'll focus on just one little detail (and leave the rest for someone else), i.e. mt_rad(). The MT scheme is known to generate poor random numbers at the beginning and then generate better ones after some time. Therefore if you run this as plain PHP you will get predictable salts, yet if you use something like php-fpm then you will end with good state for the PRNG. That's bad because your scheme produces different security concerns depending on how it is used. – grochmal Oct 13 '16 at 16:29
  • Thanks, edited the post a little to clarify I'm not planning on using it haha, and I read that salts don't even need to be too random - it's likely attackers would get access to the salts if they had access to the password, so overall it makes no difference if they're hard to predict or not – Peter Oct 13 '16 at 16:46
4

Open questions:

  • Why aren't you using a cryptographically secure RNG for the salt? You're not going to be generating them so frequently as to risk depleting the system entropy pool on a modern system.
  • How was your salt encryption key generated?
  • How long is your salt_key (before you hash it)?
  • Why are you hashing your salt_key? That adds no security.
  • Why the overly-complicated nested hashing (including use of MD5, of all things) in your key-stretching loop? It puts me in mind of Moxie Marlinspike's commentary on the (broken) MS-CHAP v2 scheme (search for the string "unnecessary complexity").

Notable issues:

  • You do, in fact, want to use securely-random salts (and IVs). It matters how predictable the salt is; predictable salts allow attackers to pre-compute lookup tables for the predicted salts against likely passwords. Predictable IVs are probably less of a risk in this context but there's no reason not to use securely random ones.
  • Your key stretching iteration algorithm provides little-if-any increase in security - for each trial password, the attacker can compute the iteration count just fine, assuming they know the salt (and if they don't then they can't try to brute-force the password verifier at all) - and puts you at risk of running far fewer rounds of hashing than you intend. In particular, if the CRC is an exact multiple of 127197, the user's salt is stored as their password verifier and the password is not used at all, so any password at all will work for that user! If you get a million users, about 8 of them will have no password protection at all.
  • You have no method for rotating or upgrading the algorithm for password verification, at least nothing visible in what you've presented here. Hardcoded critical attributes (such as the key, the cryptographic primitives, and the way iteration count is determined) may need to change at some point in the future, at which point you would need to be able to verify a user's password under the old scheme before generating a new password verifier with the new scheme (unless you just plan to force everybody to reset their password, which is a hassle and should only be done when there's reason to expect the old ones are compromised). Therefore, you need a way to specify a password verification algorithm version, or at least need to store identifiers (probably indices) for your various currently-constant values, and look the values up based on each user's stored version identifier / index.
  • In the worst case, 129+k iterations of SHA-512 (never mind SHA512(SHA512(MD5(MD5)))) is going to take a long time on your server. An attacker that finds a combination of username and password for which it is so expensive to compute the verifier can just submit a bunch of requests with credentials like that (especially since, due to the reversibility of CRC, the attacker can manipulate the input to get exactly the desired checksum). That will probably cause a denial-of-service on your server. Work factors for password verification need to be high enough to slow down brute-forcing, but must not be high enough to be a way that an attacker can overload a server.
  • Thanks for the reply. I hash the salt key because mcrypt needs a 128 bit key, and I think I used one of the more secure online things to generate it (I updated the post with the key). The 'overly complicated hashing' was just to mix up the main hash a bit, and since md5 was only contained inside, I didn't think it'd particularly hurt to use it. – Peter Oct 14 '16 at 1:53
  • Although since I was generating a new salt for each user, how would lookup tables be used? Even if the salts were slightly predictable, they're still random (just not super random), so would still be unique for each user. Great point about the crc32 thing though, I totally hadn't realised that, I guess that makes a pretty terrible flaw haha. The plan was to store the 127197 value for each user, so if it was changed, it'd login using the database value then recalculate using the new one. I've no idea how you'd make it so you can change the entire algorithm though :P – Peter Oct 14 '16 at 1:57
  • @Peter: Egads. Why would you use an online generator (presumably operated by somebody else, who now potentially knows your key) to do that? If you're on a *nix system, just grab some bytes from /dev/urandom; if you're on Windows it's a little harder but there are plenty of software packages that will securely generate keys for you. It makes a lot more sense to simply use a secure random generator to generate a 16-byte string than it does to use a 128-bit hash algorithm (much less a 256-bit one, which I assume you then truncate) on a string longer than that. – CBHacking Oct 14 '16 at 4:18
  • If the salts are predictable, an attacker can generate lookup tables for those specific salts before they've breached your password verifier database. If the attacker can predict 100,000 salts and uses the list of the 100,000 most common passwords, that's only 10,000,000,000 (10 billion) combinations. At 64 bytes each (512-bit digest) that's 640GB; big but not outrageous. Even if salts are only predicted with 10% accuracy (i.e. you're actually using 10,000 of those salts) and only 10% of your users use one of the 100,000 most common passwords (realistically it's more), that's 1000 users. – CBHacking Oct 14 '16 at 4:29
  • Also, Mersenne Twister is more than slightly predictable. Its internal state is pretty big, so it takes observing a lot of outputs in a row - 624 for the most common MT-based PRNG - to determine that state from the outputs alone. Once you do, though, it's game over. Every future output can be predicted, in order, with perfect accuracy. Same if the initial state is known somehow (for example, it's seeded from the system time, and you know what time the initial seeding happened). – CBHacking Oct 14 '16 at 4:32
1

One bad thing about it is that it's complicated, and any developer joining the project won't be able to easily tell whether it's secure unless they take the time to read through it and understand it. If they care about security and about system quality in general then they may be motivated to take that time even if if's not necessary for their immediate tasks.

The simple solution, in PHP 5.5 or later, is to just use the built in password_hash and password_verify functions.

  • 1
    It actually isn't that complicated, just useless. – forest Mar 16 '18 at 6:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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