Page 1 of 1

user login/validation script

Posted: Wed Sep 26, 2007 3:56 pm
by itp
I cobled together a simple user validation/login script using a mySQL database and PHP sessions. I seem to work OK but I'm not sure that it is as secure/safe or fast as it can be. It seems a bit slower in Firefox than IE. Comments welcome.

Code: Select all

<?php
session_start();   

$logoff  	  = (empty($_REQUEST["logoff"]))  	    ?  ""  :  $_REQUEST["logoff"]       ;
$userName  	  = (empty($_REQUEST["userName"]))   	?  ""  :  $_REQUEST["userName"]      ;
$userPassword = (empty($_REQUEST["userPassword"]))  ?  ""  :  $_REQUEST["userPassword"]  ;
$userSession  = (empty($_SESSION["userSession"]))   ?  ""  :  $_SESSION["userSession"]   ;
$message  = "";

// set error reporting
error_reporting(E_ALL); 
ini_set('display_errors', 1); 
     
// user makes attempt to login  
if ( $userName <> '' && $userPassword <> '')
{
	$valid_user = fnValidateUser($userName,$userPassword);
	if ($valid_user==1)
	{
		$_SESSION['userSession'] = $userName;
		$_REQUEST["userName"] ='';
		$userSession  = $userName;
		$message  = '';
	}
	else
	{
		$_SESSION['userSession'] = '';
		$_REQUEST["userName"] ='';
		$_REQUEST["logoff"] ='';
		$userSession  = '';
		$message  = 'Username/password incorrect';
	}	 
}

// show login screen if logout requested or not currently logged in
if ($logoff == 'Y' || $userSession == '')
{
	if ($userSession <> '')
	{
		session_destroy();
		$userSession ='';
		$_SESSION["userSession"] ='';
	}
	$_REQUEST["logoff"] ='';
	$_REQUEST["userName"] ='';
	fnGetlogin($message);
}
 
// successfully signed in
if ($userSession <> '')
{
	echo $userSession . ' is logged in <br>';
	echo "<a href = 'secure3.php?logoff=Y' >logoff</a>  ";
	echo "<a href = 'secure4.php' >another page</a>";
}
 
// show login screen 
function fnGetlogin($message)
{

echo "<p align='center'><font color=red>" . $message . "</font></p>";
echo <<<EOT
	<form action="secure3.php" method="post">
	<p align="center">Please login to access this document.</p>
	<table align="center" border="0">
	 <tr>
	  <th>
	Username:
	  </th>
	  <th>
	<input type="text" name="userName">
	  </th>
	 </tr>
	 <tr>
	  <th>
	Password:
	  </th>
	  <th>
	<input type="password" name="userPassword">
	  </th>
	 </tr>
	 <tr>
	  <th colspan="2" align="right">
	<input type="submit" value="Login">
	</form>
	  </th>
	 </tr>
	</table>
	</body>
	</html>
EOT;
}

// function to validate username & password against SQL database
function fnValidateUser($userName,$userPassword)
{
	// get database parameters
	include_once("include2.php");

	//echo $userName . $userPassword. '<br><br><br><br>';
	$db = mysql_connect($host, $user, $password) or die("Could not connect."); 
	if(!$db) 
		die("no db");
	if(!mysql_select_db($database,$db))
	 	die("No database selected.");

	$sql = mysql_query("SELECT password FROM member WHERE username = '$userName'");
	$fetch_em = mysql_fetch_array($sql);
	$numrows = mysql_num_rows($sql);
	 
	if($numrows != "0" & $userPassword == $fetch_em["password"]) 
	{
		$valid_user = 1;
	}
	else 
	{
		$valid_user = 0;
	}
 	return $valid_user;
}
 
?>
Include2.php contains the following code...

Code: Select all

<?php

if ($_SERVER['DOCUMENT_ROOT'] =='/var/www/html')
{

 $host = 'localhost'     ;
 $user = 'xxx'         ;
 $password = "xxx"   ;
 $database = "xxx"     ;
 $file1 = "xxxx" ;
}

// test server
if ($_SERVER['DOCUMENT_ROOT'] == 'C:/xampp/htdocs')
{
 $host = 'localhost'     ;
 $user = 'xxx'         ;
 $password = "xxx"   ;
 $database = "xxx"     ;
 $file1 = "xxxx" ;
  
}



 $LF = chr(13) . chr(10) ;
 $quote = "'";   
 $q = "'";   
 $dq = "\"";   
 $comma = ", "; 


$db = mysql_connect($host, $user, $password) or die("Could not connect."); 
if(!$db) 
	die("no db");
if(!mysql_select_db($database,$db))
 	die("No database selected.");

if(!get_magic_quotes_gpc())
{
  $_GET = array_map('mysql_real_escape_string', $_GET); 
  $_POST = array_map('mysql_real_escape_string', $_POST); 
  $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}
else
{  
   $_GET = array_map('stripslashes', $_GET); 
   $_POST = array_map('stripslashes', $_POST); 
   $_COOKIE = array_map('stripslashes', $_COOKIE);
   $_GET = array_map('mysql_real_escape_string', $_GET); 
   $_POST = array_map('mysql_real_escape_string', $_POST); 
   $_COOKIE = array_map('mysql_real_escape_string', $_COOKIE);
}
 
?>

Posted: Wed Sep 26, 2007 4:09 pm
by feyd
Your code assumes the super globals are one dimensional arrays. Probably not the best of ideas. ;)

Posted: Wed Sep 26, 2007 6:41 pm
by itp
hmmmm, not sure I follow.

I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);


Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?

Posted: Wed Sep 26, 2007 7:19 pm
by Christopher
I think the two thing that stand out for me are that the code is not well structured and you do not filter input properly. What kind of comments were you looking for?

Posted: Wed Sep 26, 2007 7:41 pm
by Begby
itp wrote:hmmmm, not sure I follow.

I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);


Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?

The problem is that you are running everything through mysql_real_escape_string no matter what it is or where the variable will be used. Sometimes you don't want to escape stuff if you are going to use it in something other than a query.

Also, what if an array gets passed in the post? Your script will bomb. This is what feyd is pointing out.

As for error_reporting, take a look at the comments in php.ini or the documentation on php.net. For development you want to turn on pretty much everything including notices, for a production server you generally do not want any errors/warnings at all.

Posted: Wed Sep 26, 2007 8:01 pm
by itp
These are exactly the type of comments that I was looking for. I want to write code
that more than just works. I want well structured, maintainable, scalable
and secure code as well. These things that are not always obvious at first pass.

Begby's comments are very illustrative.

Posted: Thu Sep 27, 2007 11:20 am
by pickle
  • I'd have all your variables that you set at the top be boolean FALSE if the required variable is not found. It seems a bit cleaner, both code-wise and semantically. By doing so, your first if condition can be changed to:

    Code: Select all

    if($userName && $userPassword)
  • Have fnValidateUser() return boolean TRUE or FALSE rather than 1 or 0 - again it seems cleaner. Again, your if condition could then be cleaned up:

    Code: Select all

    if(fnValidateUser($userName,$userPassword)
    {...}
    else
    {...}
  • Is $logoff ever going to exist if the user doesn't want to logoff? If not, you don't need to check for it's value, just it's existence:

    Code: Select all

    if($logoff || !$userSession)
  • Why do you have 2 echo statements one right after the other in fnGelogin(). heredocs can interpret variables, so you can put $message in your heredocs. Strictly speaking, fnGetlogin() should be named fnGetLogin() to be proper camel case.
  • Include2.php: is it just for this application? If so, what are $LF, $quote, $q, $dq, $comma for? They're not used anywhere. Also, you connect to a database in the include2 file, as well as in your main file.
  • If you want to keep the db connection in include2.php, you can get rid of the superfluous if...die condition.

    Code: Select all

    if(!$db)...die("no db");
    is unnecessary
  • ~feyd is correct in that your assumption that $_GET, $_POST, and $_COOKIE are one dimensional. I also personally consider it bad form to overwrite super globals. I treat superglobals as unmodifiable & escape them as necessary.