PHP password-resetting page

Question

I am producing a PHP code and I want to implement it in my server to do its function. My question is, are there any vulnerabilities that I should correct?

I tried to establish the session first then I will initiate a SQL connection and I will ask the user to enter his email. Also I added the reset function of his email.

<?php
if ($_SESSION['admin'] !== true) {
    header('Location: /login.php?');
}

$conn = new mysqli("localhost","admin","password","db");

if ($conn->connect_error) {
    die("Connection failed: " . $conn->connect_error);
}

$msg = '';
if (isset($_POST['submit'])) {
    $email = $_POST['email'];
    $sql = "SELECT * FROM users WHERE email = $email;";
    $result = @$conn->query($sql);
    if ($result->num_rows == 0) {
        die("This email: $email is incorrect");
    }
    $row = $result->fetch_assoc();
    $id = $row["id"];
    $surename = $row["surename"];

    $sql = "SELECT * from users JOIN password_resets ON (password_resets.user_id=users.id)  WHERE id=$id;";
    $result = @$conn->query($query);
    if ($result->num_rows == 0) {
        $insert_sql = "INSERT INTO password_resets (user_id, reset_timestamp) VALUES ($id, unix_timestamp());";
        if ($conn->query($insert_sql) !== TRUE) {
            die("Error: " . $insert_sql . "<br>" . $conn->error);
        }
    }
    $result = $conn->query($query);
    $row = $result->fetch_assoc();

    $link = "https://".$_SERVER['SERVER_NAME']."/reset.php?id=".$row['id'] ."&timestamp=".$row['reset_timestamp'];
    $message = "Hi, $surename,

    Here's your password reset link: $link";
    $headers = "From: ".$_SESSION['first_name']." ".$_SESSION['last_name']." <".$_SESSION['email'].">";
    mail($email,"Your password reset link",$message,$headers);
    $msg = "Password reset for $email sent!";
?>
<html>
<head><title>Reset password for a user</title></head>
<body>
  <form>
    <div class="message"><?=$msg ?></div>
    <input type="text" width=80 name="email">
    <input type="submit" name="submit" value="Send">
  </form>
</body>
</html>
<?php
}

Suppressing errors when you do queries is not a good idea. Establish proper error handling so you are aware of and can fix problems. Don't use something like $result = @$conn->query($sql);. — Dave, Commented Feb 16, 2019 at 13:32

John Conde · Accepted Answer · 2019-02-16 00:20:32Z

Your code is wide open to SQL Injection which is the most dangerous web vulnerability. This occurs when you execute SQL queries with unsanitized user data. By placing the raw $_POST variable directly in your query you are allowing an attacker to inject their own SQL into your query and execute it. They can do anything from stealing data to deleting your database.

To combat this you must use parameterized queries. Stack Overflow covers this very well but here's what your code would like if you use mysqli with prepared statements:

$stmt = $dbConnection->prepare('SELECT * FROM users WHERE email = ?');
$stmt->bind_param('s', $email); // 's' specifies the variable type => 'string'

$stmt->execute();

$result = $stmt->get_result();
$row = $result->fetch_assoc();

You also are wide open to Cross-site scripting (XSS) attacks which is the #7 web vulnerability. This occurs because you take raw $_POST data and output it directly into your HTML. An attacker can place malicious code in this value and attack your users and site once it is rendered by the browser.

When outputting user data, always escape that data. Stack Overflow covers this as well. In PHP you can do this by using htmlspecialchars().

<?= htmlspecialchars($msg, ENT_QUOTES, 'UTF-8'); ?>

You will notice that the one $_POST variable, $_POST['email'], has left your site wide open to attack. Before you even attempt to use it you should validate it indeed is a valid email address. If it is not, you should report an error and not attempt to use it as it obviously is invalid and useless anyway.

PHP offers an easy way to validate an email addresses. filter_var() with the FILTER_VALIDATE_EMAIL flag will validate if an email address is valid.

$email = $_POST['email'];
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
    // email is invalid. report error and abort.
}

On a different note, you start your script off by checking to see if a user is an admin. If they are not you use header() to do a redirect away from that page. That's usually okay but you should follow it with a call to exit() to ensure the script stops executing. If not, the code below may still execute and, combined with other vulnerabilities in the page, leave you open to attack.

if ($_SESSION['admin'] !== true) {
    header('Location: /login.php?');
    exit;
}

Your Common Sense · Accepted Answer · 2019-02-16 14:38:09Z

To be honest, this code could be used as a teaching aid to demonstrate every possible mistake that could be made in a PHP script.

Beside problems already mentioned in the other answer, there is a truckload more.

A database connection.

It is not your fault, you are following just a terrible outdated example still shown in the PHP manual. Yet, even such a familiar task must be done properly:

the proper error reporting mode for mysqli must be set
the proper charset must be set
the improper error reporting must be wiped from the code. You don't want to show the system error message to every site user.

I even wrote a distinct article, how to use mysqli_connect properly, because, to be honest, there is not a single good example on the whole Internet.

Not to mention that you shouldn't write the same database connection script on the every PHP page. Instead, put it in a file, and then only include this file.

A non-working query

A query like this $sql = "SELECT * FROM users WHERE email = $email;"; will never return any result, simply because its syntax is wrong. This is another reason to use prepared statements.

A flawed error reporting.

To let you know, @ symbols is a pure evil that will make your life a nightmare on the long run. Errors must be fixed, not swept under the rug! And in order to fix the error you must be aware of its existence. While @ operator will leave you totally ignorant.

Superfluous queries

In this code you are running three SELECT queries to the same table. Which is just illogical. Why do you need a $sql = "SELECT * FROM users WHERE email = $email;"; if on the second line you will run almost identical query?

All you need is to use LEFT JOIN in your second query and it will happily serve for both tasks.

And after inserting into password_resets table you are trying to run the second query again. Which is also superfluous, if you care to define the reset timestamp in the script.

The code refactored.

To sum everything up, here is the database interaction part of your code slightly refactored:

db.php:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try {
    $conn = mysqli_connect("localhost","admin","password","db");
    mysqli_set_charset($mysqli, "utf8mb4");
} catch (\mysqli_sql_exception $e) {
    throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
}

password reset script (database interaction part)

require 'db.php';
$msg = '';
if (isset($_POST['submit'])) {
    $sql = "SELECT * from users 
            LEFT JOIN password_resets ON (password_resets.user_id=users.id)
            WHERE email=?";
    $stmt = $conn->prepare($sql);
    $stmt->bind_param("s", $_POST['email']);
    $stmt->execute();
    $row = $stmt->get_result()->fetch_assoc();

    if (!$row) {
        $msg = "This email: $email is incorrect");
    } else {
        if (!$row['reset_timestamp']) {
            $sql = "INSERT INTO password_resets (user_id, reset_timestamp) VALUES (?, ?);";
            $stmt = $conn->prepare($sql);
            $time = time();
            $stmt->bind_param("ii", $row['id'], $time);
            $stmt->execute();
            $row['reset_timestamp'] = $time;
        }
        $link = ... // here goes your email sending business
    }
}

Overall inconsistency

There are also other issues, like use of $_SESSION['first_name'] and $_SESSION['last_name']. I was under the impression that a password reset code is called when a user do not remember their password and therefore cannot login. In this case I am wondering why there will be their name in the session.

The same confusion is related to the topmost condition: why access to the password reset page should be restricted for the admin use only? In my understanding it must be available to any site user, or otherwise it will be of no value at all.

Stack Exchange Network

PHP password-resetting page

2 Answers 2

A database connection.

A non-working query

A flawed error reporting.

Superfluous queries

The code refactored.

Overall inconsistency

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
security
authentication
email
mysqli
or ask your own question.

Hot Network Questions

PHP password-resetting page

2 Answers 2

A database connection.

A non-working query

A flawed error reporting.

Superfluous queries

The code refactored.

Overall inconsistency

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
security
authentication
email
mysqli
or ask your own question.