HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

"Remember me" functionality for a website

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rememberwebsitefunctionalityfor

Problem

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);
}


  • It reads user id from database[already user registered].



  • 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

  • Checks for cookie_name in the browser and if it is present reads the corresponding value



  • Using that name, gets the salt from database



  • Calculates has and checks with db



  • 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

  • Is this method ok or need to modify something or everything. If so, please guide me



  • I didn't update the salt and hash once user successfully logged in. Do I need to update?



  • 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

Solution

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).

Context

StackExchange Code Review Q#88034, answer score: 3

Revisions (0)

No revisions yet.