security #cve-2019-18886 [Security\Core] throw AccessDeniedException when switch user fails (nicolas-grekas)
This PR was merged into the 4.3 branch.
This commit is contained in:
commit
3ae3094a18
@ -68,7 +68,7 @@ class SwitchUserTest extends AbstractWebTestCase
|
|||||||
return [
|
return [
|
||||||
'unauthorized_user_cannot_switch' => ['user_cannot_switch_1', 'user_cannot_switch_1', 'user_cannot_switch_1', 403],
|
'unauthorized_user_cannot_switch' => ['user_cannot_switch_1', 'user_cannot_switch_1', 'user_cannot_switch_1', 403],
|
||||||
'authorized_user_can_switch' => ['user_can_switch', 'user_cannot_switch_1', 'user_cannot_switch_1', 200],
|
'authorized_user_can_switch' => ['user_can_switch', 'user_cannot_switch_1', 'user_cannot_switch_1', 200],
|
||||||
'authorized_user_cannot_switch_to_non_existent' => ['user_can_switch', 'user_does_not_exist', 'user_can_switch', 500],
|
'authorized_user_cannot_switch_to_non_existent' => ['user_can_switch', 'user_does_not_exist', 'user_can_switch', 403],
|
||||||
'authorized_user_can_switch_to_himself' => ['user_can_switch', 'user_can_switch', 'user_can_switch', 200],
|
'authorized_user_can_switch_to_himself' => ['user_can_switch', 'user_can_switch', 'user_can_switch', 200],
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
@ -24,7 +24,7 @@
|
|||||||
"symfony/security-core": "~4.3",
|
"symfony/security-core": "~4.3",
|
||||||
"symfony/security-csrf": "~4.2",
|
"symfony/security-csrf": "~4.2",
|
||||||
"symfony/security-guard": "~4.2",
|
"symfony/security-guard": "~4.2",
|
||||||
"symfony/security-http": "^4.3"
|
"symfony/security-http": "^4.3.8"
|
||||||
},
|
},
|
||||||
"require-dev": {
|
"require-dev": {
|
||||||
"symfony/asset": "~3.4|~4.0",
|
"symfony/asset": "~3.4|~4.0",
|
||||||
|
@ -105,7 +105,8 @@ class SwitchUserListener implements ListenerInterface
|
|||||||
try {
|
try {
|
||||||
$this->tokenStorage->setToken($this->attemptSwitchUser($request, $username));
|
$this->tokenStorage->setToken($this->attemptSwitchUser($request, $username));
|
||||||
} catch (AuthenticationException $e) {
|
} catch (AuthenticationException $e) {
|
||||||
throw new \LogicException(sprintf('Switch User failed: "%s"', $e->getMessage()));
|
// Generate 403 in any conditions to prevent user enumeration vulnerabilities
|
||||||
|
throw new AccessDeniedException('Switch User failed: '.$e->getMessage(), $e);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -142,8 +143,25 @@ class SwitchUserListener implements ListenerInterface
|
|||||||
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
|
throw new \LogicException(sprintf('You are already switched to "%s" user.', $token->getUsername()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$currentUsername = $token->getUsername();
|
||||||
|
$nonExistentUsername = '_'.md5(random_bytes(8).$username);
|
||||||
|
|
||||||
|
// To protect against user enumeration via timing measurements
|
||||||
|
// we always load both successfully and unsuccessfully
|
||||||
|
try {
|
||||||
$user = $this->provider->loadUserByUsername($username);
|
$user = $this->provider->loadUserByUsername($username);
|
||||||
|
|
||||||
|
try {
|
||||||
|
$this->provider->loadUserByUsername($nonExistentUsername);
|
||||||
|
throw new \LogicException('AuthenticationException expected');
|
||||||
|
} catch (AuthenticationException $e) {
|
||||||
|
}
|
||||||
|
} catch (AuthenticationException $e) {
|
||||||
|
$this->provider->loadUserByUsername($currentUsername);
|
||||||
|
|
||||||
|
throw $e;
|
||||||
|
}
|
||||||
|
|
||||||
if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) {
|
if (false === $this->accessDecisionManager->decide($token, [$this->role], $user)) {
|
||||||
$exception = new AccessDeniedException();
|
$exception = new AccessDeniedException();
|
||||||
$exception->setAttributes($this->role);
|
$exception->setAttributes($this->role);
|
||||||
|
@ -18,6 +18,7 @@ use Symfony\Component\HttpKernel\HttpKernelInterface;
|
|||||||
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
|
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
|
||||||
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
|
use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
|
||||||
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
|
use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
|
||||||
|
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
|
||||||
use Symfony\Component\Security\Core\Role\SwitchUserRole;
|
use Symfony\Component\Security\Core\Role\SwitchUserRole;
|
||||||
use Symfony\Component\Security\Core\User\User;
|
use Symfony\Component\Security\Core\User\User;
|
||||||
use Symfony\Component\Security\Http\Event\SwitchUserEvent;
|
use Symfony\Component\Security\Http\Event\SwitchUserEvent;
|
||||||
@ -174,6 +175,7 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
{
|
{
|
||||||
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
|
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
|
||||||
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
|
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_FOO']);
|
||||||
|
$user = new User('username', 'password', []);
|
||||||
|
|
||||||
$this->tokenStorage->setToken($token);
|
$this->tokenStorage->setToken($token);
|
||||||
$this->request->query->set('_switch_user', 'kuba');
|
$this->request->query->set('_switch_user', 'kuba');
|
||||||
@ -182,6 +184,31 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
|
||||||
->willReturn(false);
|
->willReturn(false);
|
||||||
|
|
||||||
|
$this->userProvider->expects($this->exactly(2))
|
||||||
|
->method('loadUserByUsername')
|
||||||
|
->withConsecutive(['kuba'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
|
|
||||||
|
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
|
||||||
|
$listener($this->event);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function testSwitchUserTurnsAuthenticationExceptionTo403()
|
||||||
|
{
|
||||||
|
$this->expectException('Symfony\Component\Security\Core\Exception\AccessDeniedException');
|
||||||
|
$token = new UsernamePasswordToken('username', '', 'key', ['ROLE_ALLOWED_TO_SWITCH']);
|
||||||
|
|
||||||
|
$this->tokenStorage->setToken($token);
|
||||||
|
$this->request->query->set('_switch_user', 'kuba');
|
||||||
|
|
||||||
|
$this->accessDecisionManager->expects($this->never())
|
||||||
|
->method('decide');
|
||||||
|
|
||||||
|
$this->userProvider->expects($this->exactly(2))
|
||||||
|
->method('loadUserByUsername')
|
||||||
|
->withConsecutive(['kuba'], ['username'])
|
||||||
|
->will($this->onConsecutiveCalls($this->throwException(new UsernameNotFoundException())));
|
||||||
|
|
||||||
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
|
$listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', $this->accessDecisionManager);
|
||||||
$listener($this->event);
|
$listener($this->event);
|
||||||
}
|
}
|
||||||
@ -198,9 +225,10 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
||||||
->willReturn(true);
|
->willReturn(true);
|
||||||
|
|
||||||
$this->userProvider->expects($this->once())
|
$this->userProvider->expects($this->exactly(2))
|
||||||
->method('loadUserByUsername')->with('kuba')
|
->method('loadUserByUsername')
|
||||||
->willReturn($user);
|
->withConsecutive(['kuba'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
$this->userChecker->expects($this->once())
|
$this->userChecker->expects($this->once())
|
||||||
->method('checkPostAuth')->with($user);
|
->method('checkPostAuth')->with($user);
|
||||||
|
|
||||||
@ -224,9 +252,10 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'])
|
||||||
->willReturn(true);
|
->willReturn(true);
|
||||||
|
|
||||||
$this->userProvider->expects($this->once())
|
$this->userProvider->expects($this->exactly(2))
|
||||||
->method('loadUserByUsername')->with('0')
|
->method('loadUserByUsername')
|
||||||
->willReturn($user);
|
->withConsecutive(['0'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
$this->userChecker->expects($this->once())
|
$this->userChecker->expects($this->once())
|
||||||
->method('checkPostAuth')->with($user);
|
->method('checkPostAuth')->with($user);
|
||||||
|
|
||||||
@ -254,9 +283,10 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
||||||
->willReturn(true);
|
->willReturn(true);
|
||||||
|
|
||||||
$this->userProvider->expects($this->once())
|
$this->userProvider->expects($this->exactly(2))
|
||||||
->method('loadUserByUsername')->with('kuba')
|
->method('loadUserByUsername')
|
||||||
->willReturn($user);
|
->withConsecutive(['kuba'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
$this->userChecker->expects($this->once())
|
$this->userChecker->expects($this->once())
|
||||||
->method('checkPostAuth')->with($user);
|
->method('checkPostAuth')->with($user);
|
||||||
|
|
||||||
@ -282,9 +312,10 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
||||||
->willReturn(true);
|
->willReturn(true);
|
||||||
|
|
||||||
$this->userProvider->expects($this->any())
|
$this->userProvider->expects($this->exactly(2))
|
||||||
->method('loadUserByUsername')->with('kuba')
|
->method('loadUserByUsername')
|
||||||
->willReturn($user);
|
->withConsecutive(['kuba'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
|
|
||||||
$dispatcher = $this->getMockBuilder(EventDispatcherInterface::class)->getMock();
|
$dispatcher = $this->getMockBuilder(EventDispatcherInterface::class)->getMock();
|
||||||
$dispatcher
|
$dispatcher
|
||||||
@ -329,9 +360,10 @@ class SwitchUserListenerTest extends TestCase
|
|||||||
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
->method('decide')->with($token, ['ROLE_ALLOWED_TO_SWITCH'], $user)
|
||||||
->willReturn(true);
|
->willReturn(true);
|
||||||
|
|
||||||
$this->userProvider->expects($this->once())
|
$this->userProvider->expects($this->exactly(2))
|
||||||
->method('loadUserByUsername')->with('kuba')
|
->method('loadUserByUsername')
|
||||||
->willReturn($user);
|
->withConsecutive(['kuba'])
|
||||||
|
->will($this->onConsecutiveCalls($user, $this->throwException(new UsernameNotFoundException())));
|
||||||
$this->userChecker->expects($this->once())
|
$this->userChecker->expects($this->once())
|
||||||
->method('checkPostAuth')->with($user);
|
->method('checkPostAuth')->with($user);
|
||||||
|
|
||||||
|
Reference in New Issue
Block a user