Best way to check if user is logged in

[chrisrulez001]

Hi there,

 

I've been searching the internet for the best way to check if the user has been logged in. Some codes have security breaches. So I'm not sure where to start.

 

Here's what I've come up with:

 

The user logs in and is checked whether he/she is a valid user, if not return false and if true carry on and create session, I read the post that Jacques1 made about session feedback and implemented what he said. After that the session variables are assigned and then the user id, session_id and a unique identifier to check against on each page load are inserted into a database and then the user is logged in.

 

Here's my code: (please note this is in a class and only shows the login function)

function Login($username, $password)
	{
		try
		{
			$db = new PDO("mysql:host=".DB_HOST.";dbname=".DB_NAME.";charset=utf8", DB_USERNAME, DB_PASSWORD);
			$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
			$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
		}
		catch(PDOException $ex)
		{
			echo "Unable to connect to DB";
			error_log($ex->getMessage());
		}
		
		try
		{
			$User_Info = $db->prepare("SELECT * FROM users WHERE username=:username");
			$User_Info->bindValue(":username", $username, PDO::PARAM_STR);
			$User_Info->execute();
			$Info = $User_Info->fetchAll(PDO::FETCH_ASSOC);
			
			$salt = $Info['salt'];
			
			$password = $salt . $password;
			$password = $this->CreateHash($password);
			
			$unique_key = $this->GenerateRandom();
			$unique_key = $this->CreateHash($unique_key);		
			
			$Check_User = $db->prepare("SELECT * FROM users WHERE username=:username AND password=:password");
			$Check_User->bindValue(":username", $username, PDO::PARAM_STR);
			$Check_User->bindValue(":password", $password, PDO::PARAM_STR);
			$Check_User->execute();	
			
			if($Check_User->rowCount() > 0)
			{
				while($row = $Check_User->fetchAll(PDO::FETCH_ASSOC))
				{
					session_destroy();
					session_start();
					
					$_SESSION = array();
					
					session_regenerate_id(true);
					
					$_SESSION['username'] = $row['username'];
					$session_id = session_id();
					$user_id = $row['id'];
					
					$Check_Logged_In = $db->prepare("DELETE FROM logged_in_users WHERE user_id=:userid");
					$Check_Logged_In->bindValue(":user_id", $user_id, PDO::PARAM_STR);
					$Check_Logged_In->execute();
					$has_changed = $Check_Logged_In->rowCount();
					
					if($has_changed > 0)
					{
						$Logged_In = $db->prepare("INSERT INTO logged_in_users (id, user_id, session_id, unique_key) VALUES (NULL, :user_id, :session_id, :unique_key)");
						$Logged_In->bindValue(":user_id", $user_id, PDO::PARAM_STR);
						$Logged_In->bindValue(":session_id", $session_id, PDO::PARAM_STR);
						$Logged_In->bindValue(":unique_key", $unique_key, PDO::PARAM_STR);
						$Logged_In->execute();
						$affected_rows = $Logged_In->rowCount();
						
						if($affected_rows > 0)
						{
							return true;
						}
					}
					return false;
				}
			}

			return false;
		}
		catch(PDOException $ex)
		{
			echo "Unable to complete query";
			error_log($ex->getMessage());
		}
	}

Thanks

[chrisrulez001]

Update to code: (Fixed some bugs)

 

Reason for edit is that I retested the code and found some bugs, was unable to edit first post so replied in second post with updated code.

function Login($username, $password)
	{
		try
		{
			$db = new PDO("mysql:host=".DB_HOST.";dbname=".DB_NAME.";charset=utf8", DB_USERNAME, DB_PASSWORD);
			$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
			$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
		}
		catch(PDOException $ex)
		{
			echo "Unable to connect to DB";
			error_log($ex->getMessage());
		}
		
		try
		{
			$User_Info = $db->prepare("SELECT * FROM users WHERE username=:username");
			$User_Info->bindValue(":username", $username, PDO::PARAM_STR);
			$User_Info->execute();
			$Info = $User_Info->fetch();
			
			$salt = $Info['salt'];
			
			$password = $salt . $password;
			$password = $this->CreateHash($password);
			
			$unique_key = $this->GenerateRandom();
			$unique_key = $this->CreateHash($unique_key);		
			
			$Check_User = $db->prepare("SELECT * FROM users WHERE username=:username AND password=:password");
			$Check_User->bindValue(":username", $username, PDO::PARAM_STR);
			$Check_User->bindValue(":password", $password, PDO::PARAM_STR);
			$Check_User->execute();	
			
			if($Check_User->rowCount() > 0)
			{
				while($row = $Check_User->fetch())
				{
					session_start();
					
					$_SESSION = array();
					
					session_regenerate_id(true);
					
					$_SESSION['username'] = $row['username'];
					$session_id = session_id();
					$_SESSION['unique_key'] = $unique_key;
					$user_id = $row['id'];
					$_SESSION['user_id'] = $user_id;
					
					$Check_Logged_In = $db->prepare("DELETE FROM logged_in_users WHERE user_id=:user_id");
					$Check_Logged_In->bindValue(":user_id", $user_id, PDO::PARAM_STR);
					$Check_Logged_In->execute();
					$has_changed = $Check_Logged_In->rowCount();
					
					$Logged_In = $db->prepare("INSERT INTO logged_in_users (id, user_id, session_id, unique_key) VALUES (NULL, :user_id, :session_id, :unique_key)");
					$Logged_In->bindValue(":user_id", $user_id, PDO::PARAM_STR);
					$Logged_In->bindValue(":session_id", $session_id, PDO::PARAM_STR);
					$Logged_In->bindValue(":unique_key", $unique_key, PDO::PARAM_STR);
					$Logged_In->execute();
					$affected_rows = $Logged_In->rowCount();
					
					if($affected_rows > 0)
					{
						return true;
					}
					return false;
				}
			}

			return false;
		}
		catch(PDOException $ex)
		{
			echo "Unable to complete query";
			error_log($ex->getMessage());
		}
	}

Edited July 25, 2014 by chrisrulez001

[Rifts]

What exactly is your question? It seems like you have an idea of what to do and how to do it.

 

To add to what you have; you may want a script to run every few minutes checking the last time a user was 'online' (loaded a new page), and if it's over 5 or 10 minutes you could log them out.

[Jacques1]

What is the unique key supposed to do? What's the point of the logged_in_users table?

 

The standard way of authenticating users is very simple: The user submits their credentials, you check them, and if they're correct, you create a fresh session and store the user ID. That's it. On each protected page, you simply check the presence of a user ID in the session and possibly look up the user for authorization checks.

 

As long as the session ID is sufficiently random (which is determined by session.entropy_file and session.entropy_length), this scheme is fairly secure and sufficient for average websites. If you want to limit the number of sessions to only one, you simply store the active session IDs in the users table and add a check for this. And of course you can have additional security features like a time limit.

 

Besides that, there are several issues in your code:

• Don't establish a new database connection for every single function call. This is extremely inefficient and bloats the code. Use a single shared database connection for the entire application. For example, make a script which creates a global $db variable holding the database connection, include this script when needed and access the connection through $db.
•  Your error handling makes no sense. What is the point of telling the user that the database connection or a query has failed? What are they supposed to do with this information? Just let the exception terminate the script and create a nice generic error page for your users.
• You're using some home-made password hash algorithm where you prepend a salt and hash the result probably with something like MD5 or SHA. This is a very bad idea. Do not invent your own algorithms, because you never know if they're sound. If you indeed use MD5(salt + password) or SHA(salt + password), this is not secure at all. Both MD5 and SHA are extremely vulnerable to brute-force attacks. Salting doesn't help either, because if the attacker can search the entire space of possible passwords in a few seconds, they don't care about the salt. So always use an established algorithm like bcrypt.
• What's the point of making a second query to see if the password is correct? You already know the hash from the previous query. Just check it with PHP (using bcrypt).

[chrisrulez001]

Thanks to you both for your replies.

 

In response to your question, I was just wondering if this was a secure way of doing things. But it probably will be after I implement he suggestions made by Jacques. But thanks for your suggestion about run it every so often. Would 10minutes be a reasonable time?

 

I will certainly implement all that everyone has said.

 

The purpose of the logged_in_users table is basically a log of which users are logged on. The purpose of the unique key is to check that it is the user using that session id and I read what you said in the other thread about session hijacking and thought it might add another layer of security.

 

Thanks for the suggestion about the global $db. I don't know what I was thinking when I added the cannot connect to the db error.

 

The password hash is running through hash_hmac and is sha512 and the salt is a random string about 50 characters in length. But I will look into crypt definatly. It does make life easier. Just a quick question about bcrypt, it mentions cost, does this mean how long it takes to generate a hash?

 

Thanks again to you both and hope this helps elaborate on a few items.

Edited July 25, 2014 by chrisrulez001

[Jacques1]

But thanks for your suggestion about run it every so often. Would 10minutes be a reasonable time?

 

It makes no sense to literally delete the sessions after a certain time. Just store the timestamp when the session is started and delete expired sessions when you encounter them. This has the exact same effect. And unused sessions are automatically removed by the garbage collector.

 

 

 

The purpose of the logged_in_users table is basically a log of which users are logged on.

 

Store this information in the users table. It makes no sense to put so much effort into maintaining the one-to-one relationship between the two tables when you might as well store the data directly in the user records.

 

 

 

The purpose of the unique key is to check that it is the user using that session id and I read what you said in the other thread about session hijacking and thought it might add another layer of security.

 

The session ID already is a secret unique identifier, so it's not very useful to store another identifier next to it. To protect the session against hijacking, use HTTPS and set the Secure and HTTPOnly flag for the session cookie.

 

 

 

The password hash is running through hash_hmac and is sha512 and the salt is a random string about 50 characters in length.

 

SHA-512 is absolutely unsuitable for password hashing. A stock PC can calculate hundreds of millions of SHA-512 hashes per second, so an attacker is able to find passwords simply by trying out many different combinations.

 

Don't let the fancy name fool you: With regard to password hashing, there's no fundamental difference between SHA-512 and, say, MD5. They're just as bad. Sure, SHA-512 is a bit more expensive, but an attacker can easily make up for it by adding some more computing power or waiting a bit longer.

 

The HMAC also makes no sense. Now your entire password security depends on the secret key, which is exactly what we do not want. The reason why we hash passwords instead of encrypting them is because we want them to be secure even if the attacker has compromised the entire system. There shouldn't be any shortcuts to finding out the passwords.

 

Last but not least, 50 characters are excessive. Random strings in a security context are typically 16 bytes long. If you generate them properly using a device like /dev/urandom, there's absolutely no risk of anybody guessing them. It's physically infeasible.

 

 

 

Just a quick question about bcrypt, it mentions cost, does this mean how long it takes to generate a hash?

 

The cost argument specifies how much computing power it takes to calculate a hash. Of course you want this to be as much as possible to slow down attackers. However, if the value is too high, you slow down your own site as well and make it susceptible to denial-of-service attacks.

 

A common recommendation is to decide how much time you're willing to spend on the hashing on your current hardware. Let's say one second. Then you try different values until the hash calculation indeed takes one second. You'll probably end up with something around 15.

 

It's also important to increase the value over time as hardware becomes better. That's actually the whole point of the parameter and a very important property of modern password hash algorithms. Simple algorithms like SHA-512 require only a fixed amount of computing power, which means the situation gets worse for us with each new generation of hardware. In the 90s, MD5 or SHA may have been acceptable for hashing passwords, because people only had slow CPUs. Nowadays, we're dealing with extremely powerful GPUs or even specialized hardware like ASICs and FPGAs. The only way to deal with that is to use an adaptable hash algorithm with a variable cost factor.

 

bcrypt is actually rather primitive in that regard. There are much more sophisticated schemes like scrypt which let you fine-tune all kinds of different parameters. However, scrypt is too young and not well supported, so it's best to stick with bcrypt for now.

[chrisrulez001]

Thank you very much for all the excellent information. I'll get straight onto fixing the issues in the code.

 

Just one quick question, how do you check that it takes one second to complete the hash? I had a look on the link you sent about bcrypt and it doesn't mention about checking how long it takes.

[Jacques1]

<?php


require_once __DIR__ . '/lib/password.php';



$cost = 14;


$start = microtime(true);

$hash = password_hash('foobar', PASSWORD_BCRYPT, array('cost' => $cost));

echo microtime(true) - $start;

[chrisrulez001]

<?php

require_once __DIR__ . '/lib/password.php';


$cost = 14;

$start = microtime(true);
$hash = password_hash('foobar', PASSWORD_BCRYPT, array('cost' => $cost));
echo microtime(true) - $start;

 

Thanks very much for your help.

 

Everything you suggested is in place and fixed