password_verify fails in an if statement

Question

EDIT: I guess the stars just fell out of alignment, because after reloading the page and trying to use it again it works.

I'm creating a simple administration system with user accounts. I'm having a hard time with the login bit, because for some reason password_verify fails if it's in an if statement. It might be related to my method of getting the passwords out of the database, but I'm not sure.

here's what doesn't work

$query = $db->query("SELECT * FROM users WHERE `username` = '" . $db->real_escape_string($_POST['username']) . "';");

if (!$query or $query == NULL)
{
    echo $db->error . "<br>";
    exit("EXIT: database error");
}

while ($row = $query->fetch_assoc())
{
    if (password_verify($_POST['password'], $row['password']))
    {   
        $db->close();
        header("LOCATION: main.php");
        exit;
    }
    else
    {
        header("LOCATION: index.php?error=Wrong password.");
        exit("wrong password<br>");
    }
}

if I were to replace the line if (password_verify($_POST['password'], $row['password'])) with

$success = password_verify($_POST['password'], $row['password']);
if ($success)
// success

it works as expected. is there something wrong with me doing ($row = $query->fetch_assoc()), or did I make a mistake somewhere else? I don't understand why it won't work in the IF statement. I'm sure I just did something stupid, but I cannot for the life of me find out what.

Would it be bad, or considered bad for me to use the latter method for password authentication?

_Side note:_ In this line. `if (!$query or $query == NULL)`, the `or` part isn't needed. Just have `if (!$query)` is enough since it would cover `null` as well (it's falsy). — M. Eriksson, May 27 '18 at 20:51
@MagnusEriksson I thought about that, but I wasn't sure, and it's everywhere in the codebase (which is very old) so I didn't want to remove it just yet. — kennyrkun, May 27 '18 at 20:56
There's no difference between having `password_verify()` in an `if`-statement or storing the result in a variable and check that instead. If you get a difference, there must be something else wrong, like you passing the wrong password or similar. — M. Eriksson, May 27 '18 at 21:00

score -1 · Accepted Answer · answered May 27 '18 at 21:31

I seem to have fixed the problem somewhere in here:

if (!empty($_POST['password']))
{   
    while ($row = $query->fetch_assoc())
    {
        if (password_verify($_POST['password'], $row['password']))
        {                   
            $db->close();
            header("LOCATION: main.php");
            exit("EXIT: Logged in successfully.");
        }
        else
        {               
            header("LOCATION: index.php?error=Wrong password.");
            exit("EXIT: Wrong password");
        }
    }
}

I don't think it was the actual code that was the problem, but my databases. I don't know if it has anything to do with it, but phpmyadmin is showing duplicates for all my rows. I'm still very new to using SQL (today) so I don't know if that's normal. Maybe I just needed to give it time to update the tables? Who knows.

score -2 · Answer 2 · answered May 27 '18 at 20:40

-2

Make sure to sanitize your inputs, never simply use $_POST['password'], but use a sanitize filter instead.
For me your first block of code works just fine and your second one has mismatched parenthesis.

answered May 27 '18 at 20:40

Armin

168
1
1
15

1

Parameterizing should be in used over sanitizing. The `mysqli` function would sanitize the input, but it has known flaws which is why the parameterizing is better. – user3783243 May 27 '18 at 20:42
1

...and why would you sanitize `$_POST['password']` when it's never sent to the database or echoed? It's only passed to `password_verify()`, which is perfectly OK to do without any sanitation/escaping. It actually _shouldn't_ be sanitized/escaped since that would actually _change_ the password. – M. Eriksson May 27 '18 at 20:44
1

Don't post comments as answers. If you don't have enough rep to comment, work on that first. Answers are reserved for actual _solutions_. Commenting is a privilege and trying to bypass the rep restrictions like this is _not_ a good way to earn rep, rather the opposite. – M. Eriksson May 27 '18 at 20:53
@user3783243: No mysqli_real_escape_string() does not have flaws it just makes it possible to do REALLY STUPID things. OTOH parameter binding can break function calls by implicit casting and doesn't support variable lists. – symcbean May 27 '18 at 20:54
@symcbean - _"No mysqli_real_escape_string() does not have flaws"_ - Yes, it does... sure it's in edge cases but they do exist: https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string/12118602#12118602 – M. Eriksson May 27 '18 at 20:57
@symcbean I guess I should have phrased the issue with the `escaping` differently (and called it escaping, not `mysqli`). Are you playing devil's advocate or do you really believe parameterizing is not a better approach? – user3783243 May 27 '18 at 20:59
@Magnus: like I said, it doesn't prevent you doing really stupid things. Would you really fall on your keyboard and implement a charset change bypassing the API, employing one of the three vulnerable character sets by accident? – symcbean May 29 '18 at 08:32

password_verify fails in an if statement

2 Answers2