Skip to content
Permalink
Browse files Browse the repository at this point in the history
[Security] Add a separator in the remember me cookie hash
  • Loading branch information
pborreli authored and michaelcullum committed Apr 6, 2019
1 parent 3e0b235 commit a29ce28
Showing 1 changed file with 1 addition and 1 deletion.
Expand Up @@ -120,6 +120,6 @@ protected function generateCookieValue($class, $username, $expires, $password)
*/
protected function generateCookieHash($class, $username, $expires, $password)
{
return hash_hmac('sha256', $class.$username.$expires.$password, $this->getSecret());
return hash_hmac('sha256', $class.self::COOKIE_DELIMITER.$username.self::COOKIE_DELIMITER.$expires.self::COOKIE_DELIMITER.$password, $this->getSecret());
}
}

6 comments on commit a29ce28

@simoheinonen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logs out all users with the old hash. 😐

@stof
Copy link
Member

@stof stof commented on a29ce28 Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simoheinonen which is better than allowing to spoof remember me cookies

@simoheinonen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but worth mentioning imo. Logging out thousands of users might cost a lot

@stefanospetrakis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One remark regarding this (a bit too late perhaps);
I would like this a little bit shorter for readability/redundancy/etc., sth like that:
return hash_hmac('sha256', implode(self::COOKIE_DELIMITER, func_get_args()), $this->getSecret());

Any point to opening a follow-up issue for this?

@stof
Copy link
Member

@stof stof commented on a29ce28 Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanospetrakis this code is less explicit about what gets included in the hash exactly, due to using func_get_args instead of the actual arguments. So to me, this actually makes it less readable.

@stefanospetrakis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Fair enough, how about the following:

implode(self::COOKIE_DELIMITER, [$class, $username, $expires, $password])

Please sign in to comment.