Skip to content

Commit ee5f4d2

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '4.4'
* 4.4: [Console] Constant STDOUT might be undefined. Add missing conflict with symfony/serializer <4.4 Allow returning null from NormalizerInterface::normalize bumped Symfony version to 4.4.0 updated VERSION for 4.4.0-BETA1 updated CHANGELOG for 4.4.0-BETA1 [Security\Core] throw AccessDeniedException when switch user fails [Mime] fix guessing mime-types of files with leading dash [HttpFoundation] fix guessing mime-types of files with leading dash [VarExporter] fix exporting some strings [Cache] forbid serializing AbstractAdapter and TagAwareAdapter instances Use constant time comparison in UriSigner
2 parents 1b452e4 + ddc39f7 commit ee5f4d2

File tree

2 files changed

+67
-17
lines changed

2 files changed

+67
-17
lines changed

‎Firewall/SwitchUserListener.php

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ public function __invoke(RequestEvent $event)
101101
try {
102102
$this->tokenStorage->setToken($this->attemptSwitchUser($request, $username));
103103
} catch (AuthenticationException $e) {
104-
throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage()));
104+
// Generate 403 in any conditions to prevent user enumeration vulnerabilities
105+
throw new AccessDeniedException('Switch User failed: '.$e->getMessage(), $e);
105106
}
106107
}
107108

@@ -133,7 +134,24 @@ private function attemptSwitchUser(Request $request, string $username): ?TokenIn
133134
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
134135
}
135136

136-
$user = $this->provider->loadUserByUsername($username);
137+
$currentUsername = $token->getUsername();
138+
$nonExistentUsername = '_'.md5(random_bytes(8).$username);
139+
140+
// To protect against user enumeration via timing measurements
141+
// we always load both successfully and unsuccessfully
142+
try {
143+
$user = $this->provider->loadUserByUsername($username);
144+
145+
try {
146+
$this->provider->loadUserByUsername($nonExistentUsername);
147+
throw new \LogicException('AuthenticationException expected');
148+
} catch (AuthenticationException $e) {
149+
}
150+
} catch (AuthenticationException $e) {
151+
$this->provider->loadUserByUsername($currentUsername);
152+
153+
throw $e;
154+
}
137155

138156
if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) {
139157
$exception = new AccessDeniedException();

‎Tests/Firewall/SwitchUserListenerTest.php

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
1919
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
2020
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
21+
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
2122
use Symfony\Component\Security\Core\User\User;
2223
use Symfony\Component\Security\Http\Event\SwitchUserEvent;
2324
use Symfony\Component\Security\Http\Firewall\SwitchUserListener;
@@ -157,6 +158,7 @@ public function testSwitchUserIsDisallowed()
157158
{
158159
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
159160
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
161+
$user = new User('username', 'password', []);
160162

161163
$this->tokenStorage->setToken($token);
162164
$this->request->query->set('_switch_user', 'kuba');
@@ -165,6 +167,31 @@ public function testSwitchUserIsDisallowed()
165167
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
166168
->willReturn(false);
167169

170+
$this->userProvider->expects($this->exactly(2))
171+
->method('loadUserByUsername')
172+
->withConsecutive(['kuba'])
173+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
174+
175+
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
176+
$listener($this->event);
177+
}
178+
179+
public function testSwitchUserTurnsAuthenticationExceptionTo403()
180+
{
181+
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
182+
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_ALLOWED_TO_SWITCH']);
183+
184+
$this->tokenStorage->setToken($token);
185+
$this->request->query->set('_switch_user', 'kuba');
186+
187+
$this->accessDecisionManager->expects($this->never())
188+
->method('decide');
189+
190+
$this->userProvider->expects($this->exactly(2))
191+
->method('loadUserByUsername')
192+
->withConsecutive(['kuba'], ['username'])
193+
->will($this->onConsecutiveCalls($this->throwException(new UsernameNotFoundException())));
194+
168195
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
169196
$listener($this->event);
170197
}
@@ -181,9 +208,10 @@ public function testSwitchUser()
181208
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
182209
->willReturn(true);
183210

184-
$this->userProvider->expects($this->once())
185-
->method('loadUserByUsername')->with('kuba')
186-
->willReturn($user);
211+
$this->userProvider->expects($this->exactly(2))
212+
->method('loadUserByUsername')
213+
->withConsecutive(['kuba'])
214+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
187215
$this->userChecker->expects($this->once())
188216
->method('checkPostAuth')->with($user);
189217

@@ -207,9 +235,10 @@ public function testSwitchUserWorksWithFalsyUsernames()
207235
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
208236
->willReturn(true);
209237

210-
$this->userProvider->expects($this->once())
211-
->method('loadUserByUsername')->with('0')
212-
->willReturn($user);
238+
$this->userProvider->expects($this->exactly(2))
239+
->method('loadUserByUsername')
240+
->withConsecutive(['0'])
241+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
213242
$this->userChecker->expects($this->once())
214243
->method('checkPostAuth')->with($user);
215244

@@ -237,9 +266,10 @@ public function testSwitchUserKeepsOtherQueryStringParameters()
237266
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
238267
->willReturn(true);
239268

240-
$this->userProvider->expects($this->once())
241-
->method('loadUserByUsername')->with('kuba')
242-
->willReturn($user);
269+
$this->userProvider->expects($this->exactly(2))
270+
->method('loadUserByUsername')
271+
->withConsecutive(['kuba'])
272+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
243273
$this->userChecker->expects($this->once())
244274
->method('checkPostAuth')->with($user);
245275

@@ -265,9 +295,10 @@ public function testSwitchUserWithReplacedToken()
265295
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
266296
->willReturn(true);
267297

268-
$this->userProvider->expects($this->any())
269-
->method('loadUserByUsername')->with('kuba')
270-
->willReturn($user);
298+
$this->userProvider->expects($this->exactly(2))
299+
->method('loadUserByUsername')
300+
->withConsecutive(['kuba'])
301+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
271302

272303
$dispatcher = $this->getMockBuilder(EventDispatcherInterface::class)->getMock();
273304
$dispatcher
@@ -312,9 +343,10 @@ public function testSwitchUserStateless()
312343
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
313344
->willReturn(true);
314345

315-
$this->userProvider->expects($this->once())
316-
->method('loadUserByUsername')->with('kuba')
317-
->willReturn($user);
346+
$this->userProvider->expects($this->exactly(2))
347+
->method('loadUserByUsername')
348+
->withConsecutive(['kuba'])
349+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
318350
$this->userChecker->expects($this->once())
319351
->method('checkPostAuth')->with($user);
320352

0 commit comments

Comments
 (0)