1. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    Well, there are several issues with this:

    • mt_rand() is in no way suitable for security applications, because its output can be predicted. Even worse: For some reason you've truncated the function range (which is already quite small), making it even easier to guess the tokens.
    • The reset tokens are like passwords, because knowing them gives you full access to the corresponding user accounts. But you don't protect the tokens, you simply store them as plaintext.
    • Your request form displays an error message in case of a wrong email address, allowing everybody to check if an email address is registered on the website. This obviously violates the privacy of the members. The forum post you used as a template even says this!
    • Do not expose the PHP version in the email (the X-Mailer header). I don't know where you copied this from, but displaying internal server info to your users is a terrible idea. If there's a known security hole in this version, people can make use of that.
    • Get rid of the mail() function altogether. There are all kinds of mailer libraries like PHPMailer, so it's simply a waste of time to fumble with that low-level header stuff.
    • Get rid of all the "try-catch" stuff. It's useless. All it does is clutter the code and force you to remove it when the site goes into production.



    A secure password reset needs to have at least the following features:

    • The request form must not give feedback about whether the email address is valid or not. Send an email in any case. If the address isn't registered, say that in the email.
    • You must use a cryptographically secure random number generator (CSPRNG) to create the reset tokens. PHP has two CSPRNGs: openssl_random_pseudo_bytes() in the OpenSSL extension and mcrypt_create_iv() in the Mcrypt extension. The token must be sufficiently long (like 16 bytes).
    • Hash the tokens. They are as critical as the actual passwords.
    • The tokens must expire after a while. Simply store the current time when creating a token. When you validate it, you calculate the time difference and check whether it exceeds a certain limit (like 30 minutes).


    You also need to replace the password hashing scheme with an established and secure algorithm like brcypt. E-Oreo's scheme is great for explaining the concept of salts and iterative hashing, but it's not suitable for live websites.

    For further info and ideas, simply google for "secure password reset".
    Last edited by Jacques1; April 7th, 2013 at 08:54 AM.
  2. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    Your request form displays an error message in case of a wrong email address, allowing everybody to check if an email address is registered on the website. This obviously violates the privacy of the members. The forum post you used as a template even says this!
    What do you exactly mean by the forum post used as a template?

    Do not expose the PHP version in the email (the X-Mailer header). I don't know where you copied this from, but displaying internal server info to your users is a terrible idea. If there's a known security hole in this version, people can make use of that.
    I copied this from php.net


    Get rid of all the "try-catch" stuff. It's useless. All it does is clutter the code and force you to remove it when the site goes into production.
    I know, it was also mentioned in the threat by E-oreo that it is only for the production of the site, but not when it goes public.


    The request form must not give feedback about whether the email address is valid or not. Send an email in any case. If the address isn't registered, say that in the email.
    I put in a comment saying that I will delete those, but is was just to show that the mail was good or not ( for me to adjust the code) so I would delete those later.

    You must use a cryptographically secure random number generator (CSPRNG) to create the reset tokens. PHP has two CSPRNGs: openssl_random_pseudo_bytes() in the OpenSSL extension and mcrypt_create_iv() in the Mcrypt extension. The token must be sufficiently long (like 16 bytes).
    What could I do to make the tokens 16bytes? (yes I'm a noob in this so please help )


    You also need to replace the password hashing scheme with an established and secure algorithm like brcypt. E-Oreo's scheme is great for explaining the concept of salts and iterative hashing, but it's not suitable for live websites.
    Are you saying that this (the threat of making a secure login) is not the good way or something for a public site, I thought it was intended as such thing?

    but the intention of this was that when a user enters a email-adress there would be an email sent with a random token and the location of the page where the user would enter his token and his new password. The user doesn't see if the mail has been sent or not.

    You can't spam with this system because the user entered his username and his email, if the entered email is the email that corresponds with the email that belongs to this user, then an email would be sent. So you can't spam with this or the user would've given his email.

    The user gets an email with the token and enters it with his new password, the token will be given when the emails are the same.

    This was the intention...
  3. No Profile Picture
    Lost in code
    Devshed Supreme Being (6500+ posts)

    Join Date
    Dec 2004
    Posts
    8,296
    Rep Power
    7170
    Don't get too discouraged, it's not bad for an initial attempt.

    Are you saying that this (the threat of making a secure login) is not the good way or something for a public site, I thought it was intended as such thing?
    I would be completely comfortable using this method on a live site; although I probably wouldn't simply because using a library is easier.

    The only significant difference between bcrypt and this example is that the Blowfish algorithm used by bcrypt is significantly slower than sha256. You could compensate for this by increasing the number of rounds of sha256, however obviously the bcrypt algorithm will still have a slower-slowest speed than the sha256 algorithm, which is an advantage for bcrypt. This means that bcrypt is more future-proof than the sha256 method used in this tutorial.

    I may change this at some point in the future if I can work up some concise-enough code to do so.

    What do you exactly mean by the forum post used as a template?
    He's referring to the stackoverflow post that you referenced.

    I copied this from php.net
    It's best to just use PHPMailer or Swift mailer like Jacques1 recommended. The built-in PHP mail function is cumbersome and difficult to use.

    What could I do to make the tokens 16bytes? (yes I'm a noob in this so please help )
    Jacques1 is correct that mt_rand is not a good RNG to use for security purposes. However, unfortunately, writing widely-compatible code to generate secure random numbers in PHP is not simple.

    If you're going to use mt_rand, the best way to do it is to use the maximum possible range:
    PHP Code:
    mt_rand(0mt_getrandmax()) 
    Here's a slightly tweaked version of the RNG from the bcrypt library that Jacques1 linked to. My changes are noted in the comments. This will give you much better random numbers:
    PHP Code:
    <?php
        $raw_length 
    16;
        
    $buffer '';
        
    $buffer_valid false;
        if (
    function_exists('mcrypt_create_iv') && !defined('PHALANGER')) {
            
    $buffer mcrypt_create_iv($raw_lengthMCRYPT_DEV_URANDOM);
            if (
    $buffer) {
                
    $buffer_valid true;
            }
        }
        if (!
    $buffer_valid && function_exists('openssl_random_pseudo_bytes')) {
            
    // Note: I added a check against $is_crypto_strong
            
    $buffer openssl_random_pseudo_bytes($raw_length$is_crypto_strong);
            if (
    $buffer && $is_crypto_strong) {
                
    $buffer_valid true;
            }
        }
        
    // Note: I added an error supression operator on is_readable
        // If open_basedir is enabled, /dev/urandom is almost certainly not going to
        // be inside it, and this will cause PHP to throw an E_WARNING from is_readable
        
    if (!$buffer_valid && @is_readable('/dev/urandom')) {
            
    $f fopen('/dev/urandom''r');
            
    // Note: I added a check to make sure $f was actually opened successfully
            
    if ($f) {
                
    $read strlen($buffer);
                while (
    $read $raw_length) {
                    
    $buffer .= fread($f$raw_length $read);
                    
    $read strlen($buffer);
                }
                
    fclose($f);
                if (
    $read >= $raw_length) {
                    
    $buffer_valid true;
                }
            }
        }
        if (!
    $buffer_valid || strlen($buffer) < $raw_length) {
            
    $bl strlen($buffer);
            for (
    $i 0$i $raw_length$i++) {
                if (
    $i $bl) {
                    
    $buffer[$i] = $buffer[$i] ^ chr(mt_rand(0255));
                } else {
                    
    $buffer .= chr(mt_rand(0255));
                }
            }
        }
        
        
    // This is your secure random number
        
    echo $buffer;

    One thing you'll want to test is what happens when a single user requests multiple password reset tokens. In most cases you would want to expire all of their previous tokens when a new one is generated.
    PHP FAQ

    Originally Posted by Spad
    Ah USB, the only rectangular connector where you have to make 3 attempts before you get it the right way around
  4. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    Taking the random number generator from the library above is a good idea. Wrap it in a function, and you're done.



    Originally Posted by E-Oreo
    I would be completely comfortable using this method [the custom hash algorithm in this tutorial] on a live site; although I probably wouldn't simply because using a library is easier.
    I hate to be a party pooper, but how can you claim your password scheme is ready for production when it has never been reviewed or tested?

    How do you know you can append the salt rather than, say, prepending it? How do you know you can simply iterate the hashing without any kind of variation? The UNIX crypt() function already has a SHA-256 and a SHA-512 implementation, and they are very different from your algorithm. How do those differences affect security?

    This tutorial is great for understanding the concepts and learning how to use PDO. It really is. But when you start promoting your own crypto algorithm as an actual alternative to bcrypt, you're making promises you cannot keep. You're not a cryptographer.

    Do you know how much effort and testing went into bcrypt? Why do you think they did that instead of simply sitting down for 5 minutes and writing a simple loop?
    Last edited by Jacques1; April 7th, 2013 at 05:27 PM.
  5. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    E-oreo, can you else please show us how you would make an login with a library?
  6. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    Originally Posted by derplumo
    E-oreo, can you else please show us how you would make an login with a library?
    I already posted a link to an excellent hashing library (twice, actually):

    https://github.com/ircmaxell/password_compat

    If you scroll down on that site, there's a readme giving concrete examples of how to generate and check passwords (it's very easy).

    That library should be pretty much the best you can get at the moment:

    • It only uses established and well-tested functions. No home-made stuff.
    • The code is clean and simple.
    • It has already gained some attention, which means many people have checked it.
    • It's very easy to use. There's hardly a chance to make a mistake (in contrast to the native crypt() function in PHP).


    And last but not least: It provides forward-compatiblity for PHP 5.5 (hence the name). This means that when PHP 5.5 with its new password API is out, you can keep your code and simply remove the library in order to use the native functions.

    So if you have a current PHP version >= 5.3.7, use this library!

    If not, use the older PHPass library.
  7. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    so now we just have to change this:
    PHP Code:
    $salt dechex(mt_rand(02147483647)) . dechex(mt_rand(02147483647)); 
    $password hash('sha256'$_POST['password'] . $salt); 
    for(
    $round 0$round 65536$round++) 
      { 
          
    $password hash('sha256'$password $salt); 
      } 
    into this:
    PHP Code:
    $hash password_hash($passwordPASSWORD_DEFAULT, ["cost" => 10]);
    if (
    password_verify($password$hash)) {
        if (
    password_needs_rehash($hash$algorithm$options)) {
            
    $hash password_hash($password$algorithm$options);
            
    /* Store new hash in db */
        
    }

    right? (yes we then just have to fill in the variables)
  8. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    No, it's just

    PHP Code:
    $hash password_hash($passwordPASSWORD_BCRYPT, ["cost" => 10]); 
    for hashing $password

    and

    PHP Code:
    password_verify($password$hash
    for checking if $password matches $hash.

    That's it. One function call for each action.

    The code you quoted is for updating all existing hashes in case you wanna change the algorithm or cost factor. But that's rather unlikely. If you do actually need to change it in a few years, you can still add the code.
    Last edited by Jacques1; April 9th, 2013 at 02:35 PM.
  9. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    ok, danke but I copied the if(.... part because the code controls if the password was hashed like it should... so should I use the if(... or the single line? And of course I'll have to change the password => char(65) (something like this) to varchar(255).
  10. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    Originally Posted by derplumo
    ok, danke but I copied the if(.... part because the code controls if the password was hashed like it should... so should I use the if(... or the single line?
    The function call password_verify($password, $hash) returns true or false depending on whether the password is correct or not. Of course you have to actually use this return value in your concrete script.

    Like so:

    PHP Code:
    // $password comes from the login form, $hash comes from the database

    if ( password_verify($password$hash) )    // login successful
    {
        echo 
    "You've successfully logged in. Welcome!";
        
    // set the session variables etc.
    }
    else                                        
    // login failed
    {
        echo 
    'Sorry, the email address or password is incorrect';



    Originally Posted by derplumo
    And of course I'll have to change the password => char(65) (something like this) to varchar(255).
    A brypt hash (PASSWORD_BCRYPT) has exactly 60 characters, so the appropriate type is CHAR(60). The VARCHAR(255) is meant for PASSWORD_DEFAULT (which means letting the PHP developers choose the strongest current algorithm instead of picking one yourself).
  11. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    ok, I think I got it, but what do I exactly have to do to call the library, like require ...?
  12. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,904
    Rep Power
    1045
    Yes, require. The library is just a normal PHP script like your own files.
  13. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    weird, i get an error at this line:
    PHP Code:
         $hash password_hash($_POST['password'], PASSWORD_BCRYPT, ["cost" => 10]); 
    and the error was:
    Parse error: syntax error, unexpected '['

    what could be wrong?

    I also tried to change the $_POST['password'] into an other variable by making a new variable but it didn't work, so I think the fault could be in ["cost" => 10]
  14. Come play with me!
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    13,742
    Rep Power
    9397
    That's PHP 5.4+ syntax. If you have <=5.3 then you need the longhand notation.
    PHP Code:
    $hash password_hash($_POST['password'], PASSWORD_BCRYPT, array("cost" => 10)); 
  15. Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Feb 2013
    Posts
    311
    Rep Power
    8
    So now I think the table users now has this structure:

    Code:
    CREATE TABLE `users` (
      `id` int(11) NOT NULL AUTO_INCREMENT,
      `username` varchar(255) COLLATE utf8_unicode_ci NOT NULL,
      `password` char(60) COLLATE utf8_unicode_ci NOT NULL,
      `email` varchar(255) COLLATE utf8_unicode_ci NOT NULL,
      PRIMARY KEY (`id`),
      UNIQUE KEY `username` (`username`),
      UNIQUE KEY `email` (`email`)
    ) ENGINE=InnoDB  DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci AUTO_INCREMENT=1;
    note that the salt column has been deleted and the column `password` the char(64) has been changed in char(60)

IMN logo majestic logo threadwatch logo seochat tools logo