Log in code... Do you see gaps

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
User avatar
micknc
Forum Contributor
Posts: 115
Joined: Thu Jan 24, 2008 11:13 pm

Log in code... Do you see gaps

Post 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!
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

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

Post 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.
User avatar
micknc
Forum Contributor
Posts: 115
Joined: Thu Jan 24, 2008 11:13 pm

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

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

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

Post 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.
User avatar
micknc
Forum Contributor
Posts: 115
Joined: Thu Jan 24, 2008 11:13 pm

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

Post 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");
}
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

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

Post 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.
User avatar
micknc
Forum Contributor
Posts: 115
Joined: Thu Jan 24, 2008 11:13 pm

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

Post 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.
Post Reply