security question!

[chrisuk]

Hi all, I posted this in the beta testing forum but got no response so thought I would try my luck in here.

 

I've been working on beefing up the security on my login script and I think I have most things covered

 

What I would like is for someone to try and break it...

 

the link is here: http://www.yump.co.uk/stuff/login.php

 

if you successfully manage to login you will simply get the message "login success!"

 

Otherwise you will get an error....but the error messages arn't supposed to be helpful

 

there is also an index.php page protected by sessions

 

Feedback etc appreciated.

 

Also if you do find a bug please do let me know and also a pointer as to how to fix it!

 

Thanks!

[per1os]

As far as it seems, sql injection does not seem like an issue.

 

If you want a real critique I suggest posting the login code, but yea. Other than that it seems fine.

[chrisuk]

oops, forgot

 

login processing script:

 

<?php

session_start();

include("config.php");

//ensure there are no problems with ' or other special characters in someones name and avoid SQL injection

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

$password = md5($_POST['password']);
$password = mysql_real_escape_string($password);

$sql = "SELECT * FROM users WHERE username = '$username'";

$result = mysql_query($sql) or die(mysql_error());
$num = mysql_num_rows($result);

if ($num == 1) //record found

//'COLLATE latin1_bin' allows case sensitivity
$sql2 = "SELECT * FROM users WHERE username = '$username' AND password = '$password' COLLATE latin1_bin";
$result2 = mysql_query($sql2) or die(mysql_error());
$num2 = mysql_num_rows($result2);


if ($num2 > 0) //password correct, set session variables and proceed to user home
{
	$_SESSION['loggedin'] = "yes";
	$_SESSION['id'] = $username;
	session_write_close();
	//setcookie('Admin', md5($_POST['password'].$random));

	header ("Location: index.php");
}
else //incorrect password
{
	header ("Location: login.php?error_id=2");
}

}
else //record not found	
{
header ("Location: login.php?error_id=3");
}


//exit;

?>

 

[per1os]

A few suggestions:

 

<?php
// check and make sure we do have post data and not throw a silly error giving information away.
$username = isset($_POST['username'])?trim($_POST['username']):'';
$password = isset($_POST['password'])?trim($_POST['password']):''; 

// add a check here to make sure that they did enter data into the form.
if (empty($username) || empty($password)) {
    header ("Location: login.php?error_id=4"); // either username or password was left blank
}

$username = mysql_real_escape_string($username); // escape username
$password = md5($password); // no escape needed here as it is hashed into md5.

$sql = "SELECT * FROM users WHERE username = '$username' COLLATE latin1_bin"; // do this on 1 shot no need to make 2 trips to the db.

$result = mysql_query($sql) or die(mysql_error());
$num = mysql_num_rows($result);

if ($num == 1) //record found
        // verify the password vs md5. Since md5 is a hash it is case SenSitIve. username here will also be case SeNSiTiVe (to fix strtolower both)
if ($result['password'] == $password && $result['username'] == $username) //password correct, set session variables and proceed to user home
{
	$_SESSION['loggedin'] = "yes";
	$_SESSION['id'] = $username; // I would suggest doing the userid, to avoid giving out more information than is needed.
	session_write_close();
	//setcookie('Admin', md5($_POST['password'].$random));

	header ("Location: index.php");
}
else //incorrect password
{
	header ("Location: login.php?error_id=2");
}

}
else //record not found	
{
header ("Location: login.php?error_id=3");
}


//exit;

?>

[chrisuk]

thanks frost, most appreciated! will certainly do that!

[chrisuk]

I have made the suggested changes, but it is giving me a password incorrect error!

 

any ideas? The details I am entering are correct

[mmarif4u]

Try out this:

<?php
// check and make sure we do have post data and not throw a silly error giving information away.
$username = isset($_POST['username'])?trim($_POST['username']):'';
$password1 = isset($_POST['password'])?trim($_POST['password']):''; 

// add a check here to make sure that they did enter data into the form.
if (empty($username) || empty($password)) {
    header ("Location: login.php?error_id=4"); // either username or password was left blank
}

$username = mysql_real_escape_string($username); // escape username
$password = md5($password1); // no escape needed here as it is hashed into md5.

$sql = "SELECT * FROM users WHERE username = '$username' COLLATE latin1_bin"; // do this on 1 shot no need to make 2 trips to the db.

$result = mysql_query($sql) or die(mysql_error());
$num = mysql_num_rows($result);

if ($num == 1) //record found
        // verify the password vs md5. Since md5 is a hash it is case SenSitIve. username here will also be case SeNSiTiVe (to fix strtolower both)
if ($result['password'] == $password && $result['username'] == $username) //password correct, set session variables and proceed to user home
{
	$_SESSION['loggedin'] = "yes";
	$_SESSION['id'] = $username; // I would suggest doing the userid, to avoid giving out more information than is needed.
	session_write_close();
	//setcookie('Admin', md5($_POST['password'].$random));

	header ("Location: index.php");
}
else //incorrect password
{
	header ("Location: login.php?error_id=2");
}

}
else //record not found	
{
header ("Location: login.php?error_id=3");
}


//exit;

?>