Explaining a bad approach - not comparing hashes

Question

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

Answer 1

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.