Login System

Chris_Muench

Are their any major problems you notice with this code: (I have many files because this is just a start to the re write of my program) I just need to know if their are any security issues or bad coding.

login.php

<html>
<head>

</head>

<body>
<?php
session_start();

include ("config.php");
include ("classes/db_functions.php");
include ("classes/security_functions.php");

//create two objects that are needed in this script
$dbf=new db_functions($cfg_server,$cfg_username,$cfg_password,$cfg_database);
$sec=new security_functions();

if(isset($_POST['username']) and isset($_POST['password']))
{
	$username = $_POST['username'];
	$password = $_POST['password'];

	if($sec->checkLogin($dbf,$username,$password))
	{
	 	$_SESSION ['user'] = $username;
		header ("location: index.php");
	}
    else
    {
  	 	echo '<center>username or password are incorrect</center>';
	}
}

elseif($sec->isLoggedIn())
{
	header ("location: index.php");
}
else
{
?>
<center>
	<form name="login" action="login.php" method="POST">
		<table width="348" border="0" cellspacing="2" cellpadding="0">
			<tr>
				<td width="85">
					Username:
				</td>
				<td>
					<input type="text" name="username" size="24">
				</td>
			</tr>
			<tr>
				<td width="85">
					Password:
				</td>
				<td>
					<input type="password" name="password" size="24">
				</td>
			</tr>
		</table>
		<input type="submit" name="Submit" value="Log In"> 
	</form>
</center>

<?php
}

?>
</body>
</html>

index.php

<?php
session_start();
include ("config.php");
include ("classes/db_functions.php");
include ("classes/security_functions.php");


$dbf=new db_functions($cfg_server,$cfg_username,$cfg_password,$cfg_database);
$sec=new security_functions();

if(!$sec->isLoggedIn())
{
	header ("location: login.php");
}

?>

db_functions.php

<?php
class db_functions
{	
	//class variable that represents the database connection.
	var $conn;

//user-defined constructor
function db_functions($server,$username,$password,$database)
{
	//pre: parameters must be correct in order to connect to database.
	//post: connects to database.

	$this->conn = mysql_connect("$server", "$username", "$password") or die("Could not connect : " . mysql_error());
	mysql_select_db("$database",$this->conn) or die("Could not select database <b>$database</b>");

}


}

?>

security_functions.php

<?php


class security_functions
{
	//defalt constructor
	function security_functions()
	{


}

function isLoggedIn()
{
	if(isset($_SESSION['user']))
	{
		return true;
	}
	return false;
}

function checkLogin($dbf,$username,$password)
{
	//pre: $dbf must be a db_functions object and $username and password must be strings
	//post: returns boolean based on if their login was succesfull.

	$result = mysql_query ("SELECT * FROM users WHERE username='$username' and password='$password'",$dbf->conn);	

	$num = @mysql_num_rows($result);

	if($num > 0)
	{
		return true;
	}

	return false;
}



}

Thanks,

Chris Muench

planetsim

Im a bit weary about this function "isLoggedIn() "

You are only checking if the user has a session registered. You should also check that the user session matches maybe a userid or username to make sure they are not only logged in but registered.

Also you must encrypt your passwords either by md5() which just hashes or there is mcrypt not sure on others

Weedpacket

Originally posted by planetsim
Also you must encrypt your passwords either by md5() which just hashes or there is mcrypt not sure on others

There's also PHP's native support of SHA1, which happens to be what the NIST recommends, but that's a more recent addition.

Chris_Muench

How do you dycrpt passwords inserted into the database (md5)

Weedpacket

Originally posted by Chris Muench
How do you dycrpt passwords inserted into the database (md5)

You can't. That's the idea.

Shrike

Then the client asks "can we have a remind password feature please?" 😃

planetsim

And you create a random password function

function rnd_Pass()
{
  srand(microtime() * 1000000000000);
  $pass_rnd = array('a','b','c',
	'd','e','f',
	'g','h','i',
	'j','k','l',
	'm','n','o',
	'p','q','r',
	's','t','u',
	'v','w','x',
	'y','z','A',
	'B','C','D',
	'E','F','G',
	'H','I','J',
	'K','L','M',
	'N','O','P',
	'Q','R','S',
	'T','U','V',
	'W','X','Y',
	'Z','0','1',
	'2','3','4',
	'5','6','7',
	'8','9');
  $rnd = array_rand($pass_rnd, 8);
  $password = "";
  for($i=0;$i<=7;$i++){
    $password .= $pass_rnd[$rnd[$i]];
  }
  return $password;
}

Quite a basic one there

agoe

Originally posted by planetsim
Im a bit weary about this function "isLoggedIn() "

You are only checking if the user has a session registered. You should also check that the user session matches maybe a userid or username to make sure they are not only logged in but registered.

i disagree. if the user types the correct username and password, the session-variable is set. otherwise it is not.

consider this:

$sql = "SELECT the, fields, you, need FROM your_table WHERE Username='".validiate_function($_POST['username'])."' AND Password='".md5($_POST["password"])."'";
	$result = mysql_query($sql) or die(mysql_error());

if (mysql_num_rows($result) == 1) {
        // the user is authenticated
	$_SESSION['authed'] = 1;
} else {
	echo "wrong username/password";
}

then, on top of every page that needs authentication, just check if $_SESSION['authed'] == 1.

it's as bulletproof as it gets.

Moonglobe

  $pass_rnd = array('a','b','c',
    'd','e','f',
    'g','h','i',
    'j','k','l',
    'm','n','o',
    'p','q','r',
    's','t','u',
    'v','w','x',
    'y','z','A',
    'B','C','D',
    'E','F','G',
    'H','I','J',
    'K','L','M',
    'N','O','P',
    'Q','R','S',
    'T','U','V',
    'W','X','Y',
    'Z','0','1',
    '2','3','4',
    '5','6','7',
    '8','9');

$pass_rnd = array_merge(range('a','z'), range('A','Z'), range(0,9));

daveyboy

checking a single session value is fine, as long as u only use it for authentication. whenever you call session_start() your session vars will overwrite any the user has passed in. just make sure that you're checking $_SESSION['user'] not just $user.

one thing: your redirects are incorrect: the location header requires a fully qualified url, e.g. http://www.example.com/index.php

Moonglobe

Originally posted by daveyboy
one thing: your redirects are incorrect: the location header requires a fully qualified url, e.g. http://www.example.com/index.php

in theory yes, but there's near total backward compatibiliy with HTTP/1.0 (that's a 1.1 thing)

planetsim

Originally posted by Moonglobe
$pass_rnd = array_merge(range('a','z'), range('A','Z'), range(0,9));
[/B]

Suppose should update my code.. I remember writing that function like when i first learnt i thought it was the best thing i made and never went and made it better.