issue with login

[cloudll]

Hey guys.

 

I am having an issue with my login script. lets say the password is 'freak'. if i input the password 'freakghsdgsd' it will still accept it as okay and log in. It will still fail if the password doesn't start with 'freak'.

 

here is my code, can anyone see what I have done wrong?

    $password = '';
    if(isset($_POST['password']))
    $password = htmlentities($_POST['password']);

    $salt = "Snake";
    $cryptPass = crypt($password,md5($salt));        
    $sql = "SELECT * FROM login WHERE id='1'";
    foreach ($connect->query($sql) as $row)
    {
    $db_username = $row['username'];
    $db_password = $row['password'];
    }
    if (($username === $db_username) && ($cryptPass === $db_password)) {
    $_SESSION['username'] = $_POST['username'];

[Ofarchades]

Are you sure? I can't see any obvious reason why it would do that. Would you be able to post all of the code for that file, in case there's a problem elsewhere?

 

Other than that, I have a couple of advisory notes regarding how you've approached password hashing.

 

1) Your salt isn't very strong. Check the PHP documentation on how to create a bcrypt salt, for example.

2) "$cryptPass === $db_password" is vulnerable to a timing attack.

 

If you're able, it may be better to check out the password_hash and password_verify functions added in PHP 5.5. Otherwise, strengthen the salt and look for a time-constant method of comparing the hashes (e.g. hash_equals).

[fastsol]

This part makes no sense.

 foreach ($connect->query($sql) as $row)
    {
    $db_username = $row['username'];
    $db_password = $row['password'];
    }

Why would you use a foreach on this?  You should be only getting a single row from the db, so absolutely zero reason to use a loop of any kind. Also it's rather pointless, but not necessarily wrong, to use a === to compare the values.  The POST will only hold string type values and so will the returned varchar columns of a db query.  So they are obviously going to match type anyway.  I only mention it cause when you read the code it makes it seem like you could be expecting something else besides a string value to compare against, when it's not really possible.

[Ofarchades]

This part makes no sense.

 foreach ($connect->query($sql) as $row)
    {
    $db_username = $row['username'];
    $db_password = $row['password'];
    }

Why would you use a foreach on this?  You should be only getting a single row from the db, so absolutely zero reason to use a loop of any kind. Also it's rather pointless, but not necessarily wrong, to use a === to compare the values.  The POST will only hold string type values and so will the returned varchar columns of a db query.  So they are obviously going to match type anyway.  I only mention it cause when you read the code it makes it seem like you could be expecting something else besides a string value to compare against, when it's not really possible.

 

You're right about the foreach - and as I mentioned in my reply, he shouldn't be doing any sort of string comparison between two hashes.

 

However, you can never trust PHP's == string comparison to work as you'd expect; there are plenty of documented cases where it will return crazy results (such as the famous "md5('240610708') == md5('QNKCDZO')" scenario).

[fastsol]

You're right about the foreach - and as I mentioned in my reply, he shouldn't be doing any sort of string comparison between two hashes.

 

However, you can never trust PHP's == string comparison to work as you'd expect; there are plenty of documented cases where it will return crazy results (such as the famous "md5('240610708') == md5('QNKCDZO')" scenario).

I am confused as to how that comparison would be weird.  If the md5 hash is the same result for both strings, then yeah they would equal each other.  MD5 is highly know to produce same hashes for different strings.  I guess I have never ran into a scenario that comparing string values together would not work as expected if they are both strings.  By all means there are plenty of times I use === on INT or Bool type values, but never found a reason to on strings.  Can you provide us a better example of bad scenario, I am very interested.  

[Ofarchades]

The comparison is weird because it evaluates as true despite the hashes being different. This actually works for any string beginning with "0e" due to the fact that PHP will convert them to 0 internally.

 

There's also the fact that the integer 0 will return true when compared to most* strings (e.g. 'test' == 0) because PHP turns this into an integer comparison and typecasts the string to an integer (which results in 0).

 

* I say "most" strings because some strings will be typecast to different ints e.g. '2test' would become 2 and therefore '2test' == 2.

 

I'll use == myself in cases where security isn't a concern or I can be certain of what the values can and can't be, but otherwise it's probably better to just not gamble with PHP's bizarre internal magic.

[fastsol]

Very interesting, I just tried your examples to see for myself.  I will say that it's pretty stupid of php to typecast  when you didn't tell it too.  For me it's never an issue cause I always appropriately typecast or know it's type beforehand to know what comparison to do with it.  But very good to know none the less.  Thanks!

[cloudll]

Thanks for the replies. I will look into a new verify functions.

 

I was using that method to query my database from http://wiki.hashphp.org. I have been trying a few different ways to remove the loop while keeping the $rows but cannot seem to get it working. Would anyone be able to show me the correct way to query the database and fetch the results into variables please.

[fastsol]

What connection library are you using? mysqli or PDO?

[cloudll]

PDO :-)

[fastsol]

$sql = "SELECT * FROM login WHERE id='1'";

$get = $connect->query($sql);

$row = $get->fetch(PDO::FETCH_ASSOC);


echo $row['username'];

echo $row['password'];

[cloudll]

Thanks

[cloudll]

So I looked into using the new hashing function. To test it i created a password with the following code and inserted it into my database

$password = "password";
$hash = password_hash($password, PASSWORD_DEFAULT);

I then check if the login password matches that password in the database using the following

if (($username == $db_username) && (password_verify($db_password, $hash))) {
$_SESSION['username'] = $_POST['username'];

However my login fails.

If i echo the database password directly and print the password entered in my login form. The hashes do not match. Infact they change on every refresh so how would they ever match the password in the database?

[Ofarchades]

I could be wrong, but I think you need to change

if (($username == $db_username) && (password_verify($db_password, $hash))) {

to

if (($username == $db_username) && (password_verify($password, $db_password))) {

Where $password is the plain text password as posted from the login form - and $db_password is the hash stored in the database.

[cloudll]

okay, i think i have it working okay now. It also fixed the original comparison error

 

does this look okay?

$username = '';
    if(isset($_POST['username']))
    $username = htmlentities($_POST['username']);

    $password = '';
    if(isset($_POST['password']))
    $password = htmlentities($_POST['password']);    

    $sql = "SELECT * FROM login WHERE username='$username'";
    $get = $connect->query($sql);
    $row = $get->fetch(PDO::FETCH_ASSOC);

    $db_username = $row['username'];
    $db_password = $row['password'];    
       
    if (password_verify($password, $db_password)) {     
    $_SESSION['username'] = $_POST['username'];
      
    $id = 1;
    $sql = "UPDATE login SET last_login=?, ip=? WHERE id=?";
    $statement = $connect->prepare($sql);
    $statement->execute(array($dt,$ip,$id));
    //reloadPage();

    }
    if ((isset($_POST['username'])) && ($username !== $db_username)) {                       

    $loginmsg ="Username/password is incorrect";

    } 
    elseif ((isset($_POST['password'])) && ($password !== $db_password)) {     

    $loginmsg ="Username/password is incorrect";
    }

    function loginstats() {
    global $last_login,$dbip;
    echo "Welcome back: ";
    echo $_SESSION['username'];
    echo "<br />Last Login:";
    echo $last_login;                                 
    }

Edited May 14, 2015 by cloudll

[fastsol]

There was still a lot to be desired with that script.  Here is a reworked version.  I didn't run the script to check to syntax errors, but the logic should be pretty close.  I added lots of comments too.

if(isset($_POST['submit'], $_POST['username'], $_POST['password']))
{
	$username = htmlentities($_POST['username']);
	$password = $_POST['password']; // Don't use the htmlentities on this unless you did so on the register side also, or they won't match.  It's being hashed anyway, so not really needing to use htmlentities on it.  
	
	$sql = "SELECT * FROM login WHERE username=?"; // ? Placeholder, you did it below, but not here for some reason.
	$get = $connect->prepare($sql); // Use prepare to prevent SQL injection
	$get->execute(array($username)); // Execute the query
	
	if($get->rowCount() === 1)
	{
		$row = $get->fetch(PDO::FETCH_ASSOC); // Fetch the result
		
		$db_username = $row['username'];
		$db_password = $row['password'];    
		   
		if(password_verify($password, $db_password)) 
		{     
			$_SESSION['username'] = $db_username; // Don't use the POST version just to be safe, use the DB record version.
			  
			$sql = "UPDATE login SET last_login=?, ip=? WHERE id=?";
			$statement = $connect->prepare($sql);
			$statement->execute(array($dt,$ip,$row['id'])); // easier to simply put $row['id'] in here rather than assign a var to $id for a single use of the var $id.
			//reloadPage();
			
			// This function either needs to NOT be a function and simply echo out the info, or put in a separate file with other functions. Plus you didn't actually call the function, you only created it, so the info wouldn't have shown anyway.
			function loginstats()
			{
				global $last_login,$dbip;
				echo "Welcome back: ";
				echo $_SESSION['username'];
				echo "<br />Last Login:";
				echo $last_login;                                 
			}
			
			loginstats();
		}
		else
		{
			echo 'Wrong Username / Password';	
		}		
	}
	else
	{
		echo 'Wrong Username / Password';
	}
}
else
{
	echo 'Not all credentials were provided. Please try again.';
}

[cloudll]

Wow thats amazing. Thank you for the comments as well. very helpful.