Is this code safe enough for PHP session with "remember me"?

Question

I have read ALOT of information about session security, and have come up with this little piece of code.

Would appreciate if you guys took a look at it and told me if I need to change something to make it better and safer.

function cookie_auth(){
    if(isset($_COOKIE['cookie_name'])){
        $data = $_COOKIE['cookie_name'];
        list(,$username) = explode(':', $data);
        $sql = "SELECT * FROM tbl WHERE tbl.usrname= '$username'";
        $res = mysql_query($sql) or die(mysql_error());
        $row = mysql_fetch_array($res);
        $num_rows = mysql_num_rows($res);

        if ($num_rows==1){
            // AUTHENTICATE COOKIE VALUES
            $salt1 = sha1($row['alt_username']);
            $text = "constant_text_here";
            $salt2 = sha1($text);

            if($data == $salt1.':'.$username.':'.sha1($row['alt_username'].$salt2)){
                // USER IS AUTHENTICATED AND CORRECT
                $_SESSION['logged_in'] = true;
            }
        }

        else if ($num_rows!=1){
            // REDIRECT TO LOGIN PAGE
        }
    }//end if isset cookie

    // ELSE IF COOKIE ISN'T SET //
    else {
        // REDIRECT TO LOGIN PAGE
    }
}// end function cookie_auth //



// AUTHENTICATE USER //
if(!isset($_SESSION['logged_in']) || $_SESSION['logged_in']!==true){
        cookie_auth();
}

else if ($_SESSION['logged_in']===true){
    // FURTHER AUTHENTICATION //
    if($_SESSION['HTTP_USER_AGENT'] != sha1($_SERVER['HTTP_USER_AGENT']){
        header('Location: http://www.domain.com/login');
        session_destroy();
        die();
    }
}

What do you think so far?

Also something I have thought about:

What code should I use if user isn't authenticated? Should I use "session_destroy" and then "die()"? Should I use "unset session"?

Thanks

So wait, if someone uses the user name they can just login? I don't think you understand how $_SESSION works, you should **not** be assigning a username to $_COOKIE. All of this code should be simplified to `$_SESSION['user_name']=$row['username']` and then check if they are logged in by `if($_SESSION['user_name']){...}` — rook, Nov 06 '10 at 18:36
Rook: You dont understand my question, I DO authenticate users the way you suggest already, it is the "remember me" which I use the cookie for! — , Nov 07 '10 at 22:24

score 1 · Accepted Answer · answered Nov 06 '10 at 14:46

1

$data = $_COOKIE['cookie_name'];

Watch out: $_COOKIE['cookie_name'] can be an array. Use filter_input whenever possible: $data = filter_input(INPUT_COOKIE, 'cookie_name');.

list(,$username) = explode(':', $data);

$data does not have to contain a :, so check whether $username exist or not.

$sql = "SELECT * FROM tbl WHERE tbl.usrname= '$username'";

High risk: SQL injection vulnerability, you should always escape untrusted data. Safer code:

$sql = sprintf("SELECT * FROM tbl WHERE tbl.usrname='%s'", mysql_real_escape_string($data));

If an the login state changes (e.g. the user logs in), it's a good idea to regenerate the session id before continuing. This will prevent session fixation.

answered Nov 06 '10 at 14:46

Lekensteyn

64,486
22
159
192

about regenerating the session id, could you give a piece of code on that. I have found a bunch of different codes on that, where they set the new_session = old_session etc, and I don't know why they do that. – Nov 06 '10 at 14:51
1

There is a function for that. You shouldn't try making an own one unless you really know what you're doing. The function that should be used is [`session_regenerate_id()`](http://nl.php.net/session_regenerate_id). – Lekensteyn Nov 06 '10 at 15:22
the best way to stop session fixation (http://www.owasp.org/index.php/Session_Fixation) is to not accept a session id via GET or POST. – rook Nov 06 '10 at 18:40
Could you show me how to filter the input of the cookie, or atleast explain some more about that? What do you mean by "can be an array"? – Nov 07 '10 at 22:28
A `$_COOKIE` array works like a `$_GET` and `$_POST` array. The values of these arrays can either be a string or an array. To make it an array, name the key like `var[]`. (test it, by dumping `$_GET` with `print_r($_GET);`, and opening that page with `thatfile.php?var[]=val&varWith[key]=val`. You'll see an array instead of a string. Remeber that `$_COOKIE` is similar to `$_GET`) `filter_input` accepts strings only and has additional flags to limit the possible list of characters. Please consult the PHP manual pages http://php.net/filter_input and http://php.net/filter.filters – Lekensteyn Nov 08 '10 at 15:57

score 0 · Answer 2 · edited May 23 '17 at 12:11

Your are very vulnerable to SQL injections, I highly suggest you read some more about that subject and maybe switch to PDO if your web host supports this. Best way to stop SQL injections in PHP

There is also a problem with the cookie you're generating. It contains values which will not change, unless the user changes his password. This means in a case a malicious user obtains someones cookie they will be able to use it indefinitely. A way to avoid a malicious user from using the same cookie could be by adding the IP address in the cookie generation. Probably a better way to generate a cookie is to use the following code:

//I assume $row['alt_username'] is a secret key thing?
//It's best to change this value every login
$secretkey=$row['alt_username'];
//Best if your system supports the hash function with sha256
$hashed=hash("sha256", $secretkey."your_salt_here".$_SERVER['REMOTE_ADDR']);
//or else
$string=sha1($secretkey."your_salt_here".$_SERVER['REMOTE_ADDR']);

Also you don't want to double hash things, like your salt and username because this will only limit the possibilities of the hash and the username values (although this is probably purely a theoretical problem, since it's not common use to hash your salt).

Is this code safe enough for PHP session with "remember me"?

2 Answers2