2

Today I saw a snippet a code for authentication logic :

function checkPass($pass, $login){
    $inputedPass = $pass;

    $req       = $bdd->prepare('SELECT password FROM agents WHERE password = MD5(?) AND ID = ?;');
    $req->bindParam(1, $inputedPass, PDO::PARAM_STR);
    $req->bindParam(2, $login, PDO::PARAM_STR);
    $req->execute();
    $result2   = $req->fetch();

    if (empty($result2)) {
        $req->closeCursor();
        return false;
    } else {
        $req->closeCursor();
        return true;
    }
}

Ignoring the MD5 part, which we know is outdated, this code doesn't check if your password is the right one (as in, comparing the hash of the password you entered with the hash in the database), it checks if there's a user with your name and the same password.

I changed this to allowing access on equal hashes instead, but I've been told "it's the same".

How do I explain it's not? What are the specific risks with his approach?

[EDIT]

Following @Marc's answer, here's how i updated this function :

checkPass($pass, $login){
    $inputedPass = $pass;

    $req       = $bdd->prepare('SELECT password FROM agents WHERE Id = ?;');
    $req->bindParam(1, $login, PDO::PARAM_STR);
    $req->execute();

    $result2   = $req->fetch();
    $req->closeCursor();

    if(isset($result2["password"]){
        if (password_verify($inputedPass , $result2["password"])) {
            return true;
        }
    } else {

        sleep(rand(40, 100) / 100); //measly attempt to mitigate side channel timing attack
        return false;
    }
}

Which i realized after reading the comments, doesn't handle the case when the user doesn't exists (i assume it will crash). However, it doesn't sends plain passwords nor relies on MD5.

Thank you

  • 2
    Ultimately, you want to see if there is a user with the same username and password. In this case, it's the same (login, md5(password)) pair. The main distinction between checking for (login, md5(password)) vs fetching stored_md5_of_password for login and comparing it with md5(password) is that you can't tell the difference between non-existing user vs wrong password (this is usually not something you want to leak anyway). The nice thing about doing it this way is that this is constant time (modulo the DB lookup time) unlike your solution which is faster when the user does not exist. – Marc Jun 4 at 9:18
  • 1
    But yes, using md5(password) alone marks this as horrible. – Marc Jun 4 at 9:18
  • ahah so it has a builtin side-attack protection in the end. thank you for your answer – Orsu Jun 4 at 9:24
  • Sort of. I've moved my last comment (and expanded on the other) into an answer. – Marc Jun 4 at 9:45
3

Assuming the value stored in the agents.password field is the MD5 hash of the password, the code as shown is correct:

  • sends the database the (login, password) pair
  • asks it to find a matching entry where agents.ID == login and agents.password = MD5(password) and return the value of the agents.password field
  • if the returned value is empty: return an error (I'm not sure about the dialect, so it could be empty as in "no result", or empty as in "empty password field". Let's assume the former).

The good points:

  • the behavior is correct: it returns true if and only if both the username and password match, and an error otherwise. This is good security behavior in the sense that it does not leak information about the existence of the user (eg: you cannot tell the difference between "user A does not exist" and "wrong password").
  • timing is similar: the time spent in the function will not differ much depending on whether the user exists or not, which avoids leaking that information (though it depends on the behavior of the query itself).

The bad points:

  • a single round of MD5 is a terrible way to hash a password. You want a much more expensive password hashing function as well as a random salt and possibly even a secret pepper
  • this is sending the plaintext password to the database, using builtin DB functions to perform the MD5 hash. This introduces another location where the plaintext password can be logged (eg: data audit logs) and leaked.

Compared to your proposal:

If I understand your proposal correctly, you would prefer to do something along the lines of the following:

hashedPassword = query(select password where ID = login)
hash = md5(password)
if hash != hashedPassword {
    fail
} else {
    success
}

The end result (in terms of input/output) is exactly the same. But looking through the good and bad points, the differences are:

  • you are not sending the plaintext password to the DB. That's a plus in my book
  • you may or may not be much faster when the user does not exist (it depends if you exit early if the user does not exist)

Conclusion:

The two approaches are mostly similar, and the little details I pointed out are nothing compared to the fact that MD5 is used. That is the biggest issue that should be resolved as soon as possible.

| improve this answer | |
  • @Orsu: in your snippet, you definitely want to check whether the user exists. If the hashing function is expensive, you also want to do it before returning (even if the user doesn't exist). You say it does not rely on MD5. Do you mean the database builtin function, or at all? Unless you change what the DB stores, you'll still need to use MD5 hashes. – Marc Jun 4 at 10:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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