From 0cf874e23e12cad4e3546de10aa793e5fe8a31f0 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 18 May 2018 09:42:46 +0200 Subject: [PATCH 1/4] [HttpFoundation] Break infinite loop in PdoSessionHandler when MySQL is in loose mode --- .../Session/Storage/Handler/PdoSessionHandler.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php index 0825ee6ea9..bb000f5c9a 100644 --- a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php +++ b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php @@ -494,6 +494,7 @@ class PdoSessionHandler implements \SessionHandlerInterface $selectSql = $this->getSelectSql(); $selectStmt = $this->pdo->prepare($selectSql); $selectStmt->bindParam(':id', $sessionId, \PDO::PARAM_STR); + $insertStmt = null; do { $selectStmt->execute(); @@ -509,6 +510,11 @@ class PdoSessionHandler implements \SessionHandlerInterface return is_resource($sessionRows[0][0]) ? stream_get_contents($sessionRows[0][0]) : $sessionRows[0][0]; } + if (null !== $insertStmt) { + $this->rollback(); + throw new \RuntimeException('Failed to read session: INSERT reported a duplicate id but next SELECT did not return any data.'); + } + if (self::LOCK_TRANSACTIONAL === $this->lockMode && 'sqlite' !== $this->driver) { // Exclusive-reading of non-existent rows does not block, so we need to do an insert to block // until other connections to the session are committed. From a5855e8c9700c4f438cfad5e3e2cbf5994298605 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Sat, 10 Feb 2018 17:35:59 -0500 Subject: [PATCH 2/4] Adding session strategy to ALL listeners to avoid *any* possible fixation --- .../AbstractPreAuthenticatedListener.php | 15 +++++++++++++++ .../Firewall/BasicAuthenticationListener.php | 16 ++++++++++++++++ .../Firewall/DigestAuthenticationListener.php | 14 ++++++++++++++ .../Firewall/SimplePreAuthenticationListener.php | 16 ++++++++++++++++ .../Session/SessionAuthenticationStrategy.php | 5 ++++- .../SessionAuthenticationStrategyInterface.php | 4 ++-- 6 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php b/src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php index 0065fe8237..2054c4aa07 100644 --- a/src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php @@ -82,6 +82,9 @@ abstract class AbstractPreAuthenticatedListener implements ListenerInterface if (null !== $this->logger) { $this->logger->info('Pre-authentication successful.', array('token' => (string) $token)); } + + $this->migrateSession($request); + $this->tokenStorage->setToken($token); if (null !== $this->dispatcher) { @@ -114,4 +117,16 @@ abstract class AbstractPreAuthenticatedListener implements ListenerInterface * @return array An array composed of the user and the credentials */ abstract protected function getPreAuthenticatedData(Request $request); + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } } diff --git a/src/Symfony/Component/Security/Http/Firewall/BasicAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/BasicAuthenticationListener.php index 1ddc416434..63bd013c64 100644 --- a/src/Symfony/Component/Security/Http/Firewall/BasicAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/BasicAuthenticationListener.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Security\Http\Firewall; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; @@ -70,6 +71,9 @@ class BasicAuthenticationListener implements ListenerInterface try { $token = $this->authenticationManager->authenticate(new UsernamePasswordToken($username, $request->headers->get('PHP_AUTH_PW'), $this->providerKey)); + + $this->migrateSession($request); + $this->tokenStorage->setToken($token); } catch (AuthenticationException $e) { $token = $this->tokenStorage->getToken(); @@ -88,4 +92,16 @@ class BasicAuthenticationListener implements ListenerInterface $event->setResponse($this->authenticationEntryPoint->start($request, $e)); } } + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } } diff --git a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php index 070d61a674..91b46e4f81 100644 --- a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php @@ -118,6 +118,8 @@ class DigestAuthenticationListener implements ListenerInterface $this->logger->info('Digest authentication successful.', array('username' => $digestAuth->getUsername(), 'received' => $digestAuth->getResponse())); } + $this->migrateSession($request); + $this->tokenStorage->setToken(new UsernamePasswordToken($user, $user->getPassword(), $this->providerKey)); } @@ -134,6 +136,18 @@ class DigestAuthenticationListener implements ListenerInterface $event->setResponse($this->authenticationEntryPoint->start($request, $authException)); } + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } } class DigestData diff --git a/src/Symfony/Component/Security/Http/Firewall/SimplePreAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/SimplePreAuthenticationListener.php index dd51869405..23e517969f 100644 --- a/src/Symfony/Component/Security/Http/Firewall/SimplePreAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/SimplePreAuthenticationListener.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Security\Http\Firewall; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface; use Psr\Log\LoggerInterface; use Symfony\Component\HttpKernel\Event\GetResponseEvent; @@ -85,6 +86,9 @@ class SimplePreAuthenticationListener implements ListenerInterface } $token = $this->authenticationManager->authenticate($token); + + $this->migrateSession($request); + $this->tokenStorage->setToken($token); if (null !== $this->dispatcher) { @@ -119,4 +123,16 @@ class SimplePreAuthenticationListener implements ListenerInterface } } } + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } } diff --git a/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategy.php b/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategy.php index b431289392..15e9b24bb9 100644 --- a/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategy.php +++ b/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategy.php @@ -47,8 +47,11 @@ class SessionAuthenticationStrategy implements SessionAuthenticationStrategyInte return; case self::MIGRATE: + // Note: this logic is duplicated in several authentication listeners + // until Symfony 5.0 due to a security fix with BC compat + // Destroying the old session is broken in php 5.4.0 - 5.4.10 - // See php bug #63379 + // See https://bugs.php.net/63379 $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; $request->getSession()->migrate($destroy); diff --git a/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategyInterface.php b/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategyInterface.php index 9b05f15134..8de89b1868 100644 --- a/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategyInterface.php +++ b/src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategyInterface.php @@ -27,8 +27,8 @@ interface SessionAuthenticationStrategyInterface /** * This performs any necessary changes to the session. * - * This method is called before the TokenStorage is populated with a - * Token, and only by classes inheriting from AbstractAuthenticationListener. + * This method should be called before the TokenStorage is populated with a + * Token. It should be used by authentication listeners when a session is used. */ public function onAuthentication(Request $request, TokenInterface $token); } From f2e83ba44df88adea3268ab81380417cb7366538 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Thu, 1 Feb 2018 08:53:47 -0500 Subject: [PATCH 3/4] Adding session authentication strategy to Guard to avoid session fixation --- .../Security/Guard/GuardAuthenticatorHandler.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php b/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php index 3b62c41253..5e6eba339b 100644 --- a/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php +++ b/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php @@ -46,6 +46,7 @@ class GuardAuthenticatorHandler */ public function authenticateWithToken(TokenInterface $token, Request $request) { + $this->migrateSession($request); $this->tokenStorage->setToken($token); if (null !== $this->dispatcher) { @@ -127,4 +128,16 @@ class GuardAuthenticatorHandler is_object($response) ? get_class($response) : gettype($response) )); } + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } } From 891ae6b28b33dc3482ed6a5724976cf01b57df78 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Tue, 22 May 2018 10:38:56 -0400 Subject: [PATCH 4/4] migrating session for UsernamePasswordJsonAuthenticationListener --- .../UsernamePasswordJsonAuthenticationListener.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php index 955288c23c..748ba07ad8 100644 --- a/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php @@ -139,6 +139,8 @@ class UsernamePasswordJsonAuthenticationListener implements ListenerInterface $this->logger->info('User has been authenticated successfully.', array('username' => $token->getUsername())); } + $this->migrateSession($request); + $this->tokenStorage->setToken($token); if (null !== $this->eventDispatcher) { @@ -182,4 +184,15 @@ class UsernamePasswordJsonAuthenticationListener implements ListenerInterface return $response; } + + private function migrateSession(Request $request) + { + if (!$request->hasSession() || !$request->hasPreviousSession()) { + return; + } + // Destroying the old session is broken in php 5.4.0 - 5.4.10 + // See https://bugs.php.net/63379 + $destroy = \PHP_VERSION_ID < 50400 || \PHP_VERSION_ID >= 50411; + $request->getSession()->migrate($destroy); + } }