Solved

Review my login function before I open a pull request?

Asked by Oskar Brandt · 314 views · 3 replies

Oskar Brandt5 messages

This is for an old internal tool we are moving to Laravel. It works, but I would rather hear what is wrong with it here than in code review:

function login($email, $password)
{
    $user = DB::select("SELECT * FROM users WHERE email = '$email'")[0];

    if ($user->password == md5($password)) {
        session(['user_id' => $user->id]);
        return true;
    }

    return false;
}

3 replies

Priya NairSolution

Good call asking first. There are four problems, roughly in order of how bad they are.

  1. SQL injection. $email goes straight into the SQL. An email of ' OR '1'='1 logs in as the first user in the table. Use bindings: DB::select('... WHERE email = ?', [$email]), or better, Eloquent.
  2. MD5 is not password hashing. It is fast by design, so leaked hashes can be cracked by the billion per second. Use Hash::make() to store and Hash::check() to verify. They use bcrypt or argon2 with a salt and a cost factor.
  3. == on hashes. Loose comparison treats two strings that look like numbers in scientific notation, such as "0e1234...", as equal. Some MD5 hashes start that way, so a few different passwords can match each other. Hash::check uses a proper constant-time comparison.
  4. [0] on no result. An unknown email gives an empty array, then a warning, then an error when you read ->password on null.

The Laravel way handles all four:

if (Auth::attempt(['email' => $email, 'password' => $password])) {
    request()->session()->regenerate();
    return true;
}

return false;

The regenerate() call issues a new session ID at login, which prevents session fixation.

Link
Tomasz Wiśniewski

For the migration itself: you cannot convert existing MD5 hashes into bcrypt, because you do not have the passwords. The usual route is to check the old MD5 once on each user's next login and, if it matches, immediately store Hash::make($password) and clear the old value. After a few months, force a reset for whoever never logged in.

Link
Oskar Brandt

Well, I am glad I asked here first. Rewritten with Auth::attempt and the rehash-on-login plan, and I have added a test for the unknown-email case.

Link