Grabbing hashed password from a database

I have just discovered the beauty of prepared statements in PHP and how they protect against SQL injection. In my first time using them, I am attempting to grab a hashed password from a database and seeing if it matches up against the password the user inputted into the login form.

Is this the most efficient way to:

• Grab the data from the database?

• Match the passwords using password_verify()?

stmt_init();
$stmt2 = $conn->prepare("SELECT apassword FROM login WHERE auser = ?"); 
$stmt2->bind_param("s", $user);
$stmt2->execute();
$stmt2->store_result();
$stmt2->bind_result($hashedPassword);
$stmt2->fetch();
//echo $hashedPassword; this returns hashed password string from DB
$numberofrows = $stmt2->num_rows; //this is an integer!!
$stmt2 -> close();

if($numberofrows > 0) //if username exists in database 
{ 
    if (password_verify($password, $hashedPassword)) //if user-inputted password (from form) equals hashed password from DB...
    {
        print("Password is valid, login successful!");
        session_start();
        $_SESSION['sess_user'] = $user;
        header("location:member.php"); //redirect user to member page
    } 
    else 
    {
        echo 'Invalid password for the username: ' . $user; //if password didn't match DB, we tell them
    }
}
else
{
    echo 'The username, ' . $user . ', does not exist! Please try again.'; //if num_rows is 0, we know username doesnt exist
}
?>

Regarding your questions, yes, that looks fine to me.

XSS

When echoing user input - or really any variable data - you need to protect against XSS attacks. Otherwise, attackers can inject JavaScript code, which will then be executed in the context of the browser of the victim, leading to bypass of CSRF protection, cookie stealing, phishing, or injection of JavaScript keyloggers.

Specifically, these statements are vulnerable:

echo 'Invalid password for the username: ' . $user;
echo 'The username, ' . $user . ', does not exist! Please try again.';

You can defend against XSS by using htmlspecialchars($string, ENT_QUOTES, 'UTF-8'); when echoing data.

Nesting

Personally, I don't like these kinds of nested ifs. It's hard to see which if clause the different else statements close. With just two, it's still manageable, but when the code is extended, it will become very difficult to read.

A different structure might look like this:

if($numberofrows <= 0) { 
    // return false or echo and die
}

if (!password_verify($password, $hashedPassword)) {
    // return false or echo and die
}

// return true, or start session, redirect

Now, it's a lot clearer which error message results from which check.

Misc

• it's good practice to regenerate the session id when the state of the session changes to prevent session fixation (not an issue with default php.ini, but with some settings it is an issue). Use session_regenerate_id(true); for this.

• using relative URLs in the Location header violates current standards, so if you can, use absolute URLs.

• your comments don't really add any clarity to the code, so I would just remove them.

• in PHP, it is generally standard to put curly brackets of if/else statements on the same line.

• do you really need the call to store_result? It doesn't seem necessary to me.

• your spacing is not internally consistent (eg $stmt2 -> close(), if().

• I would either use print or echo, but not both for the same task.

• as a security precaution, you should always die after a redirect as clients do not have to follow it, and thus code below the redirect can be executed as well. In this case, it doesn't matter because the redirect is for a successful login, but it's good practice.

StackExchange Code Review Q#101396, answer score: 3