…and stores it in the member’s record as well as e-mailing out a “Password Reset Email” where the user would click on a link, and if the code matches the one I stored in the database, then it temporarily logs them in.
Is the code above sufficient as far as security goes?
I think it would be better if it did not reset the user’s password first. Ideally a “password reset” link is sent by email, allowing the user to change the password thereafter. Otherwise this is open to abuse whereby someone can enter random email addresses forcing peoples’ passwords to be reset.
Your code is sufficient enough in terms of security; you should also set a time limit to boost security further.
The PHP function for handling passwords is password_hash() which takes care of all the hashing for you using whatever hash is considered most effective at the time as it can update hashes automatically as people log in if they are using an older hash.
For a reasonably secure reset you should send them an email containing a link containing a token that uniquely identifies their account for a short period (no more than say two or three hours). If they click the link in the email before the token expires then they get taken to a page that gives them a new password. The new password can be generated at that point - no need to generate it just because Joe Anybody said they forgot the password to someone else’s account.
For additional security you can add a value to the token that is derived from information in their account and expire the token early if someone tries to guess the token value and gets that part wrong (not that this is likely to happen as even a 10 character token would require many millions of guesses a second to have a chance of guessing the right token value before it expires).
Anything that generates a random string of 10 or more characters including both numbers and letters (preferably including both lower and uppercase letters) should be suitable for use as a token for allowing people to reset their password. Given that it will take far longer to guess at any reasonable rate of guessing to have even a slight chance of getting it right it is also extremely unlikely that the same token would be issued a second time before the first expires - it is still worth checking for that though since if you do happen to have that occur it could cause major problems while regenerating a token if it duplicates an existing one is relatively simple to do.
In your case because you are using sha1 there is an extremely small possibility of duplicating a token (but not a zero chance).
The output from sha1 gives 64 possible values for every character so even five characters gives over a million possible values and seven characters gives over 4 trillion values to test
Taking the first 40 characters is somewhat superfluous since a sha1 string is always 28 characters long (which is still way longer than necessary to be secure for the short time such a token needs to exist for).
The problem you have with that code though is the value that you are feeding to the sha1 ad mt_rand itself does not return very many alternative values. In most cases it only has 2147483647 different values it can return which means that no matter what you do from there the token can only have that many possible values. While these values are scattered through the possible sha1 values and are almost certainly not goint to have two that map to the same sha1 value this does mean that only those couple of billion values need to be used in making a guess (if the person realises that’s how you are generating the tokens) instead of all possible values. This is still relatively unlikely though. It could be improved on by using code that has a larger number of possible values aswhat you have basically corresponds to a token length of five characters in terms of the number of values actually being used (although unless someone does recognise how the tokens are being generated to realise that only that small subset can ever be valid and so the guess only needs to be made from amongst those, you effectively have a situation where almost 100% of the possible values to try will never be valid).
Based on what you said, it sounds like what I have is more than sufficient until I have time to revisit things in my next version and maybe make them even more secure.
You shouldn’t need to generate a temporary password at all, and I agree with N9ne that doing so can allow abuse.
What you really want is a new field in your database, separate from the password, called something like password_reset_token. That token would be generated much the same way that you’re currently generating the temporary password. (Though, the substr is unnecessary. A sha1() hex string will always be exactly 40 characters, and you may as well use them all for the token.) Then you would email a reset link with that token (something like debbie.com/user/password-reset?token=d0be2dc421be4fcd0172e5afceea3970e2f3d940). When the user goes to that link, then the page allows them to enter a new password for their account.
User enters email into form and clicks “Reset Password”
My script checks the $_POST array for a value
My script runs a regex to make sure it is a valid email format
My script looks for the email in the database
If the email is found, then…
My script generates a Temporary Password as seen in my OP
My script runs this update…
$q2 = "UPDATE member
SET temp_password = ?,
temp_reset_on = NOW(),
updated_on = NOW()
WHERE email = ?
LIMIT 1";
My script sends an email to the user…
Dear Debbie,
We received a request to reset your password.
If you forgot your password, then click on this link.
If you did not make this request, or remembered your password, then ignore this email.
Sincerely,
Customer Service
If the user clicks on the link, then the user is taken to my “create-password.php” script, and this happens…
Check for Temp Password in $_GET
Look for user by Temp Password
If the user is found, then…
Make sure Temp Password is not more than 30 minutes old
As far as I can make out you have called the extra field temp_password while Jeff has called it password_reset_token. His name is slightly more descriptive of what the field actually contains.
Yup, what felgall said. I think we were all thrown because when you said you were generating a temporary password, we assumed that meant you were overwriting the actual password field. Now I see that isn’t the case, so the process you have will work just fine.
