My User Authentication system - your comments please

rupertbj

Please see the below code which I have made for a simple user authentication system using sessions. I would be very greatful for any comments and suggestions to make it more secure or user friendly.

<form action="authenticate.php" method="post" name="authUser" enctype="multipart/form-data">
<table width=50% align="center">
  <tr bgcolor="#FFFFFF" class="blacktext"> 
    <td width="30%"><div align="right"><strong><font color="#350000" face="Verdana, Arial, Helvetica, sans-serif">Email:</font></strong></div></td>
    <td width="70%" class="blacktext"><font color="#350000" face="Verdana, Arial, Helvetica, sans-serif"> 
      <input type="text" size=25 name="username">
      </font></td>
  </tr>
  <tr valign="top" bgcolor="#FFFFFF" class="blacktext"> 
    <td width="30%" height="26"><div align="right"><strong><font color="#350000" face="Verdana, Arial, Helvetica, sans-serif">Password:</font></strong></div></td>
    <td width="70%" class="blacktext"><font color="#350000" face="Verdana, Arial, Helvetica, sans-serif"> 
      <input name="pwd" type="password" size="25">
      </font></td>
  </tr>
  <tr valign="top" bgcolor="#FFFFFF" class="blacktext"> 
    <td height="26" colspan="2" align="center">
      <font color="#350000" face="Verdana, Arial, Helvetica, sans-serif"> 
        <strong><input type="submit" value="Login"></strong>
        </font></td>
  </tr>
</table>

Authenticate.php

<?php
if (!$username || !$pwd) { print "ERROR - Missing user name or password."; exit; }
    else {
        include("mysql.inc");
       	$result = mysql_query( "SELECT * FROM loginTable WHERE email='$username' && password='$pwd'");
		$num_rows = mysql_num_rows( $result );
        if ($num_rows != "1") { print "ERROR - Your username and password do not match."; exit; }
            else {
				session_start();
				session_register( "username" );
                mysql_close( $link );
                include("newpage.php"); }
		}
?>

newpage.php

<?php include("validate.inc"); ?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<title>Untitled Document</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>

<body>
Welcome to the password protected page!!!! 
<br><a href="nextpage.php">link to next page</a>
</body>
</html>

validate.inc

<?php
session_start();
// print session_encode();
if ( isset( $_SESSION[ 'username' ] ) && ( $_SESSION[ 'username' ] = $username ) )
	{
	}
else
	{
	include("emptyUser.php");
	exit;
	}
?>

nextpage.php

<?php include("validate.inc"); ?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<title>Untitled Document</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>

<body>
This is the second page of the site
</body>
</html>

I hope that all makes sense. It seems to work, but perhaps there there are some security issues I should be aware of?

Hopefully this will develop into an interesting discussion.

Rupert

Houdini

What about this line?

$result = mysql_query( "SELECT * FROM loginTable WHERE email='$username' && password='$pwd'");

I believe unless their username is passed in the form as an email address then how does that part work?

rupertbj

The username is an e-mail address. It is easy this way as everone who has an e-mail address in the whole wide world has a unique one.
R

Houdini

OK then it makes sense.

samudasu

Here's one. You're not doing any checking on the variables passed to your script. This opens your program up to something called sql injection. Using mysql_real_escape_string should help to solve that problem. On validate.inc, where are you coming up with the $username variable? Some of your coding is dated. $SESSION['username'] = $username; is now the preferred method to set session variables, you shouldn't need to use session_register(). You might also want to turn off register_globals and use the current super globals $POST and $_GET to pass variable to your script from forms.
Here's that info on mysql_real_escape_string and sql injection.

http://www.php.net/manual/en/function.mysql-real-escape-string.php

rupertbj

The username variable is registered in authenticate.php. If successful, this is passed to newpage.php, which includes validate.inc at the top.
I will check out the mysql_real_escape_string and sql injection attack - many thanks.
R

thorpe

Nowhere in your code can i see where $username and $pwd are defined. If they are being sent via a form (post method) you should be using the $_POST array to retrieve them. Otherwise, your code will break on most php installs.

This...

if (!$username || !$pwd) {}

Should be...

if (!$_POST['username'] || !$_POST['pwd']) {}

rupertbj

$username and $pwd are defined and stored in a MySql database. This is a separate process altogether.
The entries from the login form are checked against this database and if they match, then login is permitted.
Also, you say that my code should be different - my code works, but is there any particular reason why I should use your code? Is it better performing / more secure / best practice? I'n just making this up as I go along, so would appreciate your comments.

Rupert

samudasu

You should use it because it's most likely better performing, definitely more secure, and best practice according to the php manual and most php developers. Yes, it works now on your machine but if you want it to be portable then take our recommendations. If you move your script to a current default setup of php it will break. Read the php manual about register_globals and sessions.