Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

The code below is a attempt to create a persistent login cookie. I am an amateur and not professional so this is the first attempt to have secure remember me cookie. Now I want you guys to review two things:

  1. I am trying to create a system like best way to implement remember me but I am not sure if I have done the same thing as explained there ( username row in table and cookie value isn't there but i have added).
  2. Is my code a good way to secure remember me cookie ?

Any other suggestions or critiques are welcome.

    if (isset($_POST['rememberme'])) {

    $hash = sometext;
    $identifier = md5($username);
    $token = hash('sha512',$hash);
    setcookie('auth', $username. "," .$identifier. "," . $token, time()+31556926);
    $query=$db->prepare("SELECT username from tokens WHERE username=:username");
    $query->execute(array(':username'=>$username));
    if($query->rowCount() <=0 ){
    $result = $db->prepare("INSERT INTO tokens (username,identifier,token) VALUES (:username,:identifier,:token)");
    $result->execute(array(':username'=>$username,':identifier'=>$identifier,':token'=>$token));
     }
    }

    if(isset($_COOKIE["auth"])){
    $pieces = explode(",", $_COOKIE["username"]);
    $username = $pieces[0];
    $Uidentifier = $pieces[1];
    $Utoken = $pieces[2];

    $sql=$db->prepare("SELECT * FROM tokens WHERE identifier=:identifier");
    $sql->execute(array(':identifier'=>$Uidentifier));
    if($sql->rowCount()>0){
     $row = $sql->fetch(PDO::FETCH_ASSOC);
     $dbtoken = $row['token'];
     if($Utoken==$dbtoken){
     $newhash = 'sometext';
     $newtoken = hash('sha512',$newhash);
     $que=$db->prepare("UPDATE tokens SET token=:token WHERE identifier=:identifier");
     $que->execute(array(':token'=>$newtoken,':identifier'=>$Uidentifier));
     setcookie('auth', $username. "," .$identifier. "," . $token, time()+31556926);
     header("Location:home.php");
     }
     if($Utoken!=$dbtoken){
     echo "Unauthorized login attempt!";
     setcookie("auth","",time()-31556926);
     $stmt->$db=("DELETE FROM tokens WHERE identifier=:identifier");
     $stmt->execute(array(':identifier'=>$identifier));
     header("Location:login.php");
      }
     }
    } 
share|improve this question

Security

I am not sure if I have done the same thing as explained there

Is my code a good way to secure remember me cookie ?

No and no.

Non-Random Token: Login as anyone

The only data you store in your cookie is the username*. But usernames are generally not considered secret, and are relatively easy to obtain.

Because of this, an attacker can just calculate cookie=$username. "," .md5($username). "," . hash('sha512','sometext') themselves and log in as anyone.

You definitely always want at least partly random data in your cookie.

* and sometext. It's not entirely clear what this means, but because of the line $newhash = 'sometext';, I'm assuming that it is some hardcoded value, which is not good enough. Having (correctly generated) random data is essential here.

Non-Random Identifier: Invalidating Remember-Me Cookie of any user

Your identifier only consists of the (hashed) username.

An attacker can invalidate the remember-me cookie of any user by simply submitting a remember-me cookie like this: username,md5(username),invalid_token.

This is why the post suggests that the identifier should be random (otherwise, it wouldn't be needed, as the username could be used instead; the hashing itself doesn't add any security).

Plaintext Cookies: Login as anyone after database compromise

You also do not want to store the cookie as plaintext in the database, because it means that if an attacker gained access to your database, they can log in as anyone.

The hashing you do apply (md5 and sha512) do not solve this problem, as they are used to create the cookie, but not to store the cookie, or to compare the stored cookie to a user supplied cookie.

You should use bcrypt to store the cookie (the token part) in the database (just as you would when storing passwords).

Identifier and Token: Preventing timing attacks

The reason that the linked answer suggests using an identifier and a token is to prevent timing attacks which may result from a db lookup such as "SELECT * FROM tokens WHERE cookie=:user_supplied_cookie".

The problem here is that you actually use the identifier correctly, but then use ==, re-introducing the timing attack. If you use bcrypt as suggested, you will get a timing-safe string-compare for free.

httpOnly

Cookies such as this should be httpOnly to somewhat mitigate the risk of XSS attacks.

For more information on remember-me, this and this may help.

Misc

  • Your formatting is rather non-standard and partly inconsistent, making your code hard to read: Your indentation is inconsistent, and one space for indentation is definitely not enough. Your spacing is also inconsistent, and you could use more spaces (especially around comparisons such as !=, >, etc).
  • Don't shorten variable names, it makes your code hard to understand. What's a Utoken or a Uidentifier?
  • be consistent in the structure of your variable names. Some of your variables start with an upper-case character, some with a lower-case one, and all your variables are all lower-case afterwards, making them harder to read.
  • die after a header redirect, as a client does not have to actually follow the redirect.
share|improve this answer
    
thanks going to use bin2hex(openssl_random_pseudo_bytes($bits)); for random number Is this safe? and sorry didn't catch ur point **you actually use the identifier correctly, but then use ==, re-introducing the timing attack.` could you explain that ** how its introducing timing attack – bɪˈɡɪnə Feb 22 at 10:52
1  
@bɪˈɡɪnə openssl_random_pseudo_bytes is safe, yes. And == isn't timing safe. This isn't that big of a problem, as you are invalidating the cookie after one invalid try, but as defense in depth, it still makes sense to use a safe function here (eg hash_equals, or just bcrpyt, which is timing-safe) (if you don't, you could just as well get rid of the identifier and use the username instead). – tim Feb 22 at 11:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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