Is this code secure?

Question

<?php
session_start();

include("connect.php");

$timeout = 60 * 30;
$fingerprint = md5($_SERVER['REMOTE_ADDR'] . $_SERVER['HTTP_USER_AGENT']);

if(isset($_POST['userName']))
{
    $user = mysql_real_escape_string($_POST['userName']);
    $password = mysql_real_escape_string($_POST['password']);
    $matchingUser = mysql_query("SELECT * FROM `users` WHERE username='$user' AND password=MD5('$password') LIMIT 1");
    if (mysql_num_rows($matchingUser))
    {
     if($matchingUser['inactive'] == 1)//Checks if the inactive field of the user is set to one
     {
      $error = "Your e-mail Id has not been verified. Check your mail to verify your e-mail Id. However you'll be logged in to site with less privileges.";
      $_SESSION['inactive'] = true;
     }
     $_SESSION['user'] = $user;
     $_SESSION['lastActive'] = time();
     $_SESSION['fingerprint'] = $fingerprint;
    }
    else
    {
     $error = "Invalid user id";
    }
}
if ((isset($_SESSION['lastActive']) && $_SESSION['lastActive']<(time()-$timeout)) || (isset($_SESSION['fingerprint']) && $_SESSION['fingerprint']!=$fingerprint)
     || isset($_GET['logout'])
    )
{
    setcookie(session_name(), '', time()-3600, '/');
    session_destroy();
}
else
{
    session_regenerate_id(); 
    $_SESSION['lastActive'] = time();
    $_SESSION['fingerprint'] = $fingerprint;
}
?>

This is just modified version of http://en.wikibooks.org/wiki/PHP_Programming/User_login_systems

What does the setcookie(session_name(), '', time()-3600, '/'); do here?

Here's a bug: I use this login form:

<?php 
   if(!isset($_SESSION['user']))
    {
        if(isset($error)) echo $error;
           echo '<form action="' . $_SERVER["PHP_SELF"] . '" method="post">
        <label>Username: </label>
        <input type="text" name="userName" value="';if(isset($_POST['userName'])) echo $_POST["userName"]; echo '" /><br />
        <label>Password: </label>
        <input type="password" name="password" />
        <input type="submit" value="Login" class="button" />
        <ul class="sidemenu">
        <li><a href="register.php">Register</a></li>
        <li><a href="forgotPassword.php">Forgot Password</a></li>
    </ul>
    </form>';
    }
    else
    {
        echo '<ul class="sidemenu">
        <li>' . $_SESSION['user'] . '</li>
        <li><a href="' . $_SERVER["PHP_SELF"] . '?logout=true">Logout</a></li>
        </ul>';
    }
?>

The bug is that when I logout, the page remains the same, i.e. the login form does not show up but the same logout and user are shown. When I refresh the page, it gets normal.

Answer 1

$_SERVER['REMOTE_ADDR'] could change if the user is behind a proxy.

Answer 2

It looks fine. setcookie(session_name(), '', time()-3600, '/') essentially deletes the cookie by setting the time to before the current time.

Answer 3

What does the setcookie(session_name(), '', time()-3600, '/');

It looks like it erases the old session cookie, by setting a empty string to it and by setting a expiration time in the past. Not sure why the author used both methods, though.

Answer 4

In this case you have to make sure that you have your server settings are correct for the mysql_real_escape_string to work properly. In this case, your code is assuming that Magic Quotes are turned off whereas I often find that many servers have it enabled. You should probably be checking to see if get_magic_quotes_gpc() returns true or false to see if the server settings are automatically escaping the strings for you.

The code looks safe but I would suggest looking into a new way of doing MySQL querys: PDO. This allows for parameterized queries.

Answer 5

MD5 is a pretty weak means to encrypt the password and there are lots of ways to get around it. It does help, however that you have it setup with the IP address but IPs certainly change quite a bit.

Also, you don't have anything in there to make sure that someone doesn't hit the system over and over to break the password for a user.

G-Man

Answer 6

You could use this method described by Nate Abele to improve the reliability of the information used to build the fingerprint.

Answer 7

Use salt along with MD5 or sha1. What I use to generate the password and to check for password upon login.

function generateHash($plainText, $salt = null)
{
    define('SALT_LENGTH', 9);
    if ($salt === null)
    {
        $salt = substr(md5(uniqid(rand(), true)), 0, SALT_LENGTH);
    return array($salt, sha1($salt . $plainText) );
    }
    else
    {
        $salt = substr($salt, 0, SALT_LENGTH);
    return sha1($salt . $plainText);
    }

}

For $plainText , send in the password variable.

When you want to generate a new hash, it will return 2 values. The first value is called 'salt' and second value is the encrypted password. Store both of them into the database.

When someone attempts to login to your site, and you want to check, send their password and the salt variable to the function and it will return a hash. Then you can compare that with the value stored in the database, to check if the password entered by the user is correct.

Answer 8

When you log out, first, you are queuing the destruction of the cookie (it will occur after the response is sent), then immediately after, rendering your page. The browser has no chance to delete the cookie before rendering, and your $_SESSION variables are still alive.

PHP docs say about session_destroy:

session_destroy() destroys all of the data associated with the current session. It does not unset any of the global variables associated with the session, or unset the session cookie.

A solution is to, instead of destroying the session and the cookie, simply unset the variables which would cause authentication:

unset($_SESSION['user']);
unset($_SESSION['lastActive']);
unset($_SESSION['fingerprint']);

Just a note: I would suggest splitting your code up into functions. This would make it much more organized and readable (and reusable if you do things right).

Answer 9

Some security notes:

if($matchingUser['inactive'] == 1)

is better written as

if(!$matchingUser['inactive'])

Because if the database schema changes (e.g. it's now an integer to indicate a certain type of activity (which is bad design, in my opinion: an enumeration would do better)) your code would have issues.

Of course, this is a double negative, which may be less readable. Better would be:

if($matchingUser['isactive'])

Or even:

if($matchingUser->isActive())

assuming you create a User class, etc. etc.

Just in case, use or require or require_once where necessary (preferably the latter if connect.php contains function declarations).

Store the user's ID in the session variable instead of the username. There is a possibility you will allow a user to change his name later on, and the session data would be invalid (at least ['user'] would, anyway). It's also faster to find a database record by ID (primary key, unique) than it is to by username (maybe indexed, string).

Kicking me after 30 minutes is really annoying. You're not the only site I go to, and I may revisit later after doing some work (if I get called in to do something, for example, or take a lunch break).

Use htmlspecialchars to help prevent XSS.

No need to use $_SERVER['PHP_SELF'] here:

<a href="' . $_SERVER["PHP_SELF"] . '?logout=true">

Simply write without it:

<a href="?logout=true">

When the user POSTs something, make sure to redirect them (TODO: note how). Otherwise, the user's back button may cause a re-POST of the data (which probably isn't what you want!).

Answer 10

$matchingUser['inactive'] will never be set, as you don't get the actual data from your db with mysql_fetch_assoc().

Modified version:

$matchingUser = mysql_query("SELECT * FROM `users` WHERE username='$user' AND password=MD5('$password') LIMIT 1");
if (mysql_num_rows($matchingUser))
{
    $matchingUserData = mysql_fetch_assoc($matchingUser);
    if($matchingUserData['inactive'] == 1) //Checks if the inactive field of the user is set to one
    {
        $error = "Your e-mail Id has not been verified. Check your mail to verify your e-mail Id. However you'll be logged in to site with less privileges.";
        $_SESSION['inactive'] = true;
    }