first attempt at admin section security

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
Tehquickness
Forum Commoner
Posts: 32
Joined: Mon Oct 24, 2005 11:31 pm

first attempt at admin section security

Post by Tehquickness »

Ok so I as you may have seen in a few posts around here about various aspects of this website I am designing, I am working an admin section for a sorortity. I would like to post my login and index page/scripts for analysis on how secure they are. So here we go.

My Log in script:
1.my login script first checks for a login by seeing if the variable formid exhists and if it matches the
formid in the $_SESSION
2. upon varification, it then cleans the user input from the form and checks the database for to make
sure the passwords match
3. if the password matches, then it stores the session variables username and sid and approved and
then it stores the current system id in the database and provides a link to the index page
4. it then checks if the username was either unapproved or "deleted" at which point it will present
and error and explination to the user
5. if the form id exhists but the login fails, then it tells them to try again, and if the formid doesnt
exhist then it presents the login form.

here is the code for that:

Code: Select all

<?php
//open session
session_start();
session_regenerate_id();
$conn = include($_SERVER['DOCUMENT_ROOT']."/webadmin/library/openconndb.php");
if (isset( $_POST['formid']) && $_POST['formid'] == $_SESSION['formid']){
include ($_SERVER['DOCUMENT_ROOT']."/webadmin/header.inc");
//declare variables
$username = $_POST['username'];
$password = $_POST['password'];
$cleanusername = mysql_real_escape_string($username);
$cleanpassword = mysql_real_escape_string($password);

//get login information
$query = "SELECT * from userlogin WHERE username = '$cleanusername'";
$result = mysql_query($query, $conn);
$userinfo =  mysql_fetch_array($result);

if ($userinfo['password'] == $password ){
	if ($userinfo['approved'] == 1 && $userinfo['deleted'] == 0){
		$_SESSION['username'] = $cleanusername;
		$sid = session_id();
		$_SESSION['sid'] = $sid;
		$_SESSION['approved'] = 1;
		$query = "UPDATE userlogin SET sid = '$sid' where username = '$cleanusername'";
		$result = mysql_query($query, $conn);
		print "Authentification Accepted<br />";
		print "Proceed to <a href=\"http://www.millergirls.org/webadmin/index.php?pageid=index\">Administrator Home</a>";
	}elseif( $userinfo['approved'] == 0 && $userinfo['deleted'] != 1){
		print "Your account has not yet been validated by an Admin.<br /> \n You will recieve and email when this is done.";
		print "\n <br /> Feel free to contact the Webmaster for further information.";
	}elseif( $userinfo['deleted'] == 1){
		print "This account has been deleted";
		print "Please contact the webmaster if you feel this is erroneous";
	} 
}else{
$formid = sha1(md5(mt_rand(100000, 999999)));
$_SESSION['formid'] = $formid;
?>
Login in Failed<br />
Try Again <br />
<form action="login.php" method="POST">
	Username: <input type="text" name="username" /><br />
	Password: <input type="password" name="password" /><br />
	<input type="hidden" name="formid" value="<?php print $formid; ?>" />
	<input type="submit" />
</form>

<?php
}
}else{
$formid = sha1(md5(mt_rand(100000, 999999)));
$_SESSION['formid'] = $formid;
include ($_SERVER['DOCUMENT_ROOT']."/webadmin/header.inc");
?>
<form action="login.php" method="POST">
	Username: <input type="text" name="username" /><br />
	Password: <input type="password" name="password" /><br />
	<input type="hidden" name="formid" value="<?php print $formid; ?>" />
	<input type="submit" />
</form>
<?php
}
include ($_SERVER['DOCUMENT_ROOT']."/webadmin/footer.inc");
?>
Now for the index page:
1. check for the exhistance of session variable sid
2. compare session['sid'] to the sid stored in the database for that user
3. if they match then the switch determines which page is needed by the pageid present (ex.
wwww.com?pageid=index)
4. include the file that would have that sections information
5. if the session['sid'] is not set then it presents the error and offers the link to the login page.

Here is the code for that:

Code: Select all

<?php
session_start();

if (isset($_SESSION['sid'])){
	$sid = $_SESSION['sid'];
	$username = $_SESSION['username'];
	$conn = include($_SERVER['DOCUMENT_ROOT']."/webadmin/library/openconndb.php");
	$query = "SELECT * from userlogin WHERE username = '$username'";
	$result = mysql_query($query, $conn);
	$userinfo =  mysql_fetch_array($result);
	if ( $username == $userinfo['username'] && $sid == $userinfo['sid']){

switch ($pageid){
	case 'index':
		print "index page";
	break;
	case 'addmember':
		include ($_SERVER['DOCUMENT_ROOT']."/webadmin/addmember.inc");
		break;
	case 'editmember':
		print "edit member page";
	break;
	case 'findmember':
		print "find member page";
	break;
	default:
		print "Sorry you have selected an incorrect page.";
	break;
}



	}else{
		print "An attempt at invalid access has been made<br /> \n The Webmaster has been notified<br />\n";
	}

}else{
	print "Your login has expired<br /> \n Please <a href=\"login.php\">login</a> again \n ";
}

?>
Let me know what you think, I thought this was pretty well thought out but I probably missed some big holes some where
Last edited by Tehquickness on Tue Feb 21, 2006 4:10 pm, edited 1 time in total.
User avatar
Buddha443556
Forum Regular
Posts: 873
Joined: Fri Mar 19, 2004 1:51 pm

Post by Buddha443556 »

Code: Select all

$username = strip_tags($_POST['username']);
$password = strip_tags($_POST['password']);

//get login information
$conn = include($_SERVER['DOCUMENT_ROOT']."/webadmin/library/openconndb.php");
$query = "SELECT * from userlogin WHERE username = '$username'";
strip_tags() does not prevent SQL injection use mysql_real_escape_string().
Tehquickness
Forum Commoner
Posts: 32
Joined: Mon Oct 24, 2005 11:31 pm

Post by Tehquickness »

ok I will have to research the difference, but i have changed it.

I have changed the code above to reflect the changes mentioned.
Post Reply