The perfect PHP login script

Question

I posted this same question on StackOverflow but guys over there told me to post it here.

I have programming in PHP since past 2 years and yet haven't found a perfect script for logging the user in. It's very messy when it comes to deciding whether to use Session or Cookies or both. What value to assign to the Session ID? Whether to store that value in MySQL table or not. How to auto-login the user? And so on...

I've been using this code for few months. Please tell me whether this is fine.

This is the main signin function.

<?php
    //this function is called after sanitizing the user-input
    //$u is username and $p is password
function signin($u,$p) {
    global $conn, $current_user, $signin_stat, $incorrect;
    $q_string="SELECT uid FROM users WHERE (emailid='$u' OR uname='$u') AND AES_DECRYPT(pass,CONCAT(uname,'$p'))='$p'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0]; //getting uid
        $current_user=new User($uid); //creating $current_user object

        set_session($uid); //setting session

        $signin_stat=1;

        return true;
    } else {
        signout(0);
        $incorrect=1;
        return false;
    }
}
?>

This is the set_session() function :

<?php
function set_session($uid) {
    global $conn;
    $sid=sha1($uid+time()+$_SERVER['REMOTE_ADDR']);
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);
    $_SESSION['user_session']=$sid;
    if($q) return true; else return false;
}
?>

This is the auto_signin script :

<?php
if(isset($_SESSION['user_session'])) {
    $sid=$_SESSION['user_session'];
    global $conn, $current_user;
    $q_string="SELECT uid FROM users WHERE sid='$sid'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0];
        $current_user=new User($uid);
        $signin_stat=1;
        $current_user->update_online_stat();
    } else {
        signout(0);
    }
}
?>

This is the signout() function

<?php
function signout($uid) {
    global $conn, $current_user, $signin_stat;
    $sid='NULL';
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);

    $signin_stat=0;
    $current_user=0;

    $_SESSION['user_session']='';
    unset($_SESSION['user_session']);
    unset($_POST);
    session_destroy();
}
?>

notes : $conn is the mysqli object

$current_user is the user object

sid is the field in mysql users table which holds the current session id of the respective user.

Is this script fine or would you suggest some changes?

Or is this completely crap and needs to be written again from scratch?

Please help me out.

Answer 1

I'm no security expert, not even close, but I can tell you what I use and have observed. I only use cookies to remember the username. Cookies aren't secure so you want to limit what you save in them. I also use sessions to create persistence by passing the user object on. Is this the right way? I don't know, but it has served me well and does not contradict anything I have read on the subject. As for auto-login, you can do something similar to what SO does. You can create a client side cookie with a unique ID and pair it to an account. If the login computer has that cookie then you can have it automatically login. An added security feature might be to also match IP addresses. Not sure if this is exactly what SO does, but its similar. Again no security expert, so I would gather more opinions and do your research.

If you are going to provide comments about a function then you should use doccomments as they are more useful in that IDEs can recognize them and provide them as auto-complete hints. Additionally, I would not use single letter variables except for incrementals or throwaways. The abstraction can lead to confusion if you are not careful and it makes it more difficult to tell what is going on at a glance.

/** this function is called after sanitizing the user-input
 * 
 * @param string $user is username
 * @param string $pass is password
 */

As tsabz pointed out, you should never use globals. They are evil and have no place in this world. Help banish these foul demons by forgetting you ever heard of them. Especially don't use globals for something as important as a database connection. It is insecure. If you must use a variable in a function, then inject that variable into the function via the argument list. In this case that would add a very large list, so you might instead want to compact those values into an array and inject the array instead. However, I'll point out in a few that some of these variables are unnecessary, which will decrease the size of said list.

If you are returning from an if statement, then there is no reason to have an else statement, the else is implied. I would remove it to reduce the amount of indentation, however that is really a preference point and entirely up to you.

if( $q->num_rows == 1 ) {
    //etc...
    return TRUE;
}

//etc...
return FALSE;

There are a couple of different things you can do instead of using $current_user as a global. The easiest way would be to convert it to a session variable so that it can be shared. Another, more practical way would be to return the user object instead of a boolean and then later persist it if necessary. This later method makes sense for a number of reasons. First, it goes along with what the function says it is doing. By signing in I expect either a user object or a failure, not a TRUE/FALSE. Second, it allows you to do away with $signin_stat and $incorrect which were both redundant. You could accomplish the same thing with the boolean return (to illustrate: if( signin() ) { $signin_stat = 1; }). Third, it allows you to change how you persist the user object so you are not reliant on sessions.

$_SESSION[ 'current_user' ] = new User( $uid );
//or
if( $q->num_rows == 1 ) {
    //etc...
    return new User( $uid );
}

//etc...
return FALSE;

Always, always, always use braces. I know it is a feature and you can get away with it, but it can be confusing and can cause issues, especially if you combine two statements on one line like that. Not that an if/else statement is even necessary here. If you want to return a boolean value based on the return of an expression, just return the expression. Here it is a little different because there is no comparison to make this a true expression. Instead you are relying on PHP to automatically convert it to a boolean for the comparison. So you might want to typecast it in the return to get a true boolean.

//braces
if( $q ) {
    return TRUE;
} else {
    return FALSE;
}
//equivalent to above
return ( bool ) $q;

Some of this code is violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. If you look at your login function and compare it to your auto-login function you should be able to see the similarities. You could create a new function that accomplishes the shared task, but because you are using regular functions this would make bypassing the normal signin function a security concern. I don't really see a way around this, except to convert to using a class, which is something you might want to consider anyways. Many of the issues I pointed out above would be solved much easier in a class. For instance, you could convert those globals to class properties to the same effect. Then there is also the encapsulation that will allow you to hide properties and methods, such as the $conn property and shared login method I was mentioning. I'm not trying to pressure you into using a class, but I am suggesting you give it a look. Hope this helps.