[RESOLVED] Login Code Critique - PLEASE

kpowning

session_start();

$checklogged = sprintf("SELECT * FROM session, user WHERE user.username = '".$_SESSION['username']."' AND session.id = user.id");
$logged = mysql_query($checklogged, $erotic) or die(mysql_error());
$loggedin = mysql_num_rows($logged);

if ($loggedin == '1'){
header('location: mempage.php?do=home');
}

if ($_GET['error']==1){
$msg="<img src=\"errors/error1.gif\" width=\"392\" height=\"86\">";
}
if ($_GET['error']==2){
$msg="<img src=\"errors/error2.gif\" width=\"396\" height=\"83\">";
}
if ($_GET['error']==3){
$msg="<img src=\"errors/error1.gif\" width=\"392\" height=\"86\">";
}
if ($_GET['error']==4){
$msg="<img src=\"errors/error3.gif\" width=\"396\" height=\"83\">";
}
if ($_GET['error']==5){
$msg="<img src=\"errors/error4.gif\" width=\"396\" height=\"83\">";
}
 ///////////////////////////////////////
/////////// START LOGIN ////////////
///////////////////////////////////////
if (isset($_POST['Submit'])){

session_start();

function CleanArray($array) {
foreach ($array as $key => $value) {
$array[$key] = mysql_real_escape_string($value);
}
return $array;
} 

$_POST = CleanArray($_POST);

if(get_magic_quotes_gpc()) {
            $username = stripslashes($_POST['username']);
            $password = stripslashes($_POST['password']);
        } else {
            $username = $_POST['username'];
            $password = $_POST['password'];
        }

///////////////////////////////////////////
/// Check if user is already logged in! ///
///////////////////////////////////////////

$checkin = sprintf("SELECT * FROM session, user WHERE user.username = '$username' AND session.id = user.id");	  

$chkin = mysql_query($checkin, $erotic) or die(mysql_error());
$checkerisin = mysql_num_rows($chkin);

if ($checkerisin == '1'){
header('Location: mempage.php');
} else {

/////////////////////////////////////////////////////////
/// Check if session exists - for duplicate sessions! ///
/////////////////////////////////////////////////////////

$query = sprintf("SELECT * FROM session WHERE sid='".session_id()."'");
$resultsess = mysql_query($query, $erotic) or die(mysql_error());
$chksess = mysql_num_rows($resultsess);

if ($chksess>0){
header('Location: mempage.php?error=2');
exit;
} else {

$FF_LoginAction = $HTTP_SERVER_VARS['PHP_SELF'];
if (isset($HTTP_SERVER_VARS['QUERY_STRING']) && $HTTP_SERVER_VARS['QUERY_STRING']!="") $FF_LoginAction .= "?".htmlentities($HTTP_SERVER_VARS['QUERY_STRING']);
if (isset($_POST['username'])) {

  $FF_fldUserAuthorization="";
  $FF_redirectLoginSuccess="/members/mempage.php?do=home";
  $FF_redirectLoginFailed="/members/index.php?error=1";
  $FF_password = md5($password);  //encrypte password
  $FF_rsUser_Source=sprintf("SELECT username, password FROM user WHERE username = '$username' AND password = '$FF_password'");

  if ($FF_fldUserAuthorization != "") $FF_rsUser_Source .= "," . $FF_fldUserAuthorization;
  $FF_rsUser=mysql_query($FF_rsUser_Source, $erotic) or die(mysql_error());
  $row_FF_rsUser = mysql_fetch_assoc($FF_rsUser);
  if((mysql_num_rows($FF_rsUser) > 0)&&($row_FF_rsUser['username'] == $username)) {
// username and password match - this is a valid user

/////////////////////////////////////////
// Update last activity//////////////////
/////////////////////////////////////////
$updateuserlast = sprintf("UPDATE user SET lastactivity = now() WHERE username = '$username'");
mysql_query($updateuserlast, $erotic) or die(mysql_error());
/////////////////////////////////////////

$_SESSION['username']=$username;
if ($FF_fldUserAuthorization != "") {
  $MM_UserAuthorization=$row_FF_rsUser[$FF_fldUserAuthorization];
} else {
  $MM_UserAuthorization="";
}
session_register("MM_UserAuthorization");
if (isset($accessdenied) && false) {
  $FF_redirectLoginSuccess = $accessdenied;
}
mysql_free_result($FF_rsUser);
session_register("FF_login_failed");
$FF_login_failed = false;
header ("Location: $FF_redirectLoginSuccess");
exit;
  }
  mysql_free_result($FF_rsUser);
  session_register("FF_login_failed");
  $FF_login_failed = true;
  header ("Location: $FF_redirectLoginFailed");
  exit;
			}
		}
	}
}

bradgrafelman

Instead of doing "SELECT *", I'd recommend that you always SELECT only the columns you actually need data from. Since in the first part of the script you don't actually need data from any of the columns, you could SELECT something arbitrary and simple, like the number 1.
You use $SESSION, $GET, $_POST, etc. variables before ever checking if they even exist (e.g. with [man]isset/man). If they do not exist when you try to use them, PHP issues an E_NOTICE level error message.
How much filtering do you do on the users' usernames? Using $_SESSION['username'] in a query could potentially expose you to SQL injection attacks and/or errors.
Why are you using sprintf() when you never use any format strings within the query string? Either ditch the sprintf() call since it's pointless at present, or actually utilize the features of the [man]sprintf/man function; personally, I use sprintf() when building queries with the MySQL library since it makes the code cleaner/easier to edit.
In a production environment, you should never display either PHP error messages or MySQL error messages, thus I hope you plan to remove the mysql_error() references when you actually go to use this code.
You use the CleanArray() function to apply [man]mysql_real_escape_string/man before you check if magic_quotes_gpc is enabled and stripslashes() if so; this should be done in reverse order else you might remove too many slashes.
Speaking of the CleanArray function, why apply [man]mysql_real_escape_string/man to the original data? The main benefit of using sprintf() for me is that it provides and easy and clean way to sanitize data as it is used in a SQL query; modifying the data makes it harder to use the data in its original format.
All of a sudden you start using the deprecated $HTTP*VARS variables; I would higly recommend you not use these and stick to the new superglobals ($GET, $POST, $_SERVER, etc. - more info at: [man]variables.superglobals[/man]).
You perform an unnecessary check in your if() statement after the $row_FF_rsUser MySQL query; you check if $row_FF_rsUser['username'] == $username even though that was a condition in the SQL SELECT statement ("... WHERE username = '$username' ..."); did you think MySQL would try to trick you or something and that you'd need to double check this? :p
You mix using $_SESSION as well as [man]session_register/man; not only is it recommended to stick to one or the other in the manual, but you are encouraged not to ever use [man]session_register/man.
Although it does work as intended, this:
```
if (isset($accessdenied) && false)
```
is a rather weird way of doing it... you'll probably see it more commonly written as:
```
if(!isset($accessdenied))
```
since the second line is much easier to understand at a glance (at least, to me it is).

cahva

To add to bradgrafelman's post.. You have used session_start() twice in your code, dump the other one. session_start() in the beginning of the script is enough.

kpowning

Here is what I have now:

I have updated everything, but I'm new to the mysql_real_escape_string stuff. How should I still incorporate this correctly? I'm I doing it right or not. Can you please show me an example that is correct with one of my scripts.

Also you where saying there can be a mysql injection attack by using $_SESSION['username']; can you please explain this. I use this variable throughout my members area to get all of their information. What would be a better way?

** I will remove all mysql error results once script is completed. So you never want to show the mysql_error?

Thanks again.

error_reporting(E_ALL);

session_start();

$msg = ''; // DEFINE DEFAULT ERROR MESSAGE

///////////////////////////////
/// CHECK IF SESSION EXISTS ///
if (!empty($_SESSION['username'])){

$checklogged = sprintf("SELECT session.id FROM session, user WHERE user.username = '".$_SESSION['username']."' AND session.id = user.id"); 
$logged = mysql_query($checklogged, $erotic) or die(mysql_error()); 
$loggedin = mysql_num_rows($logged); 

   if ($loggedin == '1'){ 
      header('location: mempage.php?do=home'); 
   } 
}
/// END SESSION EXISTS ///
//////////////////////////

/////////////////////////////
/// DEFINE ERROR MESSAGES ///
if (isset($_GET['error'])){ // CHECK IF ERROR WAS RECEIVED

if ($_GET['error']==1){ 
$msg="<img src=\"errors/error1.gif\" width=\"392\" height=\"86\">"; 
} 
if ($_GET['error']==2){ 
$msg="<img src=\"errors/error2.gif\" width=\"396\" height=\"83\">"; 
} 
if ($_GET['error']==3){ 
$msg="<img src=\"errors/error1.gif\" width=\"392\" height=\"86\">"; 
} 
if ($_GET['error']==4){ 
$msg="<img src=\"errors/error3.gif\" width=\"396\" height=\"83\">"; 
} 
if ($_GET['error']==5){ 
$msg="<img src=\"errors/error4.gif\" width=\"396\" height=\"83\">"; 
}
}
/// END ERROR MESSAGES ///
//////////////////////////

////////////////////////////////////
/////////// START LOGIN //////////// 
if (isset($_POST['Submit'], $_POST['username'], $_POST['password'])){ // CHECK POST Submit, username, password

$FF_LoginAction = $_SERVER['PHP_SELF']; // DEFINE FORM POST TO SELF

if(get_magic_quotes_gpc()) { 
   $username = stripslashes($_POST['username']); 
   $password = stripslashes($_POST['password']); 
 } else { 
   $username = $_POST['username']; 
   $password = $_POST['password']; 
} 

function CleanArray($array) { 
   foreach ($array as $key => $value) { 
   $array[$key] = mysql_real_escape_string($value);  

   } 
   return $array; 
} 

$_POST = CleanArray($_POST); 

/////////////////////////////////////////// 
/// CHECK IF USER IS LOGGED IN ALREADY! /// 
$checkin = sprintf("SELECT session.id FROM session, user WHERE user.username = '$username' AND session.id = user.id");       

$chkin = mysql_query($checkin, $erotic) or die(mysql_error()); 
$checkerisin = mysql_num_rows($chkin); 

if ($checkerisin == '1'){ 
header('Location: mempage.php?do=home'); 
} else { 

///////////////////////////////////////////// 
/// CHECK FOR DUPLICATE SESSIONS FOR USER /// 
$query = sprintf("SELECT id FROM session WHERE sid='".session_id()."'"); 
$resultsess = mysql_query($query, $erotic) or die(mysql_error()); 
$chksess = mysql_num_rows($resultsess); 

if ($chksess>0){ 
header('Location: index.php?error=2'); 
exit; 
} else { 

if (isset($_SERVER['QUERY_STRING']) && $_SERVER['QUERY_STRING']!="") $FF_LoginAction .= "?".htmlentities($_SERVER['QUERY_STRING']); 

  $FF_fldUserAuthorization=""; 
  $FF_redirectLoginSuccess="/members/mempage.php?do=home"; 
  $FF_redirectLoginFailed="/members/index.php?error=1"; 
  $FF_password = sha1($password);  //encrypte password 
  $FF_rsUser_Source=sprintf("SELECT username, password FROM user WHERE username = '$username' AND password = '$FF_password'"); 

 if ($FF_fldUserAuthorization != "") $FF_rsUser_Source .= "," . $FF_fldUserAuthorization; 
  $FF_rsUser=mysql_query($FF_rsUser_Source, $erotic) or die(mysql_error()); 
  $row_FF_rsUser = mysql_fetch_assoc($FF_rsUser); 

 if (mysql_num_rows($FF_rsUser) > 0){ // USER EXISTS PROCEED
     $updateuserlast = sprintf("UPDATE user SET lastactivity = now() WHERE username = '$username'"); 
     mysql_query($updateuserlast, $erotic) or die(mysql_error()); 

 $_SESSION['username']=$username; // REGISTER SESSION

 if ($FF_fldUserAuthorization != "") { 
     $MM_UserAuthorization=$row_FF_rsUser[$FF_fldUserAuthorization]; 
 } else { 
     $MM_UserAuthorization=""; 
 }

$_SESSION['MM_UserAuthorization'] = $MM_UserAuthorization; // REGISTER SESSION

 if (isset($accessdenied) && false) { 
     $FF_redirectLoginSuccess = $accessdenied; 
 } 

mysql_free_result($FF_rsUser); 
$FF_login_failed = false; 
$_SESSION['FF_login_failed'] = $FF_login_failed; // REGISTER SESSION
header ("Location: $FF_redirectLoginSuccess"); 
exit; 
 } 
    mysql_free_result($FF_rsUser);  

    $FF_login_failed = true;
    $_SESSION['FF_login_failed'] = $FF_login_failed; // REGISTER SESSION
    header ("Location: $FF_redirectLoginFailed"); 
    exit; 
  } 
 } 
}

cahva

Use mysql_real_escape_string always when using them in some of your queries. As bradgrafelman pointed out, you're not using the sprintf function correctly. This is the right way to way to use it:

$checklogged = sprintf("SELECT session.id FROM session, user WHERE user.username = '%s' AND session.id = user.id",
$_SESSION['username']);

In sql inserts this comes very handy. Heres an example:

$sql = sprintf("INSERT INTO tablename (name,email,homepage) VALUES ('%s','%s','%s')",
mysql_real_escape_string($name),
mysql_real_escape_string($email),
mysql_real_escape_string($homepage));

Its lot cleaner than the usual way:

$sql = "INSERT INTO tablename (name,email,homepage) VALUES ('" . mysql_real_escape_string($name) . "','" . mysql_real_escape_string($email) . "','" . mysql_real_escape_string($homepage) . "')";

Generally you use %s for strings and %d for integer values. Check out the manual page for sprintf function for more info.

About the sql injection attack by using $_SESSION['username'].. Well you should save the user id to the session instead of username and then use the sprintf with %d. That way if the user somehow gets to manipulate session variable(very unlikely though) it would not help the attacker because the query would only accept integer values.

Normal users dont need to know what mysql error the site gives so its better to show them something like "Database error" etc. Attackers can learn from the errors (field names etc.) and try to find a security hole in your site. Less information is better.

Save the errors to some log and maybe send email to yourself to acknowledge that there is an error.

kpowning

Thanks for all your feedback. Have changed my code to reflect.