3
\$\begingroup\$

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:

  1. Grab the data from the database?
  2. Match the passwords using password_verify()?

<?php 
//will correspond with 'loginLogin.html page
require("opendboLogin.php"); 

$user = $_POST['username']; //these are grabbed from the login form (textbox)
$password = $_POST['password'];

$stmt2 = $conn->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
}
?>
\$\endgroup\$
3
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So you are suggesting to reverse all my if statements? Basically, check if something failed. And if it did fail, die()? Can you explain what die(); does? And can you please explain how my spacing is not internally consistent? Also, thank you very much for your thorough answer. \$\endgroup\$ – Nicholas Roberts Aug 20 '15 at 16:56
  • 1
    \$\begingroup\$ @NicholasRoberts Yes, I would reverse the statements. Die ends the script execution. It's generally not good practice to use die all over the place because it makes code hard to follow, so I used it as more of a placeholder, as I don't know the complete context of your code (I mean in the nesting stuff, for the redirect, die is correct to use). I would probably put the code in it's own function, and then return either true or false. Spacing: $stmt2 -> close(); vs $stmt2->fetch(); and if( vs if ( (I know it's nitpicking, but still^^). \$\endgroup\$ – tim Aug 20 '15 at 17:36
2
\$\begingroup\$

First question: Is this the most efficient way to: Grab the data from the database?

I think the way you are doing it is a bit verbose. $stmt2->execute(); should be used for updates and deletes because you aren't looking for a return data (execute returns a bool). Instead I suggest fetch or fetchAll.

It should look like this:

$stmt2 = $conn->stmt_init();
$stmt2 = $conn->prepare("SELECT apassword FROM login WHERE auser = ?"); 
$stmt2->bind_param("s", $user);
$result = $stmt2->fetch(PDO::FETCH_ASSOC);   // PDO::FETCH_ASSOC returns an associative array instead of numeric

// Then if check if the result is empty and go from there..:

if(!empty($result)){
   // ... 

Second Question: Looks fine to me.

Side notes:

session_start() needs to be used at the top of your file before any output. I see you are using a print statement first. A warning will be thrown in your circumstance about output headers.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ My server doesn't have PDO drivers installed, PDO::FETCH_ASSOC would not work for me :( \$\endgroup\$ – Nicholas Roberts Aug 20 '15 at 4:48
1
\$\begingroup\$

A few general comments:

Structure

What does your verify_password function do? I imagine it might be a better option to hash/salt the input-provided password and ask the database to tell you whether both the username and the stored hashed password match. Then you wouldn't have to have the database returning anything other than a row count of the results. You wouldn't retrieve the password ever from the database, so you'd never have to worry about accidentally doing the wrong thing with it.

Security

Generally you want to give users as little information as possible about why their login failed. It's better to give a generic "unsuccessful login attempt" rather than letting someone with bad intentions know that they've happened upon a valid username and just need to get the right password.

You save the input-provided username into a session variable once the login has been validated. However, you might run into complications saving this to text/putting it into html. Are you quite sure there's no need to remove tags or format special characters? Your use may be right, but make sure you've thought about it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hello sir. Thank you for your well thought out answer. We use password_verify to work in conjunction with PhP's password_hash(). It would not work to hash the password again on this script and see if they match because password_hash includes a new "salt" everytime. Password_verify avoids this issue and lets us check the password validity. \$\endgroup\$ – Nicholas Roberts Aug 20 '15 at 16:48
  • \$\begingroup\$ password_verify() is a built in PHP method! \$\endgroup\$ – Nicholas Roberts Aug 20 '15 at 16:55
  • \$\begingroup\$ @NicholasRoberts good to know. Thanks for telling me! \$\endgroup\$ – sunny Aug 20 '15 at 17:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.