Caught in a loop.... and can't walk out

[lauren_etherington]

I need my user to log in to use their "admin panel" functions.

 

I have this bit of html for them to enter their username/password:

	 <form id="form1" name="form1" method="post" action="functions/loginprocess.php">
			<table>
				<tr>
				</tr>
				<tr>
					<td>Username:</td>
					<td><input type="text" name="username" id="username" /></td>
				</tr>
				<tr>
					<td>Password</td>
					<td><input type="password" name="password" id="password" /></td>
				</tr>
				<tr>
					<td> </td>
					<td><input type="submit" name="button" id="button" value="Login" /></td>
				</tr>
			</table>
		</form>

This bit of code at the top of the html to start the session:

<?php
session_start();
$err = $_GET['e'];
?>

This bit of code at the top of the index page to redirect to the login screen

<?php
session_start();
$l=6;
$a="out";

if(isset($_SESSION['username']))
{
	$l=$_SESSION['username'];
	$a="in";
}
else
{
	header('Location: login.php?e=Login to use the administration functions');
}
$err = $_GET['e'];
?>

And this code to perform the log in functions:

<?php
	session_start();
$username = $_POST['username'];
$password = $_POST['password'];

$conn = mysqli_connect("Connection String");

$username = mysqli_real_escape_string($conn, $username);
$query="SELECT * FROM users WHERE username='$username' AND userpassword='$password'";

$result = mysqli_query($conn, $query);

if(mysqli_num_rows($result) == 0) // User not found. So, redirect to login_form again.
{
	$conn->close();
	header('Location: ../login.php?e=User not found');
	exit();
}

$userData = mysqli_fetch_array($result);
$hash = hash('sha256', $userData['cmspassword']."$password");

//echo "$hash";

if($hash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.
{
	$conn->close();
	header('Location: ../login.php?e=Incorrect login details');
	exit();
}
else{ // Redirect to home page after successful login.
	$_SESSION['username'] = $username;
	$_SESSION['user'] = $userData['userid'];
	$_SESSION['sp'] = $userData['cmspassword'];
	$conn->close();
	header('Location: ../index.php');
	exit();
}
?>

And the database table is called 'users' with the following columns:

userid

username

useremail

userpassword

cmspassword

 

However, as stated in the title of this post, I appear to be stuck in a loop where whenever I enter the user credentials it just keeps looping round the "user not found" message on line 16.

 

 

Can anyone help me, I am well and truly stuck on how to get out of this loop?

I know I've gone wrong somewhere, just can't see where

[ginerjm]

Do these bits of code have names?

[lauren_etherington]

They do have file names sorry, but I don't get that attached to my code to name it

There is login.php

    <?php
    session_start();
    $err = $_GET['e'];
    ?>

login.php body:

    	<form id="form1" name="form1" method="post" action="functions/loginprocess.php">
    <table>
    <tr>
    </tr>
    <tr>
    <td>Username:</td>
    <td><input type="text" name="username" id="username" /></td>
    </tr>
    <tr>
    <td>Password</td>
    <td><input type="password" name="password" id="password" /></td>
    </tr>
    <tr>
    <td> </td>
    <td><input type="submit" name="button" id="button" value="Login" /></td>
    </tr>
    </table>
    </form>

index.php

    <?php
    session_start();
    $l=6;
    $a="out";
     
    if(isset($_SESSION['username']))
    {
    $l=$_SESSION['username'];
    $a="in";
    }
    else
    {
    header('Location: login.php?e=Login to use the administration functions');
    }
    $err = $_GET['e'];
    ?>

And finally loginprocess.php:

    <?php
    session_start();
    $username = $_POST['username'];
    $password = $_POST['password'];
     
    $conn = mysqli_connect("Connection String");
     
    $username = mysqli_real_escape_string($conn, $username);
    $query="SELECT * FROM users WHERE username='$username' AND userpassword='$password'";
     
    $result = mysqli_query($conn, $query);
     
    if(mysqli_num_rows($result) == 0) // User not found. So, redirect to login_form again.
    {
    $conn->close();
    header('Location: ../login.php?e=User not found');
    exit();
    }
     
    $userData = mysqli_fetch_array($result);
    $hash = hash('sha256', $userData['cmspassword']."$password");
     
    //echo "$hash";
     
    if($hash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.
    {
    $conn->close();
    header('Location: ../login.php?e=Incorrect login details');
    exit();
    }
    else{ // Redirect to home page after successful login.
    $_SESSION['username'] = $username;
    $_SESSION['user'] = $userData['userid'];
    $_SESSION['sp'] = $userData['cmspassword'];
    $conn->close();
    header('Location: ../index.php');
    exit();
    }
    ?>

[ginerjm]

you are hashing your password to compare to the query results, no?  But your query was looking for a match to an unhashed password entry.  How is that ever going to work?

 

You really should be attached to your code.  Plus - why did you make this simple little login process into 4 separate modules?  A bit complicated, no?

[lauren_etherington]

Probably, It's in 4 separate modules because they are the bits at the top of the pages to start the sessions and the form snippet was there just in case I'd cocked up a variable name.

 

So when the password is entered how do I get it to look for the hashed version in the database?

They aren't going to enter the encrypted password obviously so how would I match them up as it where?

 

I have found and modified a tutorial on this forum not long after I posted this question actually and it is doing the same thing.

 

I know it's a lot of questions but I've not actually wrote a log in script before - it seems to be another large gap in my university education!

[ginerjm]

You hash it and supply THAT value to the query.

[Psycho]

I see no problem in separating the logic as you have. To help debug this issue. comment out the header() that perform the redirect and put an echo in. That way you can see exactly where the problem is. You can also add additional debugging information to verify the contents of any relevant variables. Once you verify one redirect, uncomment it to see where the process goes next.

 

I don't see an obvious reason why there would be an infinite loop, so debugging is in order.

[lauren_etherington]

Well is this bit of the code not checking the password? :S

$userData = mysqli_fetch_array($result);
$hash = hash('sha256', $userData['userpassword']."$password");
     
     
if($hash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.

:S?

[ginerjm]

Yes it is but you have no matching record that I can see since you did a query looking for the unhashed password value which doesn't exist, no?

 

If you query looking for the true typed-in username (which you should be sanitizing before putting into a query btw) and the true typed-in password (which needs to be sanitized and hashed) you won't find it.  So it makes no sense to be doing a compare after that since there is nothing to compare.

[Psycho]

Well is this bit of the code not checking the password? :S

$userData = mysqli_fetch_array($result);
$hash = hash('sha256', $userData['userpassword']."$password");
     
     
if($hash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.

:S?

 

Yes, but your query is looking for a match based on the user name and the password (before you hash the password).

 

You should NOT write the hashing process multiple times (e.g. one where it is stored and another where it is compared). Write ONE hashing process and call it from wherever you need it. That way you are guaranteed to be 100% consistent.

 

But, even if that is failing, I don't see that it explains your original issue.

 

But, since this has been brought up. I see that you have a couple of different error conditions. One if the user was not found and another if the password does not match. That is considered a security flaw. A malicious user can use that to determine a valid user ID. Then they can use that ID to then try to find a valid password. You should just hash the password first, then look for a record that matches BOTH the user ID and the hashed password. If none is found then simply state "Unable to validate credentials" or something similar. 

Edited April 7, 2014 by Psycho

[lauren_etherington]

Process where the password is originally stored at your request

$un = $_POST['uname'];
$em = $_POST['mail'];
$ap = $_POST['apass'];
$cp = $_POST['cpass'];

$pw = hash('sha256', "$ap$cp");


//connect to Database
$conn = mysqli_connect("connection string");

if(!$conn || $conn->connect_errno){
	echo "Connection error " . $conn->connect_errno . " " . $conn->connect_error;
}

//define Query
$avquery = "INSERT INTO users (username,useremail,userpassword,cmspassword) VALUES ('$un','$em','$pw','$cp')";

//run query
$result = mysqli_query($conn, $avquery);

if($result){

	echo "Entered data successfully";
	echo "<BR />";
	echo "click <a href='http:///admin/users.php'>here</a> to return to Users";

}

else {
	echo "Oh No! Something has gone wrong and the data could not be uploaded";
	echo "<BR />";
	echo "click <a href='http:///admin/users.php'>here</a> to return to Users";
}

$conn-> close();

[ginerjm]

This just proves my supposition.  You are storing the pw in a hashed state.  Good.  But when you try to do a login, you go looking in the db for an un-hashed password.  That fails (0 rows) and you send back the not found message.  You enter the credentials again and the whole query and error msg process repeats, just like you said in your OP.

[lauren_etherington]

Like I said earlier, I have never wrote a log in script with hashed passwords ect before so rather than comparing the two would it look something more like this?

So if the user is found it would do the compare? :S

Maybe?

    session_start();
$username = $_POST['username'];
$password = $_POST['password'];
     
$conn = mysqli_connect("Connection String");
     
$username = mysqli_real_escape_string($conn, $username);
$query="SELECT * FROM users WHERE username='$username' AND userpassword='$password'";
$result = mysqli_query($conn, $query);
     
if(mysqli_num_rows($result) == 1) {
     
$userData = mysqli_fetch_array($result);
$hash = hash('sha256', $userData['userpassword']."$password");
     
//echo "$hash";
     
if($hash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.
{
	$conn->close();
	header('Location: ../login.php?e=Incorrect login details');
	exit();
}

}

else{ // Redirect to home page after successful login.
	$_SESSION['username'] = $username;
	$_SESSION['user'] = $userData['userid'];
	$_SESSION['sp'] = $userData['cmspassword'];
	$conn->close();
	header('Location: index.php');
	exit();
}

EDIT --

 

Sorry, I posted this just as you posted that comment....

So I need to sumbit the password and hash it in order it compare it to the stored password??

and if it matches then redirect to the index page?

Edited April 7, 2014 by lauren_etherington

[Psycho]

 

 

Process where the password is originally stored at your request 

 

 

I didn't ask for that - I only stated we couldn't see it. As I stated, you should have ONE process to do hashing and reuse it wherever you need it.

 

But as ginerjm has pointed out, this query will never return a result

 

	$username = $_POST['username'];
	$password = $_POST['password'];
	 
	$conn = mysqli_connect("Connection String");
	 
	$username = mysqli_real_escape_string($conn, $username);
	$query="SELECT * FROM users WHERE username='$username' AND userpassword='$password'";

 

You are looking for a match on the username and the UNHASHED password sent by the user. Hash the password before you run this query. Then only provide a single failure scenario.

[ginerjm]

Am I not writing English?  I can't make this any clearer.

 

You need to move that hash line above your query so that you will locate the record.  Period.

[lauren_etherington]

I am sorry, But like I said, This isn't something I have come accross before.

So I want to 100% fully understand what do to with it.

 

Which hash line?

Would it be this one before the query?

$hash = hash('sha256', $userData['userpassword']."$password");

So the password will be hashed on submit?

 

And then this hashed password will be compared to the one in the database?

[ginerjm]

NO

 

You are currently grabbing the input from the POST array, no?

 

You are establishing two vars - username and password, no?

 

You are then using them in a query to locate the correct login record, no?

 

But you aren't finding it because you used a password value in the query that will never match the stored one since it isn't hashed.  Right?

 

Erroneously - you continue on and try and check the query results (which don't exist) against a just-hashed password value.  Instead you need to take that hashing statement and DO IT before you try and do the query and THEN you will find a query result.  There is no need to check the password after this since you found a match in the db and you can confirm the user's login.  All you need to is ensure that the query runs and that the query results == 1 row. 

 

Program much?

[mac_gyver]

this might be a good time to slow down and first define what it is you are trying to accomplish, then what inputs you have available, what preprocessing you need to do on those inputs, and what result or output you are trying to produce, both for when the inputs are what you expect, and when they are not.

 

starting with your registration code. you have two password fields being submitted. why? and what processing did you define for those two password fields during registration? the reason i ask, is you have concatenated the values from those two password fields, hashed the concatenated value, and stored one of the password fields and the hash in your database table. this makes no sense.

[Psycho]

I see another problem (not related to the issue at hand). You are redefining $username using this:

 

$username = mysqli_real_escape_string($conn, $username);

 

So, if authentication passes you are storing THAT into the session data.

 

$_SESSION['username'] = $username;

 

You should only escape a value at the time that you need it.

 

Anyway, try the following code that has been rewritten to resolve some minor issues and will provide debugging details to help identify the issue you are facing.

 

<?php
session_start();
 
$conn = mysqli_connect("Connection String");
 
$username = trim($_POST['username']);
$usernameSQL = mysqli_real_escape_string($conn, $username);
$password = $_POST['password'];
 
$query = "SELECT userpassword, cmspassword, userid
          FROM users
          WHERE username='$usernameSQL'";
 $result = mysqli_query($conn, $query);
 
if(!$result)
{
    //Query failed
    $debug = "Debug: Query failed.<br>Query: <br>Error" . mysqli_error();
    $location = 'Location: ../login.php?e=User not found';
}
elseif(!mysqli_num_rows($result)) // User not found. So, redirect to login_form again.
{
    //User not found
    $debug = "Debug: User '{$username}' not found. Use this condition to implement a lock feature for multiple invalid login attempts";
    $location = 'Location: ../login.php?e=Unable to verify credentials';
}
else
{
    //User was found
    $user = mysqli_fetch_array($result);
    $passwordHash = hash('sha256', $user['cmspassword']."$password");
 
    if($passwordHash != $userData['userpassword']) // Incorrect password. So, redirect to login_form again.
    {
        $debug = "Debug: Password does not match. Submitted PW: {$password}, PW Hash: {$passwordHash}, DB Hash: {$userData['userpassword']}";
        $location = 'Location: ../login.php?e=Unable to verify credentials';
    }
    else
    {
        //Authentication successful. Redirect to home page.
        $debug = "Debug: Authentication passed"; 
        $_SESSION['username'] = $username;
        $_SESSION['user'] = $user['userid'];
        $_SESSION['sp'] = $user['cmspassword'];
        $location = 'Location: ../index.php';
    }
}
 
//Perform redirect
$conn->close();
//Comment out the debug and uncomment the header once fixed
echo $debug;
//header($location);
exit();
 
    ?>

[ginerjm]

I do believe this last post adds to the confusion.  The OP has already shown us that the stored password is encrypted and that the attempt to validate a user's credentials is trying to do so using an unencrypted search argument.  This code replicates that same error.

 

Simply hashing the user-supplied password (or two passwords?) and THEN doing the query will solve the OP's original question.  From that point on there are many, many things that need to be addressed in order for this to be a safe and sound php script.  Let's get thru the first problem before inundating her with more code that does not solve her question.  Yes - it won't be safe but at least she can have something that works, even if it is not the right way, that she can then improve upon.

[Psycho]

I do believe this last post adds to the confusion.  The OP has already shown us that the stored password is encrypted and that the attempt to validate a user's credentials is trying to do so using an unencrypted search argument.  This code replicates that same error.

No, it does not. Did you even read my previous posts or look at the debug statements? It queries using ONLY the username and retrieves the hashed password from the database. Then it hashes the user input password from the POST data and compares to the value retrieved from the database. This is the proper way to perform this type of operation as it allows you to determine if the username is correct, but not the password so you can lock the user after X number of failed login attempts for security purposes

 

 

Simply hashing the user-supplied password (or two passwords?) and THEN doing the query will solve the OP's original question. . . .  Let's get thru the first problem before inundating her with more code that does not solve her question.

I only modified the previous code to function in a more logical manner and added debugging code to help identify where the problem may be. Hashing the user supplied password and using that as part of the query does not let you know if the username would match an existing record or not when there are no results returned because of the password condition. That would require another query to find that out. being able to lock a record for multiple invalid login attempts is an industry standard practice that prevents a malicious user from brute-forcing a password. So, you should query for a record matching JUST the username first. If there is no match just state authentication failed and move on. If a record is returned THEN hash the user supplied password and compare to the result. If it matches, authenticate the user. If not, increment an invalid login attempts value. If more than n attempts then lock the user.

[lauren_etherington]

Hi Mr Psycho!

Thank you for that!!! I've tried that code you wrote and unfortunaltely it is not hashing the password,

I just get this error message:

 

Debug: Password does not match. Submitted PW: "unhashed version of password", PW Hash: "hashed version of password", DB Hash:

 

So it is still not hashing the password before comparing it.... I have moved this

$passwordHash = hash('sha256', $user['userpassword']."$password");

so it is now above the query but hasn't made a difference (taking into account Ginerjms advice... I think(hope)?)

 

It's authenticating the username fine though

So at least I know one bits working haha!

Edited April 8, 2014 by lauren_etherington

[lauren_etherington]

I've been browsing the internets, would I need Java/Jquery to get this to behave and hash the password on submit??

[mac_gyver]

what would make you think javasciprt/jquery would have anything to do with a comparison not matching values in the server-side code?

 

 

if you read my reply above in this thread, you will see that the hash value you are storing during registration isn't what you think.

[Psycho]

 

Debug: Password does not match. Submitted PW: "unhashed version of password", PW Hash: "hashed version of password", DB Hash:

 

 

This is why we are not getting anywhere. You need to provide EXACTLY what you are seeing. But, I will *assume* from the above (since I have no other choice based on what you provided) that the problem is as mac_gyver says. You are not storing what you think you are for the password hash in your database. From the debug statement that I was gracious enough to provide it should be obvious. There is nothing after "DB Hash". So, you must be storing an empty value.