Skip to content

Commit 5ac0763

Browse files
nicolas-grekasnicolas-grekas
committed
security #cve-2019-18886 [Security\Core] throw AccessDeniedException when switch user fails (nicolas-grekas)
This PR was merged into the 4.2 branch.
2 parents 5098b66 + 7bd4a92 commit 5ac0763

File tree

4 files changed

+67
-16
lines changed

4 files changed

+67
-16
lines changed

‎src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserTest.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public function getTestParameters()
6868
return [
6969
'unauthorized_user_cannot_switch' => ['user_cannot_switch_1', 'user_cannot_switch_1', 'user_cannot_switch_1', 403],
7070
'authorized_user_can_switch' => ['user_can_switch', 'user_cannot_switch_1', 'user_cannot_switch_1', 200],
71-
'authorized_user_cannot_switch_to_non_existent' => ['user_can_switch', 'user_does_not_exist', 'user_can_switch', 500],
71+
'authorized_user_cannot_switch_to_non_existent' => ['user_can_switch', 'user_does_not_exist', 'user_can_switch', 403],
7272
'authorized_user_can_switch_to_himself' => ['user_can_switch', 'user_can_switch', 'user_can_switch', 200],
7373
];
7474
}

‎src/Symfony/Bundle/SecurityBundle/composer.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
"symfony/security-core": "~4.2",
2525
"symfony/security-csrf": "~4.2",
2626
"symfony/security-guard": "~4.2",
27-
"symfony/security-http": "~4.2"
27+
"symfony/security-http": "^4.2.12"
2828
},
2929
"require-dev": {
3030
"symfony/asset": "~3.4|~4.0",

‎src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php

+20-2
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ public function handle(GetResponseEvent $event)
9393
try {
9494
$this->tokenStorage->setToken($this->attemptSwitchUser($request, $username));
9595
} catch (AuthenticationException $e) {
96-
throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage()));
96+
// Generate 403 in any conditions to prevent user enumeration vulnerabilities
97+
throw new AccessDeniedException('Switch User failed: '.$e->getMessage(), $e);
9798
}
9899
}
99100

@@ -130,7 +131,24 @@ private function attemptSwitchUser(Request $request, $username)
130131
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
131132
}
132133

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

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

‎src/Symfony/Component/Security/Http/Tests/Firewall/SwitchUserListenerTest.php

+45-12
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Symfony\Component\HttpKernel\HttpKernelInterface;
1818
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
1919
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
20+
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
2021
use Symfony\Component\Security\Core\Role\SwitchUserRole;
2122
use Symfony\Component\Security\Core\User\User;
2223
use Symfony\Component\Security\Http\Event\SwitchUserEvent;
@@ -161,6 +162,7 @@ public function testExitUserDoesNotDispatchEventWithStringUser()
161162
public function testSwitchUserIsDisallowed()
162163
{
163164
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
165+
$user = new User('username', 'password', []);
164166

165167
$this->tokenStorage->setToken($token);
166168
$this->request->query->set('_switch_user', 'kuba');
@@ -169,6 +171,33 @@ public function testSwitchUserIsDisallowed()
169171
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
170172
->willReturn(false);
171173

174+
$this->userProvider->expects($this->exactly(2))
175+
->method('loadUserByUsername')
176+
->withConsecutive(['kuba'])
177+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
178+
179+
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
180+
$listener->handle($this->event);
181+
}
182+
183+
/**
184+
* @expectedException \Symfony\Component\Security\Core\Exception\AccessDeniedException
185+
*/
186+
public function testSwitchUserTurnsAuthenticationExceptionTo403()
187+
{
188+
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_ALLOWED_TO_SWITCH']);
189+
190+
$this->tokenStorage->setToken($token);
191+
$this->request->query->set('_switch_user', 'kuba');
192+
193+
$this->accessDecisionManager->expects($this->never())
194+
->method('decide');
195+
196+
$this->userProvider->expects($this->exactly(2))
197+
->method('loadUserByUsername')
198+
->withConsecutive(['kuba'], ['username'])
199+
->will($this->onConsecutiveCalls($this->throwException(new UsernameNotFoundException())));
200+
172201
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
173202
$listener->handle($this->event);
174203
}
@@ -185,9 +214,10 @@ public function testSwitchUser()
185214
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
186215
->willReturn(true);
187216

188-
$this->userProvider->expects($this->once())
189-
->method('loadUserByUsername')->with('kuba')
190-
->willReturn($user);
217+
$this->userProvider->expects($this->exactly(2))
218+
->method('loadUserByUsername')
219+
->withConsecutive(['kuba'])
220+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
191221
$this->userChecker->expects($this->once())
192222
->method('checkPostAuth')->with($user);
193223

@@ -215,9 +245,10 @@ public function testSwitchUserKeepsOtherQueryStringParameters()
215245
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
216246
->willReturn(true);
217247

218-
$this->userProvider->expects($this->once())
219-
->method('loadUserByUsername')->with('kuba')
220-
->willReturn($user);
248+
$this->userProvider->expects($this->exactly(2))
249+
->method('loadUserByUsername')
250+
->withConsecutive(['kuba'])
251+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
221252
$this->userChecker->expects($this->once())
222253
->method('checkPostAuth')->with($user);
223254

@@ -243,9 +274,10 @@ public function testSwitchUserWithReplacedToken()
243274
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
244275
->willReturn(true);
245276

246-
$this->userProvider->expects($this->any())
247-
->method('loadUserByUsername')->with('kuba')
248-
->willReturn($user);
277+
$this->userProvider->expects($this->exactly(2))
278+
->method('loadUserByUsername')
279+
->withConsecutive(['kuba'])
280+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
249281

250282
$dispatcher = $this->getMockBuilder('Symfony\Component\EventDispatcher\EventDispatcherInterface')->getMock();
251283
$dispatcher
@@ -290,9 +322,10 @@ public function testSwitchUserStateless()
290322
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
291323
->willReturn(true);
292324

293-
$this->userProvider->expects($this->once())
294-
->method('loadUserByUsername')->with('kuba')
295-
->willReturn($user);
325+
$this->userProvider->expects($this->exactly(2))
326+
->method('loadUserByUsername')
327+
->withConsecutive(['kuba'])
328+
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
296329
$this->userChecker->expects($this->once())
297330
->method('checkPostAuth')->with($user);
298331

0 commit comments

Comments
 (0)