Check my password protection code for holes.

valeryan

I am writing a script to secure my page. I know there have got to be hundreds of premade password protection scripts out there but I wanted to create my own for the challenge. I would like some on to look through it and see if there are any big security holes. The script runs fine and it all seems to work the way I expect but I feel that I don't know enough about security to just take it at face value that this is enough protection. here is my code any helpful suggestions will be appreciated. if any further information is need to analyze my code please let me know.

<?php
require_once('config.php'); 
require_once('session.php'); // move sessions to database
require_once ('functions.php'); // contains all functions used by the cms
if (!isset($_SESSION['user']))// if user is not logged in
{
	if(isset($_POST['u_name'])) // if username was filled in the form
	{
		$u_name = strtolower($_POST['u_name']);		
		if(isset($_POST['u_password'])) // if password was filled in the form
		{ 
			$u_password = $_POST['u_password'];
			$u_password = md5($u_password); // encrypts the password to compare to DB
		}
		if (!dbConnect()) // attempt to connect to database
		{
			echo 'Error connecting to database';
			exit;
		}		
		$query="SELECT * FROM cmsusers WHERE `user` = '$u_name' AND `pass` = '$u_password' AND `enabled` = '1' LIMIT 1";
		$result=mysql_query($query) or die; 
		$user_exist=mysql_numrows($result); 
		if($user_exist > 0) //if a user and the password match a user in the database 
		{				
			$login_user = strtolower(mysql_result($result,0,"user"));
			$login_level = mysql_result($result,0,"superadmin");
			setSessionVariable("user",$login_user);	
			setSessionVariable("superadmin",$login_level);								 
		}
		else // if a matching user was not found
		{				
			session_unregister("user"); // clean up session
			require_once('login_error_form.php'); //login form for errors
			exit;			
		}		
	}
	if (!isset($_SESSION['user'])) // if no user is set display a login form
	{
		require_once('login_form.php'); // standard login form
		exit;	
	}
}
?>

laserlight

The first big security hole is that you do not escape your input in your SQL. In this case, you should use [man]mysql_real_escape_string/man on $_POST['u_name'].

Also, what happens if $_POST['u_password'] is not set? You still use $u_password in the SQL, yet $u_password does not exist (and indeed running the query does not make sense).

Incidentally, hashing passwords is only useful as an additional layer of protection for your users in the event that your database is compromised. This means that an attack on the passwords is likely to be an offline attack. Consequently, a simple MD5 hash is not good enough. You need to add a salt, and possibly use repeated hashing. Switching to SHA-1 ([man]sha1/man) might also help a little, though one of the phpBB3 devs has told me otherwise, but without substantiation.

valeryan

I made the changes you suggested I think. I am not really worried about the password encryption the md5 is just really a token protection. I just didn't want to store passwords in clear text. I don't know why I just didn't want to. but I think the other changes will accomplish what you are telling me.

Thanks for your input.

<?php
require_once('config.php'); 
require_once('session.php'); // move sessions to database
require_once ('functions.php'); // contains all functions used by the cms
if (!isset($_SESSION['user']))// if user is not logged in
{
	if(isset($_POST['u_name'])) // if username was filled in the form
	{
		$u_name = strtolower($_POST['u_name']);		
		if(isset($_POST['u_password'])) // if password was filled in the form
		{ 
			$u_password = $_POST['u_password'];
			$u_password = md5($u_password); // encrypts the password to compare to DB
		}
		else
		{
			$u_password = '';
		}
		if (!dbConnect()) // attempt to connect to database
		{
			echo 'Error connecting to database';
			exit;
		}
		$u_name = mysql_real_escape_string($u_name);
    $u_password = mysql_real_escape_string($u_password);		
		$query="SELECT * FROM cmsusers WHERE `user` = '$u_name' AND `pass` = '$u_password' AND `enabled` = '1' LIMIT 1";
		$result=mysql_query($query) or die; 
		$user_exist=mysql_numrows($result); 
		if($user_exist > 0) //if a user and the password match a user in the database 
		{				
			$login_user = strtolower(mysql_result($result,0,"user"));
			$login_level = mysql_result($result,0,"superadmin");
			setSessionVariable("user",$login_user);	
			setSessionVariable("superadmin",$login_level); // will be 0 1 or 2 								 
		}
		else // if a matching user was not found
		{				
			session_unregister("user");
			require_once('login_error_form.php');
			exit;			
		}		
	}
	if (!isset($_SESSION['user'])) // if no user is set display a login form
	{
		require_once('login_form.php');		
		exit;	
	}
}
?>

Weedpacket

laserlight wrote:
though one of the phpBB3 devs has told me otherwise, but without substantiation

SHA1 is on its way out. See, e.g.: http://csrc.nist.gov/groups/ST/toolkit/documents/shs/NISTHashComments-final.pdf
Note that the type of attack is the same as MD5: being able to construct two distinct messages with the same hash (which isn't the same as being able to construct a message for an arbitrary given hash).

NIST recommends something from the SHA-2 family (SHA-224, -256, etc.) as being "good enough for government work"; at least until the winner of the current hash competition is determined.

laserlight

I made the changes you suggested I think.

There are still a few possible improvements. For example, there is still a flow of control where $u_password is used by not defined. mysql_numrows() is deprecated and should be mysql_num_rows(). I would suggest something along the lines of:

<?php
require_once 'config.php';
require_once 'session.php'; // move sessions to database
require_once 'functions.php'; // contains all functions used by the cms

if (!isset($_SESSION['user'])) // if user is not logged in
{
    // if username and password was filled in the form
    if (isset($_POST['u_name'], $_POST['u_password']))
    {
        $u_name = strtolower($_POST['u_name']);
        $u_password = md5($_POST['u_password']); // hash the password to compare to DB

    if (!dbConnect()) // attempt to connect to database
    {
        exit('Error connecting to database');
    }

    $login_user = $u_name;
    $u_name = mysql_real_escape_string($u_name);
    $query = "SELECT superadmin FROM cmsusers
              WHERE `user` = '$u_name' AND `pass` = '$u_password' AND `enabled` = '1' LIMIT 1";
    $result = mysql_query($query) or die;
    $user_exist = mysql_num_rows($result);
    if($user_exist > 0) //if a user and the password match a user in the database
    {
        $login_level = mysql_result($result, 0, "superadmin");
        setSessionVariable("user", $login_user);
        setSessionVariable("superadmin", $login_level); // will be 0 1 or 2
    }
    else // if a matching user was not found
    {
        session_unregister("user");
        require_once 'login_error_form.php';
        exit;
    }
}

if (!isset($_SESSION['user'])) // if no user is set display a login form
{
    require_once 'login_form.php';
    exit;
}
}
?>

SHA1 is on its way out. See, e.g.: http://csrc.nist.gov/groups/ST/toolk...ents-final.pdf
Note that the type of attack is the same as MD5: being able to construct two distinct messages with the same hash (which isn't the same as being able to construct a message for an arbitrary given hash).

That is what I mean by lack of substantiation: SHA-1 is broken for collision attacks, not preimage attacks, but preimage attacks are what this kind of password hashing is concerned with. So, why choose MD5 when you can choose SHA-1? The phpBB3 devs just insist that MD5 is in the same boat as SHA-1, thus it does not matter, but it seems that this boat that they are referring to is the "broken for collision attacks" boat.

NIST recommends something from the SHA-2 family (SHA-224, -256, etc.) as being "good enough for government work"; at least until the winner of the current hash competition is determined.

Unfortunately PHP4 is not yet obsolete, so some people (phpBB3, valeryan) might want to continue supporting a PHP4.4 installation out of the box, and consequently the hash extension might not be available, but sha1() surely would be available.

Weedpacket

laserlight wrote:
Unfortunately PHP4 is not yet obsolete

Though it will be in less than a couple of months.

valeryan

Thanks Laserlight, I tested the changes you made and it all works. I don't know why I didn't think to do my query that way. The only element from the query I am using is the superadmin so why select everything. Anyway I always appreciate your help.

vale

MD5 will rule the world!!! FOOOOREEEEVEEEER. It's my warm fuzzy and it reminds me of when I could run Linux freely and didn't need my adobe suite. It was a happier time with out Microsoft and adobe ruling my life.