8000 bug #49078 [Security/Http] Check tokens before loading users from pro… · symfony/symfony@1eebf31 · GitHub
[go: up one dir, main page]
Skip to content

Commit 1eebf31

Browse files
nicolas-grekasnicolas-grekas
committed
bug #49078 [Security/Http] Check tokens before loading users from providers (nicolas-grekas)
This PR was merged into the 5.4 branch. Discussion ---------- [Security/Http] Check tokens before loading users from providers | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Remember me cookies and login link handler tokens contain an expiry but we check this expiry only after we've loaded a user from a provider. This can create unneeded load on the provider. Note that the now legacy security subsystem was free from this issue so this PR is fixing a regression. For persistent tokens, I've removed any logic to sign them in `PersistentRememberMeHandler` because we never validate the signature, so it's just useless. Commits ------- 889d739 [Security/Http] Check tokens before loading users from providers
2 parents 01344e3 + 889d739 commit 1eebf31

9 files changed

+123
-104
lines changed

‎src/Symfony/Component/Security/Core/Signature/SignatureHasher.php

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,24 +45,48 @@ public function __construct(PropertyAccessorInterface $propertyAccessor, array $
4545
}
4646

4747
/**
48-
* Verifies the hash using the provided user and expire time.
48+
* Verifies the hash using the provided user identifier and expire time.
49+
*
50+
* This method must be called before the user object is loaded from a provider.
4951
*
5052
* @param int $expires The expiry time as a unix timestamp
5153
* @param string $hash The plaintext hash provided by the request
5254
*
5355
* @throws InvalidSignatureException If the signature does not match the provided parameters
5456
* @throws ExpiredSignatureException If the signature is no longer valid
5557
*/
56-
public function verifySignatureHash(UserInterface $user, int $expires, string $hash): void
58+
public function acceptSignatureHash(string $userIdentifier, int $expires, string $hash): void
5759
{
58-
if (!hash_equals($hash, $this->computeSignatureHash($user, $expires))) {
60+
if ($expires < time()) {
61+
throw new ExpiredSignatureException('Signature has expired.');
62+
}
63+
$hmac = substr($hash, 0, 44);
64+
$payload = substr($hash, 44).':'.$expires.':'.$userIdentifier;
65+
66+
if (!hash_equals($hmac, $this->generateHash($payload))) {
5967
throw new InvalidSignatureException('Invalid or expired signature.');
6068
}
69+
}
6170

71+
/**
72+
* Verifies the hash using the provided user and expire time.
73+
*
74+
* @param int $expires The expiry time as a unix timestamp
75+
* @param string $hash The plaintext hash provided by the request
76+
*
77+
* @throws InvalidSignatureException If the signature does not match the provided parameters
78+
* @throws ExpiredSignatureException If the signature is no longer valid
79+
*/
80+
public function verifySignatureHash(UserInterface $user, int $expires, string $hash): void
81+
{
6282
if ($expires < time()) {
6383
throw new ExpiredSignatureException('Signature has expired.');
6484
}
6585

86+
if (!hash_equals($hash, $this->computeSignatureHash($user, $expires))) {
87+
throw new InvalidSignatureException('Invalid or expired signature.');
88+
}
89+
6690
if ($this->expiredSignaturesStorage && $this->maxUses) {
6791
if ($this->expiredSignaturesStorage->countUsages($hash) >= $this->maxUses) {
6892
throw new ExpiredSignatureException(sprintf('Signature can only be used "%d" times.', $this->maxUses));
@@ -79,7 +103,8 @@ public function verifySignatureHash(UserInterface $user, int $expires, string $h
79103
*/
80104
public function computeSignatureHash(UserInterface $user, int $expires): string
81105
{
82-
$signatureFields = [base64_encode(method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername()), $expires];
106+
$userIdentifier = method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername();
107+
$fieldsHash = hash_init('sha256');
83108

84109
foreach ($this->signatureProperties as $property) {
85110
$value = $this->propertyAccessor->getValue($user, $property) ?? '';
@@ -90,9 +115,16 @@ public function computeSignatureHash(UserInterface $user, int $expires): string
90115
if (!\is_scalar($value) && !(\is_object($value) && method_exists($value, '__toString'))) {
91116
throw new \InvalidArgumentException(sprintf('The property path "%s" on the user object "%s" must return a value that can be cast to a string, but "%s" was returned.', $property, \get_class($user), get_debug_type($value)));
92117
}
93-
$signatureFields[] = base64_encode($value);
118+
hash_update($fieldsHash, ':'.base64_encode($value));
94119
}
95120

96-
return base64_encode(hash_hmac('sha256', implode(':', $signatureFields), $this->secret));
121+
$fieldsHash = strtr(base64_encode(hash_final($fieldsHash, true)), '+/=', '-_~');
122+
123+
return $this->generateHash($fieldsHash.':'.$expires.':'.$userIdentifier).$fieldsHash;
124+
}
125+
126+
private function generateHash(string $tokenValue): string
127+
{
128+
return strtr(base64_encode(hash_hmac('sha256', $tokenValue, $this->secret, true)), '+/=', '-_~');
97129
}
98130
}

‎src/Symfony/Component/Security/Http/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) {

‎src/Symfony/Component/Security/Http/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
}

‎src/Symfony/Component/Security/Http/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
}

‎src/Symfony/Component/Security/Http/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 {

‎src/Symfony/Component/Security/Http/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['< 10000 span class="pl-s">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

‎src/Symfony/Component/Security/Http/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)
0