Skip to content

Commit 4f5f6c6

Browse files
nicolas-grekasnicolas-grekas
committed
[Security/Http] Check tokens before loading users from providers
1 parent 236aa94 commit 4f5f6c6

File tree

8 files changed

+85
-98
lines changed

8 files changed

+85
-98
lines changed

‎LoginLink/LoginLinkHandler.php‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,16 @@ public function consumeLoginLink(Request $request): UserInterface
8484
{
8585
$userIdentifier = $request->get('user');
8686

87+
if (!$hash = $request->get('hash')) {
88+
throw new InvalidLoginLinkException('Missing "hash" parameter.');
89+
}
90+
if (!$expires = $request->get('expires')) {
91+
throw new InvalidLoginLinkException('Missing "expires" parameter.');
92+
}
93+
8794
try {
95+
$this->signatureHasher->acceptSignatureHash($userIdentifier, $expires, $hash);
96+
8897
// @deprecated since Symfony 5.3, change to $this->userProvider->loadUserByIdentifier() in 6.0
8998
if (method_exists($this->userProvider, 'loadUserByIdentifier')) {
9099
$user = $this->userProvider->loadUserByIdentifier($userIdentifier);
@@ -93,19 +102,10 @@ public function consumeLoginLink(Request $request): UserInterface
93102

94103
$user = $this->userProvider->loadUserByUsername($userIdentifier);
95104
}
96-
} catch (UserNotFoundException $exception) {
97-
throw new InvalidLoginLinkException('User not found.', 0, $exception);
98-
}
99105

100-
if (!$hash = $request->get('hash')) {
101-
throw new InvalidLoginLinkException('Missing "hash" parameter.');
102-
}
103-
if (!$expires = $request->get('expires')) {
104-
throw new InvalidLoginLinkException('Missing "expires" parameter.');
105-
}
106-
107-
try {
108106
$this->signatureHasher->verifySignatureHash($user, $expires, $hash);
107+
} catch (UserNotFoundException $e) {
108+
throw new InvalidLoginLinkException('User not found.', 0, $e);
109109
} catch (ExpiredSignatureException $e) {
110110
throw new ExpiredLoginLinkException(ucfirst(str_ireplace('signature', 'login link', $e->getMessage())), 0, $e);
111111
} catch (InvalidSignatureException $e) {

‎RememberMe/PersistentRememberMeHandler.php‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -53,18 +53,16 @@ public function __construct(TokenProviderInterface $tokenProvider, string $secre
5353
*/
5454
public function createRememberMeCookie(UserInterface $user): void
5555
{
56-
$series = base64_encode(random_bytes(64));
57-
$tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
56+
$series = random_bytes(66);
57+
$tokenValue = strtr(base64_encode(substr($series, 33)), '+/=', '-_~');
58+
$series = strtr(base64_encode(substr($series, 0, 33)), '+/=', '-_~');
5859
$token = new PersistentToken(\get_class($user), method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername(), $series, $tokenValue, new \DateTime());
5960

6061
$this->tokenProvider->createNewToken($token);
6162
$this->createCookie(RememberMeDetails::fromPersistentToken($token, time() + $this->options['lifetime']));
6263
}
6364

64-
/**
65-
* {@inheritdoc}
66-
*/
67-
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
65+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
6866
{
6967
if (!str_contains($rememberMeDetails->getValue(), ':')) {
7068
throw new AuthenticationException('The cookie is incorrectly formatted.');
@@ -86,18 +84,28 @@ public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInte
8684
throw new AuthenticationException('The cookie has expired.');
8785
}
8886

87+
return parent::consumeRememberMeCookie($rememberMeDetails->withValue($persistentToken->getLastUsed()->getTimestamp().':'.$rememberMeDetails->getValue().':'.$persistentToken->getClass()));
88+
}
89+
90+
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
91+
{
92+
[$lastUsed, $series, $tokenValue, $class] = explode(':', $rememberMeDetails->getValue(), 4);
93+
$persistentToken = new PersistentToken($class, $rememberMeDetails->getUserIdentifier(), $series, $tokenValue, new \DateTime('@'.$lastUsed));
94+
8995
// if a token was regenerated less than a minute ago, there is no need to regenerate it
9096
// if multiple concurrent requests reauthenticate a user we do not want to update the token several times
91-
if ($persistentToken->getLastUsed()->getTimestamp() + 60 < time()) {
92-
$tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
93-
$tokenLastUsed = new \DateTime();
94-
if ($this->tokenVerifier) {
95-
$this->tokenVerifier->updateExistingToken($persistentToken, $tokenValue, $tokenLastUsed);
96-
}
97-
$this->tokenProvider->updateToken($series, $tokenValue, $tokenLastUsed);
98-
99-
$this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue));
97+
if ($persistentToken->getLastUsed()->getTimestamp() + 60 >= time()) {
98+
return;
99+
}
100+
101+
$tokenValue = strtr(base64_encode(random_bytes(33)), '+/=', '-_~');
102+
$tokenLastUsed = new \DateTime();
103+
if ($this->tokenVerifier) {
104+
$this->tokenVerifier->updateExistingToken($persistentToken, $tokenValue, $tokenLastUsed);
100105
}
106+
$this->tokenProvider->updateToken($series, $tokenValue, $tokenLastUsed);
107+
108+
$this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue));
101109
}
102110

103111
/**
@@ -113,7 +121,7 @@ public function clearRememberMeCookie(): void
113121
}
114122

115123
$rememberMeDetails = RememberMeDetails::fromRawCookie($cookie);
116-
[$series, ] = explode(':', $rememberMeDetails->getValue());
124+
[$series] = explode(':', $rememberMeDetails->getValue());
117125
$this->tokenProvider->deleteTokenBySeries($series);
118126
}
119127

@@ -124,9 +132,4 @@ public function getTokenProvider(): TokenProviderInterface
124132
{
125133
return $this->tokenProvider;
126134
}
127-
128-
private function generateHash(string $tokenValue): string
129-
{
130-
return hash_hmac('sha256', $tokenValue, $this->secret);
131-
}
132135
}

‎RememberMe/RememberMeDetails.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,14 @@ public function __construct(string $userFqcn, string $userIdentifier, int $expir
3636

3737
public static function fromRawCookie(string $rawCookie): self
3838
{
39-
$cookieParts = explode(self::COOKIE_DELIMITER, base64_decode($rawCookie), 4);
39+
$cookieParts = explode(self::COOKIE_DELIMITER, $rawCookie, 4);
4040
if (4 !== \count($cookieParts)) {
4141
throw new AuthenticationException('The cookie contains invalid data.');
4242
}
43-
if (false === $cookieParts[1] = base64_decode($cookieParts[1], true)) {
43+
if (false === $cookieParts[1] = base64_decode(strtr($cookieParts[1], '-_~', '+/='), true)) {
4444
throw new AuthenticationException('The user identifier contains a character from outside the base64 alphabet.');
4545
}
46+
$cookieParts[0] = strtr($cookieParts[0], '.', '\\');
4647

4748
return new static(...$cookieParts);
4849
}
@@ -83,6 +84,6 @@ public function getValue(): string
8384
public function toString(): string
8485
{
8586
// $userIdentifier is encoded because it might contain COOKIE_DELIMITER, we assume other values don't
86-
return base64_encode(implode(self::COOKIE_DELIMITER, [$this->userFqcn, base64_encode($this->userIdentifier), $this->expires, $this->value]));
87+
return implode(self::COOKIE_DELIMITER, [strtr($this->userFqcn, '\\', '.'), strtr(base64_encode($this->userIdentifier), '+/=', '-_~'), $this->expires, $this->value]);
8788
}
8889
}

‎RememberMe/SignatureRememberMeHandler.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,19 @@ public function createRememberMeCookie(UserInterface $user): void
5353
$this->createCookie($details);
5454
}
5555

56-
/**
57-
* {@inheritdoc}
58-
*/
56+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
57+
{
58+
try {
59+
$this->signatureHasher->acceptSignatureHash($rememberMeDetails->getUserIdentifier(), $rememberMeDetails->getExpires(), $rememberMeDetails->getValue());
60+
} catch (InvalidSignatureException $e) {
61+
throw new AuthenticationException('The cookie\'s hash is invalid.', 0, $e);
62+
} catch (ExpiredSignatureException $e) {
63+
throw new AuthenticationException('The cookie has expired.', 0, $e);
64+
}
65+
66+
return parent::consumeRememberMeCookie($rememberMeDetails);
67+
}
68+
5969
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
6070
{
6171
try {

‎Tests/LoginLink/LoginLinkHandlerTest.php‎

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ protected function setUp(): void
5353

5454
/**
5555
* @group time-sensitive
56+
*
5657
* @dataProvider provideCreateLoginLinkData
5758
*/
5859
public function testCreateLoginLink($user, array $extraProperties, Request $request = null)
@@ -68,7 +69,7 @@ public function testCreateLoginLink($user, array $extraProperties, Request $requ
6869
// allow a small expiration offset to avoid time-sensitivity
6970
&& abs(time() + 600 - $parameters['expires']) <= 1
7071
// make sure hash is what we expect
71-
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], array_values($extraProperties));
72+
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], $extraProperties);
7273
}),
7374
UrlGeneratorInterface::ABSOLUTE_URL
7475
)
@@ -115,7 +116,7 @@ public function provideCreateLoginLinkData()
115116
public function testConsumeLoginLink()
116117
{
117118
$expires = time() + 500;
118-
$signature = $this->createSignatureHash('weaverryan', $expires, ['ryan@symfonycasts.com', 'pwhash']);
119+
$signature = $this->createSignatureHash('weaverryan', $expires);
119120
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
120121

121122
$user = new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash');
@@ -131,44 +132,37 @@ public function testConsumeLoginLink()
131132

132133
public function testConsumeLoginLinkWithExpired()
133134
{
134-
$this->expectException(ExpiredLoginLinkException::class);
135135
$expires = time() - 500;
136-
$signature = $this->createSignatureHash('weaverryan', $expires, ['ryan@symfonycasts.com', 'pwhash']);
136+
$signature = $this->createSignatureHash('weaverryan', $expires);
137137
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
138138

139-
$user = new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash');
140-
$this->userProvider->createUser($user);
141-
142139
$linker = $this->createLinker(['max_uses' => 3]);
140+
$this->expectException(ExpiredLoginLinkException::class);
143141
$linker->consumeLoginLink($request);
144142
}
145143

146144
public function testConsumeLoginLinkWithUserNotFound()
147145
{
148-
$this->expectException(InvalidLoginLinkException::class);
149-
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires=10000');
146+
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires='.(time() + 500));
150147

151148
$linker = $this->createLinker();
149+
$this->expectException(InvalidLoginLinkException::class);
152150
$linker->consumeLoginLink($request);
153151
}
154152

155153
public function testConsumeLoginLinkWithDifferentSignature()
156154
{
157-
$this->expectException(InvalidLoginLinkException::class);
158155
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=fake_hash&expires=%d', time() + 500));
159156

160-
$user = new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash');
161-
$this->userProvider->createUser($user);
162-
163157
$linker = $this->createLinker();
158+
$this->expectException(InvalidLoginLinkException::class);
164159
$linker->consumeLoginLink($request);
165160
}
166161

167162
public function testConsumeLoginLinkExceedsMaxUsage()
168163
{
169-
$this->expectException(ExpiredLoginLinkException::class);
170164
$expires = time() + 500;
171-
$signature = $this->createSignatureHash('weaverryan', $expires, ['ryan@symfonycasts.com', 'pwhash']);
165+
$signature = $this->createSignatureHash('weaverryan', $expires);
172166
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
173167

174168
$user = new TestLoginLinkHandlerUser('weaverryan', 'ryan@symfonycasts.com', 'pwhash');
@@ -179,6 +173,7 @@ public function testConsumeLoginLinkExceedsMaxUsage()
179173
$this->expiredLinkCache->save($item);
180174

181175
$linker = $this->createLinker(['max_uses' => 3]);
176+
$this->expectException(ExpiredLoginLinkException::class);
182177
$linker->consumeLoginLink($request);
183178
}
184179

@@ -206,15 +201,12 @@ public function testConsumeLoginLinkWithMissingExpiration()
206201
$linker->consumeLoginLink($request);
207202
}
208203

209-
private function createSignatureHash(string $username, int $expires, array $extraFields): string
204+
private function createSignatureHash(string $username, int $expires, array $extraFields = ['emailProperty' => 'ryan@symfonycasts.com', 'passwordProperty' => 'pwhash']): string
210205
{
211-
$fields = [base64_encode($username), $expires];
212-
foreach ($extraFields as $extraField) {
213-
$fields[] = base64_encode($extraField);
214-
}
206+
$hasher = new SignatureHasher($this->propertyAccessor, array_keys($extraFields), 's3cret');
207+
$user = new TestLoginLinkHandlerUser($username, $extraFields['emailProperty'] ?? '', $extraFields['passwordProperty'] ?? '', $extraFields['lastAuthenticatedAt'] ?? null);
215208

216-
// matches hash logic in the class
217-
return base64_encode(hash_hmac('sha256', implode(':', $fields), 's3cret'));
209+
return $hasher->computeSignatureHash($user, $expires);
218210
}
219211

220212
private function createLinker(array $options = [], array $extraProperties = ['emailProperty', 'passwordProperty']): LoginLinkHandler
@@ -298,7 +290,7 @@ public function loadUserByIdentifier(string $userIdentifier): TestLoginLinkHandl
298290

299291
public function refreshUser(UserInterface $user): TestLoginLinkHandlerUser
300292
{
301-
return $this->users[$username];
293+
return $this->users[$user->getUserIdentifier()];
302294
}
303295

304296
public function supportsClass(string $class): bool

‎Tests/RememberMe/PersistentRememberMeHandlerTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ public function testConsumeRememberMeCookieValid()
9393

9494
/** @var Cookie $cookie */
9595
$cookie = $this->request->attributes->get(ResponseListener::COOKIE_ATTR_NAME);
96-
$rememberParts = explode(':', base64_decode($rememberMeDetails->toString()), 4);
97-
$cookieParts = explode(':', base64_decode($cookie->getValue()), 4);
96+
$rememberParts = explode(':', $rememberMeDetails->toString(), 4);
97+
$cookieParts = explode(':', $cookie->getValue(), 4);
9898

9999
$this->assertSame($rememberParts[0], $cookieParts[0]); // class
100100
$this->assertSame($rememberParts[1], $cookieParts[1]); // identifier

0 commit comments

Comments
 (0)