Forgotten password / password reset

Question

I have hand-coded a forgotten-password reset system. I am apprehensive about security issues/vulnerabilities. What part of this can be exploited or could be made more secure?

forgot-password.php

 <?php
require_once 'connect.php';
if($_SERVER['REQUEST_METHOD'] === 'GET') {
 if(isset($_GET['email']) && !empty($_GET['email'])) {
 $email = htmlentities(mysqli_real_escape_string($connection, trim($_GET['email'])));
 if(filter_var($email, FILTER_VALIDATE_EMAIL)) {
 $find_email_address_query = "SELECT * from users WHERE email = '{$email}'";
 $do_find_email_address_query = mysqli_query($connection, $find_email_address_query);
 if($do_find_email_address_query) {
 $count_email = mysqli_num_rows($do_find_email_address_query);
 if($count_email > 0) {
 $hash = sha1(md5(mt_rand(0, 1000000000)));
 $forgot_password_hash_query = "UPDATE users SET forgot_password_hash = '{$hash}'
 WHERE email = '{$email}'";
 $do_forgot_password_hash_query = mysqli_query($connection, $forgot_password_hash_query);
 if($do_forgot_password_hash_query) {
 $data = array("result" => 1, "message" => "Link To Reset Your Password Has Been Successfully
 Sent To $email");
 sendEmail($email, $hash);
 }
 else {
 $data = array("result" => -5, "message" => "Error Encountered! Try Again Later.");
 }
 }
 else {
 $data = array("result" => -4, "message" => "Non-Existant Email Address! Perhaps You Need To Register
 For An Account.");
 }
 }
 else {
 $data = array("result" => -3, "message" => "Encountered A Problem Resetting Your Password!
 Please Try Again Later.");
 }
 }
 else {
 $data = array("result" => -2, "message" => "Invalid Email Address! [email protected] is an example
 of the correct format.");
 }
 }
 else {
 $data = array("result" => -1, "message" => "No Email Address Provided!");
 }
}
else {
 $data = array("result" => 0, "message" => "Incorrect Request Method!"); 
}
function sendEmail ($recipient, $hash) {
 $to = $recipient;
 $subject = "Reset Your Password | DOMAIN.COM";
 $message = '
 Did you just forget your password ?
 No worries! It is just too easy to get a new one.
 Please click this link to reset your password.
 http://www.DOMAIN.COM/reset-password.php?email='.$to.'&hash='.$hash.'
 '; // Our message above including the link
 $headers = 'From:[email protected]' . "\r\n"; // Set from headers
 mail($to, $subject, $message, $headers); // Send our email
} 
mysqli_close($connection);
/* JSON Response */
header('Content-type: application/json');
echo json_encode($data, JSON_PRETTY_PRINT);
?>

reset-password.php

<?php
require_once 'connect.php';
session_start();
$type_json = true;
if($_SERVER['REQUEST_METHOD'] === 'GET') {
 if(isset($_GET['email']) && !empty($_GET['email']) && isset($_GET['hash']) && !empty($_GET['hash'])) {
 $email = htmlentities(mysqli_real_escape_string($connection, trim($_GET['email'])));
 $hash = htmlentities(mysqli_real_escape_string($connection, trim($_GET['hash'])));
 $search_query = "SELECT email, hash, status FROM users WHERE email = '{$email}' AND forgot_password_hash = '{$hash}' AND
 status = '1'";
 $do_search_query = mysqli_query($connection, $search_query);
 if($do_search_query) {
 $count_rows = mysqli_num_rows($do_search_query);
 if($count_rows > 0) {
 $_SESSION['email'] = $email;
 $_SESSION['hash'] = $hash;
 $type_json = false;
 echo "<form method='post' action='do-reset.php'><input type='password' name='password'><br><input type='submit' value='Reset My Password'></form>";
 }
 else {
 $data = array("result" => -3, "message" => "Invalid URL or Perhaps The Password Has Already Been Reset Using This Link!");
 }
 }
 else {
 $data = array("result" => -2, "message" => "Something Went Wrong! Try Again Later.");
 }
 }
 else
 {
 $data = array("result" => -1, "message" => "Certain Request Parameters Are Missing!");
 }
}
else {
 $data = array("result" => 0, "message" => "Incorrect Request Method!"); 
}
mysqli_close($connection);
/* JSON Response */
if($type_json) {
 header('Content-type: application/json');
 echo json_encode($data, JSON_PRETTY_PRINT);
}
?>

update-password.php

<?php
require_once 'connect.php';
session_start();
$type_json = true;
if($_SERVER['REQUEST_METHOD'] === 'GET') {
 if(isset($_GET['email']) && !empty($_GET['email']) && isset($_GET['hash']) && !empty($_GET['hash'])) {
 $email = htmlentities(mysqli_real_escape_string($connection, trim($_GET['email'])));
 $hash = htmlentities(mysqli_real_escape_string($connection, trim($_GET['hash'])));
 $search_query = "SELECT email, hash, status FROM users WHERE email = '{$email}' AND forgot_password_hash = '{$hash}' AND
 status = '1'";
 $do_search_query = mysqli_query($connection, $search_query);
 if($do_search_query) {
 $count_rows = mysqli_num_rows($do_search_query);
 if($count_rows > 0) {
 $_SESSION['email'] = $email;
 $_SESSION['hash'] = $hash;
 $type_json = false;
 echo "<form method='post' action='update-password.php'><input type='password' name='password'><br><input type='submit' value='Reset My Password'></form>";
 }
 else {
 $data = array("result" => -3, "message" => "Invalid URL or Perhaps The Password Has Already Been Reset Using This Link!");
 }
 }
 else {
 $data = array("result" => -2, "message" => "Something Went Wrong! Try Again Later.");
 }
 }
 else
 {
 $data = array("result" => -1, "message" => "Certain Request Parameters Are Missing!");
 }
}
else {
 $data = array("result" => 0, "message" => "Incorrect Request Method!"); 
}
mysqli_close($connection);
/* JSON Response */
if($type_json) {
 header('Content-type: application/json');
 echo json_encode($data, JSON_PRETTY_PRINT);
}
?>

• Zulan: I don't understand why this was downvoted without any constructive comments for improvements? Seems like a valid question to me. (I don't see downvotes directly but I saw it at -2 yesterday)

Answer 1

Security

Your code should be secure.

But just putting all user input through htmlentities and mysqli_real_escape_string is not really the recommended approach.

The recommended approach against SQL injection are prepared statements.

The recommended approach against XSS is htmlentities, but it should be applied when echoing data, not when reading it for the first time. There are two reasons for this:

1. It is hard to keep track which variables are save to echo and which are not. This can very easily lead to bugs.
2. You get dirty data, which can hurt usability. For example, what if my email address is foo&bar'[email protected] ? I would get foo&bar\'[email protected] , and would thus not get an email.

Apart from that, it is customary to expire forgotten-password-links. The reason for this is that an attacker who can read data from the database can get into users accounts without having to decrypt the passwords.

Nesting

Your code is too deeply nested, making it hard to read. If you have 6 closing elses in a row, it is really hard to see which if they actually close.

You can reduce nesting by reversing your ifs:

if($_SERVER['REQUEST_METHOD'] === 'GET') {
 $data = array("result" => 0, "message" => "Incorrect Request Method!"); 
} else {
 [...]
}

I think it's nicer to just introduce functions and return:

function emailFormSubmittion($email) {
 if($_SERVER['REQUEST_METHOD'] === 'GET') {
 return array("result" => 0, "message" => "Incorrect Request Method!"); 
 } 
 if(!empty($_GET['email'])) {
 return array("result" => -1, "message" => "No Email Address Provided!"); 
 }
 [...]
}

Misc

• You don't need to check isset if you use empty.
• You have quite a lot of unnecessary newlines.
• I think the structure of your code is a bit confusing. The naming doesn't really reveal what the responsibility of each file is (eg what's the difference between reset and do reset?), and each file does a lot of different things (send emails, read userdata, create forms, read from database, write to database, manage session, output data, etc).

• Sol: isset must be used in php 7+, !empty doesn't work anymore
• James: @Sol Do you have any source for this? !empty() should work fine in all PHP versions since PHP 4
• Sol: I get an error if I don't use isset(). I only used !empty() before PHP 7 without an issue. Now every statement forces me to put isset() instead. May be a bug, an annoying one.

Answer 2

tim's answer covers the most important issues, but I would like to put a bit more emphasis.

First of all you mastered the first step towards secure code. You obviously know that you need to sanitize user input, that is already very good.

Input sanitation

However, while your code may not have sanitation issues right now, it probably will not be once you extend/modify it. Arbitrarily throwing sanitation methods at input is not the solution. The solution is to use clean abstraction interfaces for output (e.g. template system) and database (e.g. prepared statements) and sanitize at the input to the abstraction. This way it is more feasible to actually get it right for non-trivial sites.

Randomness / hash

mt_rand is not suitable for security-related randomness. This, and suggested alternatives, are also documented. Note: It seems the alternatives suggested by the php manual are not easily available... [insert php rant here].

The arbitrary restriction to ~30 bits (1,000,000,000) is certainly not helpful. There is also no reason to hash this value here, and most certainly not by chaining two weak hash functions. Remember that hash functions map arbitrary size data to fixed size. They do not generate entropy. You can use a hash function if you have non-alphanumeric random data.

So lets say you generate a random token of sufficient entropy (30 bites are NOT!) and send that token to the user. Then you can store the hash of the token in the database. This way, when an attacker obtains a database dump somehow, he cannot reset passwords. This works the same way as with passwords, although its more important to apply it to passwords. You can use password_hash.

Protect against phishing

Emails with links are in general problematic, as both links (what is shown to the user vs. where the link goes to) and emails (sender address) can be easily manipulated. If you send an email to the user, especially when it contains a link to click on, it is customary to personally address him by name. This is at least some assurance, that it is not a phishing mail. It is of course not perfect as the name can often be devised from the email address.

An alternative to sending a link, is to send only the token (in your case hash) by mail and redirect the user to a form where he can enter the token directly after he requests the email.

Code structure

As Tim mentioned, your code structure should be improved for clarity. This also has security impact.

SQL efficiency

If you do not intend to use the result of an SQL row, don't SELECT * it, but SELECT COUNT(*) instead.

Use example.com

The domain example.com is reserved for this very use. Use it instead of a domain that is actually owned and used (domain.com).