PHP login problem

[arbitter]

Hello there,

 

The problem is that my login system isn't consistent. There's an option of staying logged in, and if you check that a cookie gets created with a hash. When you visit the site again, you should be logged in automatically. But the thing is, that when you load the page, you aren't! But after a refresh you are logged in. So I'm guessing a problem in the order of things that get done... Here's the script:

 

<?php
session_start();
if(isset($_COOKIE['LoginCookie']))
{
	$hash = $_COOKIE['LoginCookie'];
	mysql_select_db('database');
	$sql = "SELECT all the info of the people FROM people WHERE cookie_hash = '".$hash."'";
	if($result = mysql_query($sql))
	{
		$row = mysql_fetch_array($result);
		if(empty($row))
		{
			setcookie('LoginCookie','',time()-3600);
		}
		if(mysql_num_rows($result) == 1)
		{
			$_SESSION['loggedin'] = true;
			$_SESSION['loggedinnick'] = $row['nick'];
			$_SESSION['loggedinvoornaam'] = $row['voornaam'];
			$_SESSION['loggedinachternaam'] =  $row['achternaam'];
			$_SESSION['loggedinid'] = $row['id'];
			$_SESSION['loggedintype'] = $row['type'];
		}
	}
}


if(isset($_POST['login']))
{
	if(empty($_POST['username']) || empty($_POST['wachtwoord']))
	{
		$_SESSION['melding'] = 'ERROR';
		header('Location: index.php');
		exit();
	}
	$username = CleanMyDirtyData($_POST['username']);
	$wachtwoord = sha1(CleanMyDirtyData($_POST['wachtwoord']));
	mysql_select_db('database');
	$sqlmail = mysql_query("SELECT * FROM people WHERE email='$username' AND wachtwoord = '$wachtwoord'");
	$sqlnaam = mysql_query("SELECT * FROM people WHERE nick='$username' AND wachtwoord = '$wachtwoord'");
	if(mysql_num_rows($sqlmail) == 1 || mysql_num_rows($sqlnaam) == 1)
	{
		if(mysql_num_rows($sqlmail) == 1)
		{
			$row = mysql_fetch_array($sqlmail);
		}
		else
		{
			$row = mysql_fetch_array($sqlnaam);
		}
		if(isset($_POST['remember']))
		{
			$hash = sha1($username . 'Some secret thingys for your safety');
			setcookie('LoginCookie',$hash,time()+30000000);
			mysql_query("UPDATE leden SET cookie_hash='" . $hash . "' WHERE id='" . $row['id'] . "'")or die(mysql_error());
		}
		$_SESSION['loggedin'] = true;
		$_SESSION['loggedinnick'] = $row['nick'];
		$_SESSION['loggedinvoornaam'] = $row['voornaam'];
		$_SESSION['loggedinachternaam'] = $row['achternaam'];
		$_SESSION['loggedinid'] = $row['id'];
		$_SESSION['loggedintype'] = $row['type'];
		$_SESSION['melding'] = 'You are logged in now.';
	}
	else
	{
		$_SESSION['melding'] = 'ERROR';
		header('Location: index.php');
		exit();
	}
}

 

So if the checkbox that you want to stay logged in, there is no cookie made but the $_SESSION['loggedin'] is set to true. Throughout the rest of the page, this parameter is used to show either content for guests of private content for people that are logged in.

 

BUT the problem is that if it is checked on the first page load the $_SESSION['loggedin'] doesn't seem to get set even though the cookie is set.

 

 

Another slight problem with my script is that when a user logs in on another pc, the hash get's changed in the database and the user gets logged out of the site on the previous page. What is the easiest way to do that?

And do you guys think this script is safe? Or do you see some holes?

 

Thanks in advance!

arbitter

[gizmola]

-write a function and put the code that sets your session variables upon login.  You should call this function after login or after the LoginCookie matches, so that you are sure you're doing the exact same thing in both cases. 

-I don't see you redirecting to where the user should be after a successful login.

-why would you want to allow people to login multiple time from multiple different pc's?  You could accomodate that, but at the cost of a substantial increase in complexity.  Is that a use case you really want/need to support?

 

 

[arbitter]

-write a function and put the code that sets your session variables upon login.  You should call this function after login or after the LoginCookie matches, so that you are sure you're doing the exact same thing in both cases. 

-I don't see you redirecting to where the user should be after a successful login.

-why would you want to allow people to login multiple time from multiple different pc's?  You could accomodate that, but at the cost of a substantial increase in complexity.  Is that a use case you really want/need to support?

- I'll give that a try

 

- Since I never had any scripting lessons I don't quite know the right structure of these things, but either way the user gets the eg index.php, and for some parts of the site (for example where there is a possibility to login or to see your current username), I use:

<?php
   if(isset($_SESSION['loggedin']) && $_SESSION['loggedin'] == true){
      //code when user is logged in, eg to show the username and stuff
   }else{
      //code that shows eg the login part of the page
   }
?>

I'm guessing this isn't the best method, but it's what I'm currently using.

 

- I guess this point isn't neccesary, but perhaps for future projects it might be more handy.

[micmania1]

You may also want to consider validating the hash before using it in your SQL query.

 

When your cookie is set type this into your url bar:

 

Javascript: void(document.cookie="LoginCookie='or user_id=1--");

 

The above changes the value of your hash to ['or user_id=1--] check if your hash is equal to '' or user_id=1 which is usually admin. Then ignores the rest of the query using --

 

People could also delete your table:

'; DROP TABLE People --

 

See mysql_real_escape_string().

 

EDIT:

The above javascript isn't a solution; its an example of the vulnerability. mysql_real_escape_string() is the solution.

[arbitter]

You may also want to consider validating the hash before using it in your SQL query.

 

When your cookie is set type this into your url bar:

 

Javascript: void(document.cookie="LoginCookie='or user_id=1--");

 

The above changes the value of your hash to ['or user_id=1--] check if your hash is equal to '' or user_id=1 which is usually admin. Then ignores the rest of the query using --

 

People could also delete your table:

'; DROP TABLE People --

 

See mysql_real_escape_string().

 

EDIT:

The above javascript isn't a solution; its an example of the vulnerability. mysql_real_escape_string() is the solution.

Oh delightful that you saw that!

The thing I use to clean my variables is this:

<?php
function CleanMyDirtyData($dirtydata)
{
	return mysql_real_escape_string(htmlentities($dirtydata, ENT_QUOTES,'UTF-8'));
}
?>

Would this change the hash in any way? I mean, does the regular hash and the cleaned hash be different and as a result un-matching, or not?

I'm not that familiar with the real changing of the value due to these functions.

[gizmola]

I don't think you understand what I was saying. 

 

Looking at your code, there's a point where you process the login due to having data from a post, you do a database lookup, you set set a bunch of session variables and the remember me cookie.  Great -- these are things you need to do.  Then the script ends.  If you don't redirect to the "logged in page" you're sitting looking at a blank page.  This explains why when you refresh suddenly you are "logged in".  You need to redirect once you complete that work.

[arbitter]

I don't think you understand what I was saying. 

 

Looking at your code, there's a point where you process the login due to having data from a post, you do a database lookup, you set set a bunch of session variables and the remember me cookie.  Great -- these are things you need to do.  Then the script ends.  If you don't redirect to the "logged in page" you're sitting looking at a blank page.  This explains why when you refresh suddenly you are "logged in".  You need to redirect once you complete that work.

Well, this script is in the beginning of my index page, when there is no output yet. The session gets set as logged in, and after that the content of the site gets loaded. This shouldn't be the problem should it?

 

When a user isn't logged in and logs in, he is logged in right away, even though the page doesn't reload and the script

<?php
$_SESSION['loggedin'] = true;
$_SESSION['loggedinnick'] = $row['nick'];
$_SESSION['loggedinvoornaam'] = $row['voornaam'];
$_SESSION['loggedinachternaam'] = $row['achternaam'];
$_SESSION['loggedinid'] = $row['id'];
$_SESSION['loggedintype'] = $row['type'];
?>

 

So I don't see why this would be the problem...