My lost password function

Question

I'm trying to create a lost password function on my site. When a user enter their email it updates 2 columns in the users table with that email called: new_password_key and new_password_requested (int, using PHP time()), I then send them a link with that password key to a page where they may change their password.

It seems to work, and if they don't visit the link within 24 hours it's considered invalid and they have to request a new one.

Is it OK? Safe? Is there something else I should think of?

This is the https://mysite.com/password/key_goes_here page:

$value = isset($_GET['key']) ? $_GET['key'] : null;
$query = $db->prepare("SELECT new_password_key, new_password_requested FROM users WHERE new_password_key = ?");
$query->execute(array($value));
$row = $query->fetch();

if (!$row) {
    // not found
    header("Location: /password");
    die;
}

// if this key is older than 24 hours its invalid (expired)
if ($row['new_password_requested'] <= time() - (60 * 60 * 24)) {
    $query = $db->prepare("UPDATE users SET new_password_key = NULL, new_password_requested = 0 WHERE new_password_key = ?");
    $query->execute(array($value));
    header("Refresh: 3;url='/password'");
    return false;
} 


if (count($_POST) > 0) {

    $new_password1 = trim($_POST['new_password1']);
    $new_password2 = trim($_POST['new_password2']);

    if (strlen($new_password1) < 4)
        $error = "passwords must be at least 4 characters long. please choose another (longer) password";
    else if ($new_password1 != $new_password2)
        $error = "passwords do not match";

    if (empty($error)) {
        $hash = password_hash($new_password1, PASSWORD_DEFAULT);
        $query = $db->prepare("UPDATE users SET password = ?, new_password_key = NULL WHERE new_password_key = ?");
        $query->execute(array($hash, $row['new_password_key']));

    }

}

This is the form where you enter your email to get a key:

if (count($_POST) > 0) {

$email = trim($_POST['email']);

$query = $db->prepare("SELECT email, new_password_key, new_password_requested FROM users WHERE email = ?");
$query->execute(array($email));
$row = $query->fetch();

if (empty($email))
    $error = "this field can not be empty";
else if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    $error = "not a valid email address";
else if (!empty($row['new_password_key']))
    $error = 'a link has already been sent';
elseif ($query->rowCount() < 1)
    $confirmed = true;


if (empty($error)) {

    if ($row) {
        // email exists, send them mail
        $query = $db->prepare("UPDATE users SET new_password_key = ?, new_password_requested = ? WHERE email = ?");
        $query->execute(array(generateRandomString(25), time(), $row['email']));
        sendMail($email, 'lostpassword');
        $confirmed = true;
    } else {
       // email dont exist, tell them an email has been dispatched but dont actually send a mail we dont want them to know what emails are registered on this site
       $confirmed = true;
    }
}
}

Answer 1

This seems quite fine. It's good that you are using prepared statements for all database queries.

What bugs me the most is that the users table is reused for multiple purposes:

1. Store essential user information such as email and password
2. Store information about tracking reset password requests

It would be better to use another table for the 2nd purpose. The reason is that the users table is an essential piece of your site and you wouldn't want anything to happen to it by accident. So the less you touch that table, the better. Besides, for most users the new_password_key and new_password_requested fields will be blank / unused most of the time.

---

The variable names are sometimes not so great. For example:

$value = isset($_GET['key']) ? $_GET['key'] : null;

$value is a key, so I suggest to name it accordingly.

---

One last small thing, I recommend to use braces with all if, else if statements.

Answer 2

I found a few minor issues. Amongst them, are not exiting early (performance enhancement: exit before the database query happens). Additionally, you're not handling certain errors. Lastly, your lack of braces is disturbing, as it can lead to bugs if the code is ever modified incompetently. I've left my comments within the code as #### COMMENT.

Below are the commented files, assuming Key.php for the key page and Forms.php for the forms page as pseudonames:

Key.php

$value = isset($_GET['key']) ? $_GET['key'] : null;
$query = $db->prepare("SELECT new_password_key, new_password_requested FROM users WHERE new_password_key = ?");
$query->execute(array($value));
$row = $query->fetch();

if (!$row) {
    // not found  #### COMMENT NOT NECESSARY, CODE IS EXPLANATORY
    header("Location: /password");
    die;
}

# If this key is older than 24 hours its invalid (expired)
if ($row['new_password_requested'] <= time() - (60 * 60 * 24)) {
    $query = $db->prepare("UPDATE users SET new_password_key = NULL, new_password_requested = 0 WHERE new_password_key = ?");
    $query->execute(array($value));
    header("Refresh: 3;url='/password'");
    return false;
}

if (count($_POST) > 0) {
    $new_password1 = trim($_POST['new_password1']);
    $new_password2 = trim($_POST['new_password2']);

    if (strlen($new_password1) <= 4) {
        $error = "Passwords must be at least 4 characters long. Please choose a longer password";
    } else if ($new_password1 != $new_password2){
        $error = "Passwords do not match";
    }

    if (empty($error)) {
        $hash = password_hash($new_password1, PASSWORD_DEFAULT);
        $query = $db->prepare("UPDATE users SET password = ?, new_password_key = NULL WHERE new_password_key = ?");
        $query->execute(array($hash, $row['new_password_key']));
    } else {
        #### You're not handling if $error is not empty, do so here
    }
} else {
    #### You're not handling if count($_POST) == 0, do so in this else block
}

Forms.php

$email = trim($_POST['email']);

#### Move these checks up here, before you do the database query so you exit early.
if (empty($email)) {
    $error = "Email field is empty";
} else if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
    $error = "Not a valid email address";
}

#### You might want to handle the error here, and exit before you do the query

$query = $db->prepare("SELECT email, new_password_key, new_password_requested FROM users WHERE email = ?");
$query->execute(array($email));
$row = $query->fetch();

if (!empty($row['new_password_key'])) {
    $error = 'A link has already been sent';
} 

if ($query->rowCount() < 1) {
    $confirmed = true;
}

if (empty($error)) {
    if ($row) {
        // email exists, send them mail #### COMMENT NOT NECESSARY, CODE IS EXPLANATORY
        $query = $db->prepare("UPDATE users SET new_password_key = ?, new_password_requested = ? WHERE email = ?");
        $query->execute(array(generateRandomString(25), time(), $row['email']));
        sendMail($email, 'lostpassword');
        $confirmed = true;
    } else {
        // email dont exist, tell them an email has been dispatched but dont 
        // actually send a mail we dont want them to know what emails are registered on this site

        #### This is an absolutely horrible way to do this. Just implement a rate limiter (3 emails per IP address).
        #### Reason being, they can just check if an email has been received for email [email protected] but not for [email protected]
        $confirmed = true;
    }
}

Answer 3

User generated passwords?

I always find it a security risk if users select their own passwords. Especially when passwords like '1111' are accepted by the server, as they seem to be in your case.

Letting users choose their own passwords is dangerous. They will often choose something they can remember easily, and others can guess easily. Users also don't like making up new passwords for every site, so they don't.

There is another way: You generate a secure password for the user.

The complexity of the password depends on what it is protecting. A password of five numbers can have 100000 variations, and is already difficult to guess. Add in a few letters and the variantions explode into the billions.

Present the password on an SSL secured webpage, once, if you want to be secure. You can send it by mail, but it's obvious that mail is not secure at all.

The other thing you need to work on is the login page. You don't show this page in your question, but it should not allow a brut force attack. Users can try to log in a few times, and if they fail consistently access should be denied.

I'm sorry, that this is a somewhat chaotic answer, and I don't even discuss your code in detail. The point is that user generated passwords are a stupid standard, and we should not use it. The weakest point in the security of a site should not be the password itself, and don't blame the user for this, it's a design problem.