Login Form

Hi all,

I am building a login form and would be delighted if you could advise on my code below. Is it secure or are there errors in it?
Thanks

<?php
session_start();
/***********************
 declare variables
***********************/
$username = $_POST['username'];
$password = $_POST['password'];
$submit = $_POST['submit'];

// declare an empty error array
$error_message = array();

// if form submitted
if(isset($submit))
{
 		 /*************************
		  Form Error Checking
		 *************************/
     if(!$username || $username == "")
     {
        $error_message['username'] = 'Please Enter Username';
     }
     if(!$password || $password == "")
     {
        $error_message['password'] = 'Please Enter Password';
     }
		 /*************************
		  End Form Checking
		 *************************/

    /*********************************
     If No Form Errors
    *********************************/
    if(count($error_message) == 0)
    {
        // include database connection
        include('includes/db_connect.php');
        
        // perform query
        $query = "SELECT * FROM ".
        			 	 "members ".
        				 "WHERE username= '$username' ".
        				 "AND password= '$password'";
        $result = mysql_query($query) or die(mysql_error());
        /*print $query;*/
        $num_results = mysql_num_rows($result);
		/*print $num_results;*/
        if ($num_results > 0)
        {
         	 // if they are in the database register the user
        	 $HTTP_SESSION_VARS['valid_user'] = $username;
        	 
        	 // Log site visit
        	 for($i=0; $i<$num_results; $i++)
        	 {
        	 	 $row = mysql_fetch_array($result);
        	 	 $visitcount = $row['numberVisits'];
        	 }
					 
        	 $visitcount += 1;					 
        	 $query1 = "UPDATE members ".
        	 				 	 "SET numberVisits = '".$visitcount."' ".
										 "WHERE username = '".$username."'";
           $result1 = mysql_query($query1) or die(mysql_error());
					 
					 //echo $query1.'<br />';										 										 
        }// end if

	} // end if count
}// end isset $submit
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" 
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>

<h1>Members Area Login</h1>

<?php
if ((isset($submit)) && (count($error_message) == 0))
{
 	 // check if session variable registered
	 if (isset($HTTP_SESSION_VARS['valid_user']))
	 {
	    $HTTP_SESSION_VARS['valid_user'] = $username;
	 		echo '<p>You are logged in as: '.$HTTP_SESSION_VARS['valid_user'].'</p>';
			session_register('valid_user');
			
			if($username == "admin") // check if signed in as admin.
			{
				echo '<p><ul>'.
					 '<li><a href="fileupload.php">Upload File</a></li> '.
					 '<li><a href="new-member.php">Add New Member</a></li> '.					 
					 '</ul>';
					 echo '<p><a href="index.php">Return to Login Page</a></p>';		
			}
			else
			{
			 		// view docs
					echo '<p><a href="policy.php">Policy</a><br />';
					echo '<p><a href="education-training.php">Education and Training</a><br />';					
			
					echo '<p>Number of Logins: '.$visitcount.'</p>';
					echo '<p><a href="index.php">Return to Login Page</a></p>';
			}

		}
		else {
			echo '<p>Incorrect Username/Password<br />';
			echo '<a href="index.php">Go Back</a></p>'; 
		}
}
else
{
	session_destroy();

?>
<form action="index.php" name="frm_login" id="frm_login" method="post">

		<fieldset>
					<legend>Login Form - Members Area</legend>
					
					<p>
					<?php if(isset($error_message['username'])) { echo  $error_message['username']; }?>
					<label for="username">Username:</label>
					<input type="text" name="username" id="username" value="<?php if(isset($username)) { echo $username; } ?>" /></p>

					<p>
					<?php if(isset($error_message['password'])) { echo  $error_message['password']; }?>
					<label for="password">Password:</label>
					<input type="password" name="password" id="password" value="<?php if(isset($password)) { echo $password; } ?>" /></p>

					<p><input type="submit" name="submit" id="submit" class="btn" value="Login" /></p>
		</fieldset>

</form>
<?php
}// end else
?>

First thing I’ve noticed is the isset of $_POST[‘Submit’];

I assume its your submit button? - If so IE has a bug. If your user hits return when the cursor is in a text box IE will not submit the button value to your script. In other words isset() will return false and that code will not be executed.

When ever you’re doing a form like that where a user might just hit enter with the cursor in a text box use a hidden form element called submit instead.

Isn’t that only with a button element, and not an input type=“button” element?

No its with input type=“submit”.

IE doesn’t send the value if the user hits enter when the cursor is still in a text box. All other browsers do.

A few things spring to mind:

  1. The code is vulnerable to SQL injection attacks, the values for username and password would need to be sanitized ( NEVER TRUST ANY USER INPUT ), at the very least with mysqli_real_escape_string() ideally you shoud use prepared statements to prevent SQL injection attacks,[URL=“http://uk3.php.net/manual/en/security.database.sql-injection.php”] see here for more info.
  2. You do a SELECT * and then use mysql_num_rows to see how many matches there were, it may be quicker (would need testing) to do a SELECT COUNT(*) query for the username and password combination and then check the count to make sure that there is exactly 1 match
  3. The use of $HTTP_SESSION_VARS was depreciated in version 4.1.0 of PHP, you should be using the $_SESSION superglobal
  4. The use of session_register() is deprecated as of version 5.3.0 of php, you should use as $_SESSION eg $_SESSION[‘username’] = $username; (after having sanitized whatever your going to place into the session, especially if your storing sessions in a database.
  5. Have a read of this about string parsing
  6. Consider making use of hashing when storing passwords in a database and the use of a salt see this thread for an example.
  7. Try to make use of indenting and leading-commas when writing SQL queries:
SELECT
      user_name
    , password
    , email
FROM
    da_table
WHERE
    user_name = $sanitized_user_name
AND
    password = $sanitized_password