4
\$\begingroup\$

I've implemented a "remember me" automatic login if no session has been set, but a remember me cookie is on their computer. I'm concerned about security and I'm not the best. I'm relatively new to OOP (just started a learning a week ago).

$database = new db;

if (isset($_SESSION['info'])){
  $token = $_SESSION['info']['token'];
  $loggedIn = 1;
} else {
  $loggedIn = 0;
  if (isset($_COOKIE["rememberMe"])){
    $rememberId = $_COOKIE["rememberMe"][0];
    $rememberKey = $_COOKIE["rememberMe"][1];
    if ($database->rememberMe($rememberId,$rememberKey) === "verified") {
      $loggedIn = 1;
    }
  }
}

$loggedIn is for use in JavaScript.

rememberMe function in the db class:

  public function rememberMe ($userId,$rememberKey) {

    $conn = $this->connect();
    $stmt = $conn->prepare(
    "SELECT COUNT(*) as count 
     FROM sessions 
     WHERE remember_key = :remember_key AND user_id = :user_id"
    );
    $stmt->bindParam(":remember_key",$rememberKey);
    $stmt->bindParam(":user_id",$userId);
    $stmt->execute();
    $remember = $stmt->fetchAll();
    $rememberVerify = $remember[0]["count"];

    if ($rememberVerify > 0) {

      $stmt = $conn->prepare(
      "SELECT id,username,display_name,avatar FROM users WHERE id = :user_id"
      );
      $stmt->bindParam(":user_id",$userId,PDO::PARAM_INT);
      $stmt->execute();
      $userInfo = $stmt->fetchAll();

      $dbId = $userInfo[0]["id"];
      $dbUsername = $userInfo[0]["username"];
      $dbDisplayName = $userInfo[0]["display_name"];
      $dbAvatar = $userInfo[0]["avatar"];
      $token = hash('sha256',openssl_random_pseudo_bytes(50));

      $_SESSION['info'] = [
        'id'=>$dbId,
        'username'=>$dbUsername,
        'display_name'=>$dbDisplayName,
        'avatar'=>$dbAvatar,
        'token'=>$token
      ];
      return "verified";
    }
  }

When the user signs in with "remember me" checked:

  if ($_POST['remember'] === "true"){
    $rememberKey = hash('sha256',openssl_random_pseudo_bytes(50));
    $stmt = $db->prepare("INSERT INTO sessions (user_id,remember_key) VALUES (:user_id,:remember_key)");
    $stmt->bindParam(":user_id",$dbId);
    $stmt->bindParam(":remember_key",$rememberKey);
    $stmt->execute();
    setcookie ('rememberMe[0]', $dbId, time() + (86400 * 365), "/");
    setcookie ('rememberMe[1]', $rememberKey, time() + (86400 * 365), "/");
  }
\$\endgroup\$
2
\$\begingroup\$

Security

I'm concerned about security and I'm not the best.

You use prepared statements, secure random numbers, and didn't make any other giant mistakes (from what I can tell), so that is already very good.

Still, there are two issues:

  • you definitely want to set these cookies as httpOnly (it may somewhat mitigate XSS attacks).
  • you definitely want to hash the token in the database (use proper hashing such as bcrypt). Otherwise, once your database is compromised, an attacker can login as anyone that has an active remember me token. [your sha256 doesn't count here, an attacker can still just pass the value in the db]

And two minor points:

  • do you need the token in the session? It doesn't seem so to me, so I wouldn't store it. It shouldn't happen, but session data may leak, eg on shared hosts, so if it's not necessary, I wouldn't store secret information in them.
  • the feasibility of remote timing attacks is still somewhat disputed, but it's not that far fetched. With your code, it's highly unlikely that anything may happen, but still, ideally, your code should be secure against it, and I'm pretty sure that your SELECT isn't. If you use bcrypt as suggested, you get a timing safe string compare for free. Otherwise, see eg here (it also links to some more information on proper remember me mechanisms).

Misc

  • rememberMe isn't named all that good. It's not a function used to remember me, but a function used to check if I am remembered. So isRemembered would be more fitting. Something like authenticateCookie or similar might also work.
  • two spaces indentation is not enough, it makes your code hard to read. Use at least 4 spaces. If that leads to too long lines, reduce your nesting.
  • you should share one connection across your code (for performance reasons).
  • your rememberMe function may do a bit too much. It verifies the validity of a remember me token, but it also loads user info and saves it in the session. I would probably separate the second task into its own function.
\$\endgroup\$
  • \$\begingroup\$ Thanks! Really helpful. Also $token is a CSRF key unrelated to the remember me thing. \$\endgroup\$ – user3040456 Mar 7 '16 at 11:59
  • \$\begingroup\$ @user3040456 oh, yeah, that makes sense. I would probably rename it to something with csrf in its name, and move that code somewhere else as well (because it doesn't have anything to do with remember me functionality). \$\endgroup\$ – tim Mar 7 '16 at 12:09
1
\$\begingroup\$

It seems to me your rememberMe function is doing too much, plus the naming of the function does not say much about what's going on inside.

I recently came across a nice library for this functionality on PHP, which you can find on GitHub. It could surely give you good ideas on how to implement it and divide the code into classes.

Just one advice in case you prefer to continue on your own: separating DB logic from that function, extracting it on to a separate class that handles all kind of DB exceptions and queries. And yes, exception-handling is another point that you may consider important.

\$\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.