Page 1 of 1

Log in code... Do you see gaps

Posted: Sat Aug 22, 2009 4:24 pm
by micknc
I am working on a small web app for our company and I think the login area is our biggest threat for the out side world. What changes would your recommend on the following code:

First the login page:

Code: Select all

 session_start();
$time = microtime() + (3467851);
$token = md5($time);
session_start("token"); $HTTP_SESSION_VARS["token"]=$token;
$sessionID = session_id();
$expire = time() + (300);
include "../includes/connect.php";
mysql_query("INSERT INTO token (sessID, token, expire) VALUES ('$sessionID', '$token', '$expire')") or die(mysql_error());
I am setting the token into the session to keep from having to pass it in the form in a hidden field. Is that the best approach? The whole reason I did this was to knock down the cross domain attacks.

The rest of the login page is just a normal form but I do have an AJAX validation script that checks that the username and password are alphanumeric. If they are not then the user is notified and they are not allowed to submit.

The checklogin page (with some notes):

Code: Select all

 session_start();
include ("../includes/connect.php");
$sessID = session_ID();
$sql1="SELECT * FROM token WHERE sessID='$sessID' and token='$token'";
$result1=mysql_query($sql1);
$current=mysql_num_rows($result1);
$expire = time();
 
//I delete the tokens to stop multiple submits from someone from having more than one shot at getting in without going to my login screen everytime.
mysql_query("DELETE FROM token WHERE sessID = '$sessID'");
 
//this is to clean up a token from someone who got to the login screen but closed their browser.
mysql_query("DELETE FROM token WHERE expire < '$expire'");
 
if($current!=1){session_destroy(); die("<meta http-equiv='REFRESH' content='0;url=login.php'>");}
else{}
 
// username and password sent from form
$myusername=$_POST['myusername'];
$mypassword=$_POST['mypassword'];
 
 
// To protect MySQL injection
$myusername = stripslashes($myusername);
$mypassword = stripslashes($mypassword);
$myusername = mysql_real_escape_string($myusername);
$mypassword = mysql_real_escape_string($mypassword);
 
$sql="SELECT * FROM user WHERE userName='$myusername' and password='$mypassword'";
$result=mysql_query($sql);
 
// Mysql_num_row is counting table row
$count=mysql_num_rows($result);
// If result matched $myusername and $mypassword, table row must be 1 row
 
if($count==1){
$stamp = date("m-d-Y - g:i a");
$row = mysql_fetch_array($result);
  $userID = $row['ID'];
    $name = $row['fName'];
    $admin = $row['admin'];
session_register("userID");
session_register("name");
session_register("admin");
mysql_query("UPDATE user SET lastLogin='$stamp' WHERE ID='$userID'");
echo "<meta http-equiv='REFRESH' content='0;url=../index.php'>";
}
else {
echo "<meta HTTP-EQUIV='REFRESH' content='0; url=login.php?error=1'>";
} 
Then inside all of my pages I call this to check to see if they are logged in:

Code: Select all

 session_start();
if(!session_is_registered(name)){
header("location:http://www.fwbgo.net/smart/login/login.php");
} 
Have I approached this in the right way? Any tweaks?
I am still learning so let me have it!

Re: Log in code... Do you see gaps

Posted: Sat Aug 22, 2009 5:58 pm
by Darhazer
It seems that this is PHP 4 code, which is old and unsupported version.
Also it seems that it relies on register_globals, which is security issue by itsself.

Re: Log in code... Do you see gaps

Posted: Sun Aug 23, 2009 8:04 am
by micknc
hmmm... what specifically do you see that is PHP 4? I started learning PHP during 4 so I am sure it is just an old habit. Also parts of this login are copies from an old login I used (that probably came from a tut somewhere) so it could have snippets from that would be 4.

I am guessing the globals you are talking about are the session variables. Is there another way to pass the token that you can recommend? If I put it in a hidden field it seems to bypass the idea of an unknown piece to the puzzle.

Thanks for taking time to look at the code.

Re: Log in code... Do you see gaps

Posted: Mon Aug 24, 2009 4:55 am
by Mordred
Multiple SQL injections ($sessID, $token)
Anti-CSRF tokens not random, although microtime() + (3467851) is a nice twist - but the 3467851 constant should be kept secret, or the tokens could be bypassed. Better yet - use a true random.
session fixation (read on session_regenerate_id)
The redirection does not stop the rest of the page from being executed, use exit()

Also DO pay attention to the other comments - you're using old-style code with register_globals and session_register, which probably means you're reading too old books.

Re: Log in code... Do you see gaps

Posted: Mon Aug 24, 2009 7:10 am
by micknc
Let me work piece by piece through your suggestions just to make sure my understanding is correct:

"Multiple SQL injections ($sessID, $token)"
I thought (naively maybe) that I only needed to escaped user input. I guess the session var can be spoofed in some way?
I have changed:

Code: Select all

# $sessID = session_ID();
# $sql1="SELECT * FROM token WHERE sessID='$sessID' and token='$token'";
to:

Code: Select all

$sessID = mysql_real_escape_string(session_ID());
$token = mysql_real_escape_string($token);
$sql1="SELECT * FROM token WHERE sessID='$sessID' and token='$token'";
 
"Anti-CSRF tokens not random, although microtime() + (3467851) is a nice twist - but the 3467851 constant should be kept secret, or the tokens could be bypassed. Better yet - use a true random."
I would have changed + (3467851) but your point is taken that rand is a better approach. I have changed it from time to:

Code: Select all

$num = rand(500, 1000);
$token = md5($num);
"session fixation (read on session_regenerate_id)
The redirection does not stop the rest of the page from being executed, use exit()"
I was using die() for the redirection. I have changed that to exit(). I have also added the regeneration:

Code: Select all

if($current!=1){session_destroy(); session_regenerate_id(); exit("<meta http-equiv='REFRESH' content='0;url=login.php'>");}
 
"It seems that this is PHP 4 code, which is old and unsupported version.
Also it seems that it relies on register_globals, which is security issue by itsself."
I have changed:

Code: Select all

session_start("token"); $HTTP_SESSION_VARS["token"]=$token;
to:

Code: Select all

$_SESSION["token"]=$token;
Does that cover the earlier PHP that you saw. Obviously this one was old (after reading the man) but were there other instances that were troubling?

Thanks everyone for taking the time to help me improve. I have pasted the code one more time in context to make it a little easier to understand where I am now:

Login page:

Code: Select all

session_start();
$num = rand(500, 1000);
$token = md5($num);
$_SESSION["token"]=$token;
$sessionID = session_id();
$expire = time() + (300);
include "../includes/connect.php";
mysql_query("INSERT INTO token (sessID, token, expire) VALUES ('$sessionID', '$token', '$expire')") or die(mysql_error());
Login Page also some AJAX/Javascript that verifies that the username and password fields are alpha numeric only.

Check the login:

Code: Select all

<?php
session_start();
include ("../includes/connect.php");
$sessID = mysql_real_escape_string(session_ID());
$token = mysql_real_escape_string($token);
mysql_query("DELETE FROM token WHERE expire < '$expire'");
 
$sql1="SELECT * FROM token WHERE sessID='$sessID' and token='$token'";
$result1=mysql_query($sql1);
$current=mysql_num_rows($result1);
$expire = time();
mysql_query("DELETE FROM token WHERE sessID = '$sessID'");
if($current!=1){session_destroy(); session_regenerate_id(); exit("<meta http-equiv='REFRESH' content='0;url=login.php'>");}
else{}
 
// username and password sent from form
$myusername=$_POST['myusername'];
$mypassword=$_POST['mypassword'];
 
 
// To protect MySQL injection (more detail about MySQL injection)
$myusername = stripslashes($myusername);
$mypassword = stripslashes($mypassword);
$myusername = mysql_real_escape_string($myusername);
$mypassword = mysql_real_escape_string($mypassword);
 
$sql="SELECT * FROM user WHERE userName='$myusername' and password='$mypassword'";
$result=mysql_query($sql);
 
// Mysql_num_row is counting table row
$count=mysql_num_rows($result);
// If result matched $myusername and $mypassword, table row must be 1 row
 
if($count==1){
$stamp = date("m-d-Y - g:i a");
// Get user info, Register id in session and redirect to file "login_success.php"
$row = mysql_fetch_array($result);
  $userID = $row['ID'];
    $name = $row['fName'];
    $admin = $row['admin'];
session_register("userID");
session_register("name");
session_register("admin");
//header("location:../smart.php");
mysql_query("UPDATE user SET lastLogin='$stamp' WHERE ID='$userID'");
echo "<meta http-equiv='REFRESH' content='0;url=../smart.php'>";
}
else {
echo "<meta HTTP-EQUIV='REFRESH' content='0; url=login.php?error=1'>";
}
?>
 
In the header I check for a login status by:

Code: Select all

<?php 
session_start();
if(!session_is_registered(name)){
header("location:http://www.fwbgo.net/smart/login/login.php");
}

Re: Log in code... Do you see gaps

Posted: Mon Aug 24, 2009 8:01 am
by Mordred
I didn't mean to change die to exit. I meant to use exit after header("location...") in your last piece of code. The html-based redirection you use in other places is not very reliable btw, use output buffering and header(location...'); exit();

Code: Select all

$num = rand(500, 1000);
$token = md5($num);
This is now worse than before, as there are only 500 possible tokens. Don't limit the random in such a way. Use mt_rand() at least, if not a real cryptographic random (IMO mt_rand is okay for you)

Code: Select all

<?php
session_start();
include ("../includes/connect.php");
$sessID = mysql_real_escape_string(session_ID());
$token = mysql_real_escape_string($token);
mysql_query("DELETE FROM token WHERE expire < '$expire'");
...
If $expire comes from register_globals, then you have SQL injection with it as well.

Re: Log in code... Do you see gaps

Posted: Mon Aug 24, 2009 8:43 am
by micknc
Again, thanks for the help.

That $expire was coming from a time() function. I had moved that delete up from line 12 so that the expired sessions were deleted before the token was searched but I forgot to move my variable up with it.

The token is now:

Code: Select all

$num = mt_rand();
$token = md5($num);
and the code to check the page login is:

Code: Select all

session_start();
if(!session_is_registered(name)){
header("location:http://www.fwbgo.net/smart/login/login.php");
exit();
}
I originally had the the redirects done with headers (line 44 of check login) but I was getting the 'headers already sent' error so I changed it to the html. I will read up on buffer output and see if I can get back to the ideal.