Anyone able to help me with this bug in a login form?

Question

So, I have a login form which is having a bit of trouble. It keepsechoing Incorrect password, please try again. whenever I try and access the restricted page. I have had a fiddle on myself, but I have not been able to find out what is wrong. The code is as follows:

 <?php

//MySQL Database connect;

include "databaselogin.php";

//Checks if there is a login cookie

if(isset($_COOKIE["ID_my_site"]))

//If there is a cookie, the user is directed to a restricted page

{

$emailaddress = $_COOKIE["ID_my_site"];

$pass = $_COOKIE["Key_my_site"];

$check = mysql_query("SELECT * FROM Users WHERE EmailAddress='$emailaddress'") or die(mysql_error());

while($info = mysql_fetch_array( $check )) {

    if ($pass != $info["password1"]) {

    }

    else {

        header("location: restricted.php");

    }

}

}

if (isset($_POST["submit"])) { //If the form has been submitted

//Make sure they filled it all in

if(!$_POST["emailaddress"] | !$_POST["password1"]) {

echo("You did not fill in all the required fields.");

}

//Checks it against the database

if (!get_magic_quotes_gpc()) {

$_POST["emailaddress"] = addslashes($_POST["emailaddress"]);

}

$check = mysql_query("SELECT * FROM Users WHERE EmailAddress = '".$_POST["emailaddress"]."'") or die(mysql_error());

//Gives a message if the user doesn't exist

$check2 = mysql_num_rows($check);

if ($check2 == 0) {

echo ("The Email Address that you have entered is not in use, <a href='register.php'>click here</a> to register");

}

while($info = mysql_fetch_array( $check )) {

$_POST["password1"] = stripslashes($_POST["password1"]);

$info["Password"] = stripslashes($info["Password"]);

$_POST["password1"] = sha1($_POST["password1"]);

//Gives an error is the password is wrong

if ($_POST["password1"] != $info["Password"]) {

    echo("Incorrect password, please try again.");

}

else {

    //If the login is ok, a cookie is added

    $_POST["EmailAddress"] = stripslashes($_POST["EmailAddress"]);

    $hour = time() + 3600;

    setcookie(ID_my_site, $_POST["emailaddress"], $hour);

    setcookie(Key_my_site, $_POST["password1"], $hour);

    //Then they are redirected to a restricted area

    header("location: restricted.php");

}

}

}

else {

//If they are not logged in

?>

<form action="<?php echo $_SERVER['PHP_SELF']?>" method="post"> 

<table border="0"> 

<tr><td colspan=2><h1>Login</h1></td></tr> 

<tr><td>Email Address:</td><td> 

<input type="text" name="emailaddress" maxlength="40" placeholder="Email Address"> 

</td></tr> 

<tr><td>Password:</td><td> 

<input type="password" name="password1" maxlength="12" Placeholder="Password"> 

</td></tr> 

<tr><td colspan="2" align="right"> 

<input type="submit" name="submit" value="Login"> 

</td></tr> 

</table> 

</form> 

<?php 

}

?>

All help will be massively appreciated.

Answer 1

There are a couple of issues. First, mysql_query is a deprecated PHP function and should be replaced with mysqli_query. All functions in your code should use the mysqli prefix instead of mysql (so mysql_fetch_assoc should be changed to mysqli_fetch_assoc). This function also takes a parameter providing a connection to the database, which is done with mysqli_connect. So your code should have something like this:

$con = mysqli_connect($username, $password, $host, $db); // Fill in the variables with correct values
$check = mysqli_query($con, "SELECT * FROM Users WHERE EmailAddress='$emailaddress'");

$con only needs to be set once and can be used in other query calls in your code.

Answer 2

First of all the way you store your credentials in cookies is very dangerous. Anyone who has access to your computer or to your network if you're not using ssl can steal your cookies and log in to your account.

Secondly your problem lies in

while($info = mysql_fetch_array( $check )) {

this is an infinite loop. you should only call this once.

Your overall code could use some improvements such as:

• update mysql to mysqli or PDO
• use prepared statements
• optimize code for speed (use || instead of |)
• use a stronger hashing algorithm

Leave a comment if you want a more in depth instruction to improve your code

Hope this helped

Improvements

this is a great article about PDO. But PDO is object based and since you're new to PHP and i don't know your skill level you can better use mysqli for now. There're plenty of articles available on how you can do this.

PDO

---

in your code you use

if(!$_POST["emailaddress"] | !$_POST["password1"]) {

but if you use || instead of | the if condition skips the second argument if the first already failed.

---

You use sha1 for hashing your passwords. But this algorithm is a bad practice. You should use Bcrypt or at least use an individual salt for each password you encrypt with sha1 and store that next to the password in the database

SHA1 not safe anymore

---

You never store the user info in a session to retain the login on next requests, the way you're implementing it is called a remember me function and is considered hard to implement safely. it is easier to work with sessions first and if you really need it cookies later.

If you're using sessions you should also check if the session_id hasn't been set by an attacker in the clients browser. You can do this by setting a random cookie such as init and when this is not set you call

session_regenerate_id();

---

You store both the email and the hashed password in a cookie. this can be very dangerous. You shouldn't store the password even if it is hashed in an cookie. The best practice is to hash a randomly created string of characters with a high entropy and store only that in the cookie and in the database. When the user logged in once with that cookie you should refresh the cookie with a new hash.

---

To fix your error you should remove the while loop around the mysql_fetch_array($check)

Tips for in the future

Your code looks a lot more organized if you start to learn to work with PHP objects. This can also make your project a lot easier to work with.

I don't know if you're going to use this code in a production website because I highly recommend against that. You can better use a safe and sound solution that somebody with more experience has created and when you have more experience you can start building your own.