Login system

god0fgod

I've just made a new and more secure login system for my website. Although it has been tested there's probably flaws with it. What do people think?

<?
$title = "Logging in";
require_once('../includes/mysql_connection.php');
if(isset($_COOKIE['username'])){
    //user isn't logged in
    $username = mysql_escape_string($_POST['username']);
    $password = md5($_POST['password']);
    require_once('../includes/functions.php');
    $result = mysql_execute("SELECT id FROM users WHERE username = '$username' AND password = '$password' LIMIT 1", "godofgod_p_bb1");
    if(mysql_fetch_row($result)){
        //user information is correct
        if(isset($_POST['remember'])){
            //if user wants to be remembered for 100 days
            setcookie("username", $username, time()+3600*24*100, "/", ".godofgod.co.uk");
            setcookie("password", $password, time()+3600*24*100, "/", ".godofgod.co.uk");
        }else{
            setcookie("username", $username, 0, "/", ".godofgod.co.uk");
            setcookie("password", $password, 0, "/", ".godofgod.co.uk");
        }
        $direct = eregi("logout\.php",$_SERVER['HTTP_REFERER'])? "http://www.godofgod.co.uk" :  $_SERVER['HTTP_REFERER'];
        header("Location: $direct");
    }else{
        include('../template/header.php');
        echo "Bad username or password.";
        include('../template/footer.php');
    }
}else{
   include('../template/header.php');
   echo "You are already logged in.";
   include('../template/footer.php');
}
?>

Logout:

<?
$direct = eregi("(login|management|logout)\.php",$_SERVER['HTTP_REFERER'])? "http://www.godofgod.co.uk" :  $_SERVER['HTTP_REFERER'];
header("Location: $direct");
setcookie("username", 0, time() - 1, "/", ".godofgod.co.uk");
setcookie("password", 0, time() - 1, "/", ".godofgod.co.uk");
//for those who do not redirect
include('../template/header.php');
echo "You have been logged out. Click <a href='$direct'>here</a> to be taken back.";
include('../template/footer.php');
?>

Extra code included on every page for security:

if(isset($_COOKIE['username']) && isset($_COOKIE['password'])){
    $username = $_COOKIE['username'];
    $password  = $_COOKIE['password'];
    $result = mysql_execute("SELECT id FROM users WHERE username = '$username' AND password = '$password'", "godofgod_p_bb1");

if(mysql_fetch_row($result)){
    // a future change will include conversion of these variables into constants
    $user_log = 1;
    $user_name = $username;
}
}

mysql_execute() for those that are interested:

function mysql_execute($sql = 0, $database = 0){
    if($database){
        mysql_select_db($database) or die(mysql_error());
    }
    if($sql){

    $result = mysql_query($sql);
    if(!$result){
        if(USER_EMAIL){
            $dynamic_mess = " You should be emailed if the problem is fixed.";
            $from = "From: " . USER_MAIL;
        }else{
            $from = "";
        }
        $mail = mail("email", "godofgod.co.uk database error", mysql_error() . "\n\n The following query has caused a problem: \n\n$sql\n\n page: " . $_SERVER['REQUEST_URI'], $from);
        if($mail){
            $dynamic_mess = "A full error report has been emailed to the administrator." . $dynamic_mess;
        }else{
            $dynamic_mess = "There has been an error contacting the administrator about the error. Please send the full error messaage to email so the error can be fixed.";
        }
        die(mysql_error() . "<br><br> The following query has caused a problem: <br><br>$sql<br><br>" . $dynamic_mess);
    }
}
return $result;
}

bretticus

The only potential security issue I see off the top of my head is that the username and password are stored unencrypted in a cookie and they are passed each and every time from the browser. I dunno how secure this needs to be, but this brings two "problems" to bear.

If you are logging into your website on a public computer (college computer lab, etc.) other users might be able to get your credentials.
Unless every page is an SSL connection, the username and password are passed unencrypted for every page view. Cookies can be compromised via cross site scripting attacks and man in the middle attacks. Yes, sessions have the same vulnerabilities, however, a session is only good until garbage collection.

god0fgod

The password is md5 encrypted. Also, I could use a secure connection for the cookies. The only problem is that it is often very slow on the server I use. I'll try it out.

Edit: It turns out my webhost only allows SSL on a different domain and since my cookies are supposed to be stored on my domain it is a problem. A problem that could be exploited but I wont bother.

bradgrafelman

Another problem would be that this:

    $username = $_COOKIE['username'];
    $password  = $_COOKIE['password'];
    $result = mysql_execute("SELECT id FROM users WHERE username = '$username' AND password = '$password'", "godofgod_p_bb1");

would allow SQL injections to occur.

Weedpacket

godOfgod wrote:
The password is md5 encrypted.

Doesn't matter. if "lUser001" logs in with a password of "wibble" then if someone else gets the cookies that are created then they can log in as lUser001 as well - they're not going to care that they can't pronounce "50eccc6e2b0d307d5e8a40fb296f6171" as long as it gets them in.

eregi

The manual has for several years recommended using the PCRE functions instead. There are also case-insensitive string-matching functions that don't require the overhead of a regular expression.

bretticus

huh? My post that said essentially the same thing as weedpacket's is gone! Maybe, I pressed "Preview." Oh well, mine was much too wordy anyway.

My other point (besides describing a basic "recipe" for a one-time pad for non-ssl connections--oh well) was that you will need to either use the non-ssl domain or the ssl domain all the time to keep user state (cookies are designed to not cross domains.) You may know this already, just passing it along in case.

bretticus

Oh, and make sure you protect your cookie variables that will be injected into your SQL as bradgrafelman pointed out. Such as:

$username = mysql_real_escape_string($_COOKIE['username']);
    $password  = mysql_real_escape_string($_COOKIE['password']);
    $result = mysql_execute("SELECT id FROM users WHERE username = '$username' AND password = '$password'", "godofgod_p_bb1");

god0fgod

Can't believe I forgot to escape those variables. It's because I once didn't need to because my code was quite different. I've, however, put a fix to that.

I got the impression from other areas that you can't revert md5 hashes very easily. I was convinced it could be easy and secure.

I was going to add a key based encryption but that complicates things quite a lot.

Thanks for all comments.

Edit: Also using preg_match now.

laserlight

I got the impression from other areas that you can't revert md5 hashes very easily. I was convinced it could be easy and secure.

The point is that if you have the username and the password hash, then you can use those to login as the user since the password hash becomes the password. If you use a session, then the session id becomes the password (and the user name or id at the same time), but the good part is that this is a password that changes with the session. To solve this insecure credentials problem, you need to use something like SSL/TLS to create a secure channel.

As such, I suggest switching to use sessions instead of storing the username and password hash in a cookie. Without SSL/TLS you would still be vulnerable, but the vulnerability is decreased. You should continue to store your users' passwords in hashed form (with a salt) so as to protect their passwords in the event that your database is compromised.

god0fgod

Of-course. Don't know why I didn't see that.

If I wanted a session to last 100 days would I use session_cache_expire(6024100)? And for the session cookies session_set_cookie_params(360024100)?

NogDog

god0fgod wrote:
Of-course. Don't know why I didn't see that.

If I wanted a session to last 100 days would I use session_cache_expire(6024100)? And for the session cookies session_set_cookie_params(360024100)?

session_cahce_expire() has nothing to do with session lifetime. The session.gc_max_lifetime is the setting of interest for how long session data is retained on the server, in conjunction with the other session.gc_* settings. See http://www.php.net/manual/en/session.configuration.php for more info. You could set it within your scripts via the [man]ini_set/man function.

Note that if you make such alterations via functions in your scripts (as opposed to a global setting via php.ini), that any other script that does a session_start() and which uses the same sesssion data directory may have its garbage collection routine delete your session data if it has a different session.gc_max_lifetime setting, so you may also want to use [man]session_save_path/man to set a separate session data directory for those pages that other scripts will not use.