2

I have a bit of code here that returns "Username is not correct" when I input it, when it's supposed to output "Incorrect password," because I'm simply putting in a correct username, but an incorrect password.

if ($_POST['login']) {
    $username = strip_tags($_POST['username']);
    $password = strip_tags($_POST['password']);
    $fetchme = $dbc->query('SELECT * FROM users WHERE username="$username"');
    while($row = $fetchme->fetch(PDO::FETCH_ASSOC)) {
        $lastlogin = $row['lastlogin'];
    }
    if (!$username||!$password) {
        echo '<center>Please enter a valid username and password</center>';
    }else{
        $login = $dbc->query('SELECT * FROM users WHERE username="$username"');
        $num_rows_login = ($login->fetchColumn() > 0) ? true : false;
        if ($num_rows_login == 0) {
            echo "<center>Username doesn't exist</center>";
        }else{
            while($login_row = $login->fetch(PDO::FETCH_ASSOC)) {
                $password_db = $login_row['password'];
                $password_db2 = hash('sha512', $password);
                if ($password_db2 != $password_db) {
                    echo '<center>Incorrect password</center>';
                }
            }
        }    
    }
}

This is my first time using PDO. Any help is appreciated!

Ja͢ck
  • 170,779
  • 38
  • 263
  • 309
SDCore
  • 23
  • 4
  • Your SQL syntax is wrong. You should get an error message. If you are using PDO, also utilize prepared statements. – mario Jan 23 '13 at 03:09
  • 3
    Don't do `strip_tags()` on a password; that doesn't make sense, especially because you would hash it before storing anyway. And why are you repeating the query? – Ja͢ck Jan 23 '13 at 03:15

1 Answers1

0

First of all, don't use strip_tags() on usernames or passwords; they don't serve any purpose and you should just be escaping them properly in SQL (or use prepared statements).

Secondly, you can assume that at most one row will be returned from the users table, so you really don't need a while to fetch all records.

Thirdly, don't make a distinction between a non-existent username and invalid password; they should both yield the same message "invalid username or password".

Here's a quick rewrite:

$stmt = $dbc->prepare('SELECT * FROM users WHERE username=?');
$stmt->execute(array($username));
// fetch all records and take first one (returns false in case of no rows)
$user = current($stmt->fetchAll(PDO::FETCH_ASSOC));
// validate record
if ($user === false || hash('sha512', $password) !== $user['password']) {
    echo '<center>Invalid username or password</center>';
} else {
    echo 'yay, you are the man';
}

Lastly, read up on this for better password hashing:

How do you use bcrypt for hashing passwords in PHP?

Community
  • 1
  • 1
Ja͢ck
  • 170,779
  • 38
  • 263
  • 309