Security Check!

[Clinton]

I was wondering if the experts here could look at the below code real quick and see if it's safe and efficient for logging in and calling pages. There are no errors I'm just looking for a security check to see if there is anything that I need to improve upon. Thanks.

 

Login Page:

<?php
session_start();
session_unset();
include 'connect/project.htm';
if (isset($_POST['login']))
{
	if($_POST['username']!='' && $_POST['password']!='')
	{$username = $_POST['username']; $ndate = date ("Y-m-d H:m:s");
		$query = mysql_query('SELECT id, username, active, accesstype FROM users WHERE username = "'.mysql_real_escape_string($_POST['username']).'" AND password = "'.mysql_real_escape_string(md5($_POST['password'])).'"');

		if(mysql_num_rows($query) == 1)
		{
    			$row = mysql_fetch_assoc($query);
			if($row['active'] == NULL)
			{
				$_SESSION['user_id'] = $row['username'];
				$_SESSION['access_type'] = $row['accesstype'];
				$at = $row['accesstype'];
				$_SESSION['logged_in'] = TRUE;
				mysql_query("UPDATE employers SET lastlogin = '$ndate' WHERE username = '$username'");
    				header("Location: ".$at."member.php");


			}else {
			$error = $_SESSION['activate'] = TRUE; header("Location: index.php");
				}
		}
		else {
			$error = 'Login failed !';
			$error = $_SESSION['failed'] = TRUE; header("Location: index.php");
		}
	}
	else 	{
		$error = $_SESSION['single'] = TRUE; header("Location: index.php");
			}
}
?>

 

 

To verify if one is logged in the following attributes are pretty much asked for on each page:

<?php
session_start();
include 'connect/project.htm';

if(($_SESSION['logged_in'] == 1) && ($_SESSION['access_type'] == d))
{

 

 

 

[GingerRobot]

You don't escape $username in this query:

 

mysql_query("UPDATE employers SET lastlogin = '$ndate' WHERE username = '$username'");

 

That could potentially be a threat depending on your register script (basically, it would have to have allowed a 'bad' query to have been used as the username).

[RussellReal]

lol ginger beat me tro the punch.. $username is coming in from a $_POST so it could have some code that would mess with your query if the user is malicious

 

and since username would be a string you could use mysql_real_escape_string

[Clinton]

Ok, so change my:

 

$username = $_POST['username'];

 

to

 

$username = .mysql_real_escape_string($_POST['username'])

 

[RussellReal]

$username = mysql_real_escape_string($_POST['username']);

 

 

EDIT!

 

mysql_real_escape_string(md5($_POST['password'])).

 

if you use md5() on $_POST['password'] then there is no need to use mysql_real_escape_string as md5() returns a hash which is alphanumeric and will never contain a ' or . or % or ` or any other mysql specific characters

[Psycho]

For this line:

header("Location: ".$at."member.php");

 

I will assume you have a check on that page to ensure the user is already logged in AND that you are checking the value for $_SESSION['access_type']. Otherwise, users can simply change the URL to see pages they are not supposed to see. Just a suggestion, but instead of having specific pages for each accesstype you could create a single initial page which checks the value of $_SESSION['access_type'] and then includes the correct content page.

 

Also, another suggestion, I typically like to trim() the input values for text fields so spaces are not considered. In this case the username field. But, that would also require that the values are trimmed during the creation process. Otherwise you could end up with a username created with leading or trailing spaces, but the user would never be able to log in! I never trim() passwords.

[Clinton]

 

I will assume you have a check on that page to ensure the user is already logged in AND that you are checking the value for $_SESSION['access_type'].

 

Correct, I recall it on each page. That was just an example of how I'm verifying users page to page.

 

Thanks for the notes and information, folks. I'm making the changes while learning and researching at the same time.