4
\$\begingroup\$

I have read a few SO threads to implement remember_me functionality.

Steps I have followed:

User clicks on login button and if passes client side validation, it goes to php Login page.

First time

 function storeCookie($row,$connection)
 {
        $user_id = $row['UserID'];
        $token = uniqid($user_id,TRUE);
        $salt = md5(mt_rand());
        $cookie_id = hash_hmac('SHA512',$token,$salt);
        $expiry = strval(time() + (7 * 24 * 60 * 60));
        $cookie_name = "GETIN";
        $sql_store_cookie = "INSERT INTO pcookies(cookie_id,user_id,expiry,salt,cookie_name) values('$cookie_id','$user_id','$expiry','$salt','$cookie_name')";
        mysqli_query($connection,$sql_store_cookie) or die(mysqli_error($connection));//die('{"l":"mysqli_error(`$connection`)"}');
        setcookie("GETIN",$token,$expiry);
}
  1. It reads user id from database[already user registered].
  2. Creates a unique id and salt, hashed value. Sets expiration date for 7 days and store it in the database

cookie is [cookie_name=token]

All the times but not the first

  1. Checks for cookie_name in the browser and if it is present reads the corresponding value
  2. Using that name, gets the salt from database
  3. Calculates has and checks with db
  4. If success, go to the home page.
  if(isset($_COOKIE['GETIN']))
  {
    $cookie_name = 'GETIN';
    $salt = getSalt($cookie_name,$connection);
    $cookie_id = getCookieID($cookie_name,$connection);
    echo "      cookieid=".$cookie_id ;
    echo "      salt=".$salt;
    if($cookie_id == (hash_hmac('SHA512',$_COOKIE['GETIN'],$salt)))
    {
        die('{"status":"success"}');
    }
  }

It works fine. My doubts are

  1. Is this method ok or need to modify something or everything. If so, please guide me
  2. I didn't update the salt and hash once user successfully logged in. Do I need to update?
  3. I am doing everything with PHP.

My site doesn't conatain any sensitive data like payments. It's just like tutorial site. Please share your few experiences with this functionality.

 function getSalt($cookie_name,$connection)
 {
    $sql_token_query = "select salt from pcookies where cookie_name = '$cookie_name'";
    $sql_token = mysqli_fetch_array(mysqli_query($connection,$sql_token_query));
    echo "   getSalt=".$sql_token['salt'];  
    return $sql_token['salt'];
  }

  function getCookieID($cookie_name,$connection)
  {
    $sql_cookieid_query = "select cookie_id from pcookies where cookie_name = '$cookie_name'";
    $sql_cookieid = mysqli_fetch_array(mysqli_query($connection,$sql_cookieid_query));  
    echo "   getCookie=".$sql_cookieid['cookie_id'];
    return $sql_cookieid['cookie_id'];
   } 
\$\endgroup\$
  • \$\begingroup\$ can you post the getSalt and getCookieID functions? They perform SQL queries, right? It seems that this could be reduced to one query (which would be nice, considering this happens on every request). \$\endgroup\$ – tim Apr 26 '15 at 15:04
  • \$\begingroup\$ @tim Yes sure. Edited \$\endgroup\$ – Gibbs Apr 26 '15 at 15:08
3
\$\begingroup\$

Performance

As your code is executed on every request, it's best to optimize it. First of all, you perform two queries when one would be enough. You could join your getSalt and getCookieID method to a getSaltAndCookie method which returns both values in an array.

It's not all that clear, but how I understand it, you do not use a session for login check at all. It would be better to add code that handles the normal session checking, and only use the cookie based authentication if no session is set, and then set a session (so All the times but not the first would become Only the first time, and you would not have any queries on subsequent requests).

Security

If UserID is user controller, you have a second order SQL injection in storeCookie. Either way, I would use prepared statements for all variable content just to make sure.

Even if there isn't an injection at the time, I could imagine that you will extend your code in the future. For example, currently, there can only ever be one login cookie, so I'm assuming you only allow one user. But this might change in the future, in which case $cookie_name will not be the static string GETIN, but some kind of identifier send via cookie. These changes would not be made in getSalt and getCookieID, so it would be very easy to forget to secure the queries.

The documentation of uniqid says: Warning: This function does not create random nor unpredictable strings. This function must not be used for security purposes, this is because it's based on the time it is created. It's not so bad because your salt is actually random, but still. If you are afraid of collisions for the token, you could generate a truly random string and prefix it with the username.

Misc

  • If you store this: 7 * 24 * 60 * 60 in a named variable, it would be immediately obvious that it is 7 days.
  • use more spaces, eg around . to increase readability.
  • all your SQL keywords should be all uppercase to increase readability (eg values -> VALUES, and all the keywords in the insert queries).
\$\endgroup\$
  • \$\begingroup\$ Thanks Plus one from my side. I don't undestand the thing you explained about second level SQL injection when more users are logged on . Please clear it. I will implement session based login too and i ll find a better way to implement unique id. Thanks for the MISC. \$\endgroup\$ – Gibbs Apr 26 '15 at 15:31
  • 1
    \$\begingroup\$ @user3168736 second order inj and more users where two different points. 1) If we assume UserID is user controlled, then the attacker might create a user that contains special characters, such as '. When inserting the user in the database, escaping or prepared statements will make this secure. But you still have the user with ' in the database. I'm assuming $row['UserID'] is the result of a query. Well, now that contains a ', and is directly inserted into a query, without escaping or prepared statements, and thus vulnerable. 2) You always use the same entry in pcookies \$\endgroup\$ – tim Apr 26 '15 at 15:41
  • \$\begingroup\$ the one named GETIN. That means that you can only have one remember-me cookie at a time, which isn't good when you have multiple users. This is why I think that you might change it in the future, so that GETIN is variable, eg by using $_COOKIE['GETIN'] instead. To do this, you would change the code in storeCookie and in your second code block. But getSalt and getCookieID do not have to change for this to work, so you would probably not change them. BUT: now $cookie_name is user controlled and directly inserted into a query, thus resulting in an SQL injection. \$\endgroup\$ – tim Apr 26 '15 at 15:41
  • \$\begingroup\$ Thanks tim. I have to read more to implement these functionalities in a better way. Thanks for your time \$\endgroup\$ – Gibbs Apr 26 '15 at 15:46
  • 1
    \$\begingroup\$ @user3168736 yes, exactly. So you would check if a session is set and valid, and if it is not, you would check the cookie, and if that is valid set the session. \$\endgroup\$ – tim Jun 1 '15 at 17:25
2
\$\begingroup\$

There is one thing I would like to add to tim's answer regarding security. I assume the cookie should only ever be read by the server side. Therefore I would use the last parameter in the native PHP setcookie() function called httpOnly. If you set this to true you indicate that the browser must never make the cookie and its value available to client-side scripts such as javascript.

$name   = 'GETIN';
$token  = 'top secret'; // The result of your token generating logic.
$path   = '/';
$domain = null;
$secure = false; // Set to true if using SSL (https). More on that later.
$http   = true;

setcookie($name, $token, $expire, $path, $domain, $secure, $http);

How an actual malicious user would exploit the cookie is beyond my knowledge, but I believe that you should always be a healthy bit of paranoid when dealing with security and seek to implement security in depth (multiple layers) rather than a single layer.

Declaring the cookie httpOnly would certainly make it harder for any potential exploits to be successful.

Bonus

In order to further increase your security you should have your entire site running over SSL (https). This may be overkill for a local development or learning site. I mention it so you are aware that this is always a good practice to use SSL when dealing with sensitive/login-required sites. So I would strongly recommend developing your application to that a change to SSL in the future is as painless as possible.

If your site one day runs over SSL you should consider changing the configurations for the remember-me cookie. The $secure parameter from the script above indicates that the cookie can be sent over insecure connections (its value is false). Setting this to true would make so that a potential man-in-the-middle attacker would have a very difficult time (maybe even impossible) reading the value of the cookie and generating his own remember-me cookie and through that bypass your login routine, while impersonating another user (ouch!).

Happy coding!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.