PHP MySQL validation problem

[HuuD]

Hi all,

I have the below code and the issue is I'm trying to validate the login/password of user from mysql and there seems to be this issue where ONLY if the email match was found in the db and the password was WRONG the error message shows up, if the email was not found in the db, even though I have set an error message it does not show.

 

<?php
session_start();

$server = "localhost";
$user = "root";
$pwd = "";
$sql_db = "cabcustomers";
$lname = isset($_POST['lemail']);
$lpass = isset($_POST['lpass']);
$dbname;

$conn = @mysqli_connect($server,$user,$pwd,$sql_db);

if (!$conn)
{
die("Connection to Database has failed: " . mysqli_connect_error());
}

if (isset ($_POST["lemail"]) && isset ($_POST["lpass"]))
{
$lemail = $_POST["lemail"];
$lpass = $_POST["lpass"];

$query = "select email, password from customer where email='$lemail' and password = '$lpass'";
$result = mysqli_query($conn, $query);

if ($row = mysqli_fetch_assoc($result) > 0)
{
$dbname = $row["email"];
$dbpass = $row["password"];
$passchk = password_verify($lpass, $dbpass);

if ($lemail == $dbname && $passchk == $dbpass)
{
$_SESSION['sesName'] = $dbname;
header('location:booking.php');
}
else
{ /*THIS PART FAILS AS THIS ERROR ONLY SHOWS IF THE EMAIL EXISTS AND THE PASSWORD AND DOES NOT SHOW IF NO MATCH WAS FOUND
echo '<div class="row">
<div class="col-xs-10 col-xs-offset-1 col-sm-8 col-sm-offset-2 col-md-4 col-md-offset-4">
<div class="panel-body">
<form role="form">
<fieldset>
<div class="form-group">';
echo '<div class = "alert alert-danger alert-dismissable fade in"><button type = "button" class = "close" data-dismiss = "alert" aria-hidden = "true">×</button>This email is not registered or the password is incorrect, please try again or consider registering</div>
</div>
</fieldset>
</form>
</div>
</div>
</div>';
}
}
elseif (empty($lemail) || empty($lpass))
{
echo '<div class="row">
<div class="col-xs-10 col-xs-offset-1 col-sm-8 col-sm-offset-2 col-md-4 col-md-offset-4">
<div class="panel-body">
<form role="form">
<fieldset>
<div class="form-group">';
echo '<div class = "alert alert-info alert-dismissable fade in"><button type = "button" class = "close" data-dismiss = "alert" aria-hidden = "true">×</button>No email or password entered</div>';
'</div>
</fieldset>
</form>
</div>
</div>
</div>';
}
}
mysqli_close($conn);
?>

Thank You All for your help.

[Sepodati]

 

if ($row = mysqli_fetch_assoc($result) > 0)

 

Take off the > 0 part.

 

Stop storing plain text passwords and use prepared statements. This is very unsecure the way you have it.

[HuuD]

if ($row = mysqli_fetch_assoc($result) > 0)

 

Take off the > 0 part.

 

Stop storing plain text passwords and use prepared statements. This is very unsecure the way you have it.

 

I believe you missed the password_verify() which hashes and compares hashed passwords..

[Barand]

Well done for using password_verify(). Now all you need to do is read the manual to find out how to use it correctly.

[mac_gyver]

your logic doesn't make sense and won't work as written. if your passwords are hashed, then the sql query will never match a row (because the password column value will never be equal to the submitted $lpass value in the sql query). the use of the > 0 that Sepodati pointed out is causing $row to be a Boolean value and none of the $row elements will exist, so all the code using them won't work.

 

i recommend that you start over and write just the code you need (you have several statements that aren't being used and even more that are not needed) to accomplish this task - Keep It Simple (KISS.)

 

for this task, your form processing code needs to -

 

1) check if the current visitor is already logged in. if they are, there's no good reason to run the login form processing code. either display a message or redirect them elsewhere on your site.

 

2) make the database connection, without outputting any connection errors to the visitors on your site. the way to handle connection and other types of database statement errors is to use exceptions, let php catch the exception, and let php use it's error_reporting, display_errors, and log_errors settings to control what happens with the actual error information. when learning, developing, and debugging code, you should display the error information. when on a public/live site, you should log the error information.

 

3) detect that a post method form was submitted using simple generic code. just test if $_SERVER['REQUEST_METHOD'] == 'POST'. there's no good reason to write out code testing if all the form fields are set, that you have to change to match each different form.

 

4) trim and validate the input data. if the trimmed data is empty, there's no point in using it. if you store the validation errors in an array, you can validate all the inputs at once and use the array as an error 'flag'. if the array is empty, there are no errors. if the array is not empty, there are errors. only store the text of the error message in the array.

 

5) if there are no validation errors, use the submitted data.

 

5.1) produce and execute a prepared query to find if the email matches a row in your database table, fetch the row if found. if the row wasn't found, the email didn't match. setup an 'invalid login' error message.

 

5.2) check if the hashed password stored in the database table matches the submitted password. if the password didn't match, setup an 'invalid login' error message.

 

5.3) if both the email and password values match, set a session variable with the corresponding user's id (an auto-increment column in the database table.) the reason for NOT storing the email as the user's identifier in the session variable is because the value cannot be edited (directly by a moderator/administrator or by the user via a proper emailed link verification) and take effect without requiring the user to log out and back in again.

 

---

 

to detect if the current visitor is logged in or not, test the session variable holding the user's id. if the session variable isset(), there's a logged in user and their id is the value in the session variable. if it is not set, the visitor is not logged in. if you want to get any related user information, such as their username, email, or permissions, use the user's id from the session variable and run a select query against the appropriate table(s).

 

unfortunately, the php mysqli extension you are using is not the best choice, especially if using prepared queries. if you can, switch to use the php PDO extension. it is much simpler and more constant to use over the php mysqli extension.

 

also, Don't Repeat Yourself (DRY). you have repeated the section of html markup for each different error message. what happens if you want to change the html markup for the error display? you will have to change it every place you have used it. if you follow the recommendation of using an array to hold the validation error messages (only the text of the message), you would loop over that array when displaying the errors and output the text of the message in the html markup you have defined. the html markup will only exist once and any changes will only need to be made in one place.

[HuuD]

 

your logic doesn't make sense and won't work as written. if your passwords are hashed, then the sql query will never match a row (because the password column value will never be equal to the submitted $lpass value in the sql query). the use of the > 0 that Sepodati pointed out is causing $row to be a Boolean value and none of the $row elements will exist, so all the code using them won't work.

 

i recommend that you start over and write just the code you need (you have several statements that aren't being used and even more that are not needed) to accomplish this task - Keep It Simple (KISS.)

 

for this task, your form processing code needs to -

 

1) check if the current visitor is already logged in. if they are, there's no good reason to run the login form processing code. either display a message or redirect them elsewhere on your site.

 

2) make the database connection, without outputting any connection errors to the visitors on your site. the way to handle connection and other types of database statement errors is to use exceptions, let php catch the exception, and let php use it's error_reporting, display_errors, and log_errors settings to control what happens with the actual error information. when learning, developing, and debugging code, you should display the error information. when on a public/live site, you should log the error information.

 

3) detect that a post method form was submitted using simple generic code. just test if $_SERVER['REQUEST_METHOD'] == 'POST'. there's no good reason to write out code testing if all the form fields are set, that you have to change to match each different form.

 

4) trim and validate the input data. if the trimmed data is empty, there's no point in using it. if you store the validation errors in an array, you can validate all the inputs at once and use the array as an error 'flag'. if the array is empty, there are no errors. if the array is not empty, there are errors. only store the text of the error message in the array.

 

5) if there are no validation errors, use the submitted data.

 

5.1) produce and execute a prepared query to find if the email matches a row in your database table, fetch the row if found. if the row wasn't found, the email didn't match. setup an 'invalid login' error message.

 

5.2) check if the hashed password stored in the database table matches the submitted password. if the password didn't match, setup an 'invalid login' error message.

 

5.3) if both the email and password values match, set a session variable with the corresponding user's id (an auto-increment column in the database table.) the reason for NOT storing the email as the user's identifier in the session variable is because the value cannot be edited (directly by a moderator/administrator or by the user via a proper emailed link verification) and take effect without requiring the user to log out and back in again.

 

---

 

to detect if the current visitor is logged in or not, test the session variable holding the user's id. if the session variable isset(), there's a logged in user and their id is the value in the session variable. if it is not set, the visitor is not logged in. if you want to get any related user information, such as their username, email, or permissions, use the user's id from the session variable and run a select query against the appropriate table(s).

 

unfortunately, the php mysqli extension you are using is not the best choice, especially if using prepared queries. if you can, switch to use the php PDO extension. it is much simpler and more constant to use over the php mysqli extension.

 

also, Don't Repeat Yourself (DRY). you have repeated the section of html markup for each different error message. what happens if you want to change the html markup for the error display? you will have to change it every place you have used it. if you follow the recommendation of using an array to hold the validation error messages (only the text of the message), you would loop over that array when displaying the errors and output the text of the message in the html markup you have defined. the html markup will only exist once and any changes will only need to be made in one place.

 

 

Thanks for taking the time out, will follow as instructed..appreciate it..

[Psycho]

I will add the following:

 

Regarding mac_gyver's responses 5.1 and 5.2. Note that he is proposing runnign a query to find a matching record based on the email address, then doing a separate check to validate the email if a record is returned. But, notice that he states " setup an 'invalid login' error message.". This is important. Create a single process/function to tell the user that you were unable to authenticate them. The message should always be the same whether their email doesn't exist or if the password did not match. Otherwise, you will "leak" information (e.g. a malicious user could determine valid account emails).

 

Also, when writing your code, *think* about the variables and how you use them. Think about what the value is supposed to hold and why you need to use it. For example, in your original code you had this

 

$query = "select email, password from customer where email='$lemail' and password = '$lpass'";
$result = mysqli_query($conn, $query);

if ($row = mysqli_fetch_assoc($result) > 0)
{
    $dbname = $row["email"];
    $dbpass = $row["password"];
    $passchk = password_verify($lpass, $dbpass);

    if ($lemail == $dbname && $passchk == $dbpass)
    {

 

Look at the query you ran and then look at the last line. Why did you have a comparison to check if the user supplied email matched the email of the record returned in the query? You KNOW that the email would necessarily match since it was a condition in the query. The duplication check is unnecessary. While it would do no harm (assuming all the code was working properly), it adds complexity where it does not need to exist that will later come back to haunt you. For example, if you were to change the variable name of $lemail you could miss updating the condition. It just creates another point of failure.

[HuuD]

I will add the following:

 

Regarding mac_gyver's responses 5.1 and 5.2. Note that he is proposing runnign a query to find a matching record based on the email address, then doing a separate check to validate the email if a record is returned. But, notice that he states " setup an 'invalid login' error message.". This is important. Create a single process/function to tell the user that you were unable to authenticate them. The message should always be the same whether their email doesn't exist or if the password did not match. Otherwise, you will "leak" information (e.g. a malicious user could determine valid account emails).

 

Also, when writing your code, *think* about the variables and how you use them. Think about what the value is supposed to hold and why you need to use it. For example, in your original code you had this

$query = "select email, password from customer where email='$lemail' and password = '$lpass'";
$result = mysqli_query($conn, $query);

if ($row = mysqli_fetch_assoc($result) > 0)
{
    $dbname = $row["email"];
    $dbpass = $row["password"];
    $passchk = password_verify($lpass, $dbpass);

    if ($lemail == $dbname && $passchk == $dbpass)
    {

Look at the query you ran and then look at the last line. Why did you have a comparison to check if the user supplied email matched the email of the record returned in the query? You KNOW that the email would necessarily match since it was a condition in the query. The duplication check is unnecessary. While it would do no harm (assuming all the code was working properly), it adds complexity where it does not need to exist that will later come back to haunt you. For example, if you were to change the variable name of $lemail you could miss updating the condition. It just creates another point of failure.

 

Thanks for pointing out, I had a single point of data retrieval from db and validation, but it was not working as it should have so I took this from another place to test it..

[HuuD]

I will add the following:

 

Regarding mac_gyver's responses 5.1 and 5.2. Note that he is proposing runnign a query to find a matching record based on the email address, then doing a separate check to validate the email if a record is returned. But, notice that he states " setup an 'invalid login' error message.". This is important. Create a single process/function to tell the user that you were unable to authenticate them. The message should always be the same whether their email doesn't exist or if the password did not match. Otherwise, you will "leak" information (e.g. a malicious user could determine valid account emails).

 

Also, when writing your code, *think* about the variables and how you use them. Think about what the value is supposed to hold and why you need to use it. For example, in your original code you had this

$query = "select email, password from customer where email='$lemail' and password = '$lpass'";
$result = mysqli_query($conn, $query);

if ($row = mysqli_fetch_assoc($result) > 0)
{
    $dbname = $row["email"];
    $dbpass = $row["password"];
    $passchk = password_verify($lpass, $dbpass);

    if ($lemail == $dbname && $passchk == $dbpass)
    {

Look at the query you ran and then look at the last line. Why did you have a comparison to check if the user supplied email matched the email of the record returned in the query? You KNOW that the email would necessarily match since it was a condition in the query. The duplication check is unnecessary. While it would do no harm (assuming all the code was working properly), it adds complexity where it does not need to exist that will later come back to haunt you. For example, if you were to change the variable name of $lemail you could miss updating the condition. It just creates another point of failure.

 

Thanks for pointing out, I had a single point of data retrieval from db and validation, but it was not working as it should have so I took this from another place..

 

Anyway I found the issue was my implementation and NOT related to password encryption as pointed out above...

 

I was using a while loop which was causing the variables to change, so I changed the while loop to an if condition and removed $row and added a variables to receive the values, working fine and authenticating...

 

Thanks..

[Jacques1]

The message should always be the same whether their email doesn't exist or if the password did not match. Otherwise, you will "leak" information (e.g. a malicious user could determine valid account emails).

 

The ugly truth is that's it's practically impossible not to leak this information. Even if you show the same message, anybody can easily tell the difference between the two execution paths by comparing the response times. Since modern password hash algorithms are expensive by design, there will be noticable difference between an invalid e-mail address (which is instantly recognized) and an invalid password (which requires the server to compute the hash).

 

You can make such attacks harder by designing the system appropriately, but this requires a lot of work, experience and complexity.

 

A much better solution is to not use the e-mail address as a user identifier at all. Make the user choose a public name which doesn't have to be protected. This may be slightly less convenient, but the e-mail address will at least be somewhat protected.