From 41000f1de0e9a077459eb6b194f07a9e9aa9a6f5 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 25 Jan 2019 17:59:48 +0100 Subject: [PATCH 1/2] [Security] dont do nested calls to serialize() --- .../Authentication/Token/AbstractToken.php | 30 +++++++++++++------ .../Authentication/Token/AnonymousToken.php | 2 +- .../Token/PreAuthenticatedToken.php | 8 +++-- .../Authentication/Token/RememberMeToken.php | 2 +- .../Token/UsernamePasswordToken.php | 2 +- .../Core/Exception/AccountStatusException.php | 2 +- ...stomUserMessageAuthenticationException.php | 2 +- .../Exception/UsernameNotFoundException.php | 2 +- .../Token/AbstractTokenTest.php | 3 ++ .../Token/PostAuthenticationGuardToken.php | 4 +-- 10 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php index 536756ff9a6a..a8a297cdb4ce 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php @@ -134,17 +134,16 @@ public function eraseCredentials() /** * {@inheritdoc} + * + * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method + * + * @return string|array Returns an array when $isCalledFromOverridingMethod is set to true */ public function serialize() { - return serialize( - [ - \is_object($this->user) ? clone $this->user : $this->user, - $this->authenticated, - array_map(function ($role) { return clone $role; }, $this->roles), - $this->attributes, - ] - ); + $serialized = [$this->user, $this->authenticated, $this->roles, $this->attributes]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** @@ -152,7 +151,7 @@ public function serialize() */ public function unserialize($serialized) { - list($this->user, $this->authenticated, $this->roles, $this->attributes) = unserialize($serialized); + list($this->user, $this->authenticated, $this->roles, $this->attributes) = \is_array($serialized) ? $serialized : unserialize($serialized); } /** @@ -232,6 +231,19 @@ public function __toString() return sprintf('%s(user="%s", authenticated=%s, roles="%s")', $class, $this->getUsername(), json_encode($this->authenticated), implode(', ', $roles)); } + /** + * @internal + */ + protected function doSerialize($serialized, $isCalledFromOverridingMethod) + { + if (null === $isCalledFromOverridingMethod) { + $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 3); + $isCalledFromOverridingMethod = isset($trace[2]['function'], $trace[2]['object']) && 'serialize' === $trace[2]['function'] && $this === $trace[2]['object']; + } + + return $isCalledFromOverridingMethod ? $serialized : serialize($serialized); + } + private function hasUserChanged(UserInterface $user) { if (!($this->user instanceof UserInterface)) { diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php index c3af5a3d2290..167c5a8e1cfb 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php @@ -67,7 +67,7 @@ public function serialize() */ public function unserialize($serialized) { - list($this->secret, $parentStr) = unserialize($serialized); + list($this->secret, $parentStr) = \is_array($serialized) ? $serialized : unserialize($serialized); parent::unserialize($parentStr); } } diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php index d49ba54ece5c..c7d70bef245d 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php @@ -76,10 +76,14 @@ public function eraseCredentials() /** * {@inheritdoc} + * + * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method */ public function serialize() { - return serialize([$this->credentials, $this->providerKey, parent::serialize()]); + $serialized = [$this->credentials, $this->providerKey, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** @@ -87,7 +91,7 @@ public function serialize() */ public function unserialize($str) { - list($this->credentials, $this->providerKey, $parentStr) = unserialize($str); + list($this->credentials, $this->providerKey, $parentStr) = \is_array($str) ? $str : unserialize($str); parent::unserialize($parentStr); } } diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php index 9512a50eb7ce..8b8ec444fef3 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php @@ -106,7 +106,7 @@ public function serialize() */ public function unserialize($serialized) { - list($this->secret, $this->providerKey, $parentStr) = unserialize($serialized); + list($this->secret, $this->providerKey, $parentStr) = \is_array($serialized) ? $serialized : unserialize($serialized); parent::unserialize($parentStr); } } diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php index 9445f279a32a..0596f0c8a7c8 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php @@ -99,7 +99,7 @@ public function serialize() */ public function unserialize($serialized) { - list($this->credentials, $this->providerKey, $parentStr) = unserialize($serialized); + list($this->credentials, $this->providerKey, $parentStr) = \is_array($serialized) ? $serialized : unserialize($serialized); parent::unserialize($parentStr); } } diff --git a/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php b/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php index 503d3fbad48a..d2b192296ee1 100644 --- a/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php +++ b/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php @@ -55,7 +55,7 @@ public function serialize() */ public function unserialize($str) { - list($this->user, $parentData) = unserialize($str); + list($this->user, $parentData) = \is_array($str) ? $str : unserialize($str); parent::unserialize($parentData); } diff --git a/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php b/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php index 01a2ccf79997..4ea013913020 100644 --- a/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php +++ b/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php @@ -72,7 +72,7 @@ public function serialize() */ public function unserialize($str) { - list($parentData, $this->messageKey, $this->messageData) = unserialize($str); + list($parentData, $this->messageKey, $this->messageData) = \is_array($str) ? $str : unserialize($str); parent::unserialize($parentData); } diff --git a/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php b/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php index 6549307eee19..c36dda7cc368 100644 --- a/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php +++ b/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php @@ -65,7 +65,7 @@ public function serialize() */ public function unserialize($str) { - list($this->username, $parentData) = unserialize($str); + list($this->username, $parentData) = \is_array($str) ? $str : unserialize($str); parent::unserialize($parentData); } diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php index 783519469792..e8c851349387 100644 --- a/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php @@ -43,6 +43,9 @@ public function __construct($user, array $roles = []) $this->setUser($user); } + /** + * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method + */ public function serialize() { return serialize([$this->credentials, parent::serialize()]); diff --git a/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php b/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php index ca8549783fdb..f4318d89a545 100644 --- a/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php +++ b/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php @@ -76,7 +76,7 @@ public function getProviderKey() */ public function serialize() { - return serialize([$this->providerKey, parent::serialize()]); + return serialize([$this->providerKey, parent::serialize(true)]); } /** @@ -84,7 +84,7 @@ public function serialize() */ public function unserialize($serialized) { - list($this->providerKey, $parentStr) = unserialize($serialized); + list($this->providerKey, $parentStr) = \is_array($serialized) ? $serialized : unserialize($serialized); parent::unserialize($parentStr); } } From 10256fc4fdec3b668b2e046f9294b7b518a1ffe2 Mon Sep 17 00:00:00 2001 From: Renan Date: Mon, 28 Jan 2019 11:33:49 +0100 Subject: [PATCH 2/2] skip native serialize among child and parent serializable objects --- .../Authentication/Token/AbstractToken.php | 4 ---- .../Authentication/Token/AnonymousToken.php | 4 +++- .../Token/PreAuthenticatedToken.php | 2 -- .../Authentication/Token/RememberMeToken.php | 8 +++---- .../Token/UsernamePasswordToken.php | 4 +++- .../Core/Exception/AccountStatusException.php | 7 +++--- .../Exception/AuthenticationException.php | 24 ++++++++++++++++--- ...stomUserMessageAuthenticationException.php | 8 +++---- .../Exception/UsernameNotFoundException.php | 7 +++--- .../Token/AbstractTokenTest.php | 6 +++-- ...UserMessageAuthenticationExceptionTest.php | 15 ++++++++++++ .../Token/PostAuthenticationGuardToken.php | 4 +++- 12 files changed, 61 insertions(+), 32 deletions(-) diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php index a8a297cdb4ce..a65b2b417a75 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php @@ -134,10 +134,6 @@ public function eraseCredentials() /** * {@inheritdoc} - * - * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method - * - * @return string|array Returns an array when $isCalledFromOverridingMethod is set to true */ public function serialize() { diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php index 167c5a8e1cfb..1ee85363c492 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/AnonymousToken.php @@ -59,7 +59,9 @@ public function getSecret() */ public function serialize() { - return serialize([$this->secret, parent::serialize()]); + $serialized = [$this->secret, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php index c7d70bef245d..ddad0a539cd3 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/PreAuthenticatedToken.php @@ -76,8 +76,6 @@ public function eraseCredentials() /** * {@inheritdoc} - * - * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method */ public function serialize() { diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php index 8b8ec444fef3..031e4c6b98c7 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/RememberMeToken.php @@ -94,11 +94,9 @@ public function getCredentials() */ public function serialize() { - return serialize([ - $this->secret, - $this->providerKey, - parent::serialize(), - ]); + $serialized = [$this->secret, $this->providerKey, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php index 0596f0c8a7c8..3c052722bee3 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php +++ b/src/Symfony/Component/Security/Core/Authentication/Token/UsernamePasswordToken.php @@ -91,7 +91,9 @@ public function eraseCredentials() */ public function serialize() { - return serialize([$this->credentials, $this->providerKey, parent::serialize()]); + $serialized = [$this->credentials, $this->providerKey, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php b/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php index d2b192296ee1..cb91b42364bc 100644 --- a/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php +++ b/src/Symfony/Component/Security/Core/Exception/AccountStatusException.php @@ -44,10 +44,9 @@ public function setUser(UserInterface $user) */ public function serialize() { - return serialize([ - $this->user, - parent::serialize(), - ]); + $serialized = [$this->user, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Exception/AuthenticationException.php b/src/Symfony/Component/Security/Core/Exception/AuthenticationException.php index e0d1e471e8f9..2c6c7344a467 100644 --- a/src/Symfony/Component/Security/Core/Exception/AuthenticationException.php +++ b/src/Symfony/Component/Security/Core/Exception/AuthenticationException.php @@ -38,15 +38,33 @@ public function setToken(TokenInterface $token) $this->token = $token; } + /** + * {@inheritdoc} + */ public function serialize() { - return serialize([ + $serialized = [ $this->token, $this->code, $this->message, $this->file, $this->line, - ]); + ]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); + } + + /** + * @internal + */ + protected function doSerialize($serialized, $isCalledFromOverridingMethod) + { + if (null === $isCalledFromOverridingMethod) { + $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT, 3); + $isCalledFromOverridingMethod = isset($trace[2]['function'], $trace[2]['object']) && 'serialize' === $trace[2]['function'] && $this === $trace[2]['object']; + } + + return $isCalledFromOverridingMethod ? $serialized : serialize($serialized); } public function unserialize($str) @@ -57,7 +75,7 @@ public function unserialize($str) $this->message, $this->file, $this->line - ) = unserialize($str); + ) = \is_array($str) ? $str : unserialize($str); } /** diff --git a/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php b/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php index 4ea013913020..1faf6d9d5341 100644 --- a/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php +++ b/src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php @@ -60,11 +60,9 @@ public function getMessageData() */ public function serialize() { - return serialize([ - parent::serialize(), - $this->messageKey, - $this->messageData, - ]); + return serialize([parent::serialize(true), $this->messageKey, $this->messageData]); + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php b/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php index c36dda7cc368..b4b8047f341f 100644 --- a/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php +++ b/src/Symfony/Component/Security/Core/Exception/UsernameNotFoundException.php @@ -54,10 +54,9 @@ public function setUsername($username) */ public function serialize() { - return serialize([ - $this->username, - parent::serialize(), - ]); + $serialized = [$this->username, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /** diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php index e8c851349387..8280366d4af6 100644 --- a/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php @@ -44,11 +44,13 @@ public function __construct($user, array $roles = []) } /** - * @param bool $isCalledFromOverridingMethod Must be set to true when called from an overriding method + * {@inheritdoc} */ public function serialize() { - return serialize([$this->credentials, parent::serialize()]); + $serialized = [$this->credentials, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } public function unserialize($serialized) diff --git a/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php b/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php index ebf94384c0da..d69d3bffcb54 100644 --- a/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Security\Core\Tests\Exception; use PHPUnit\Framework\TestCase; +use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken; use Symfony\Component\Security\Core\Exception\CustomUserMessageAuthenticationException; class CustomUserMessageAuthenticationExceptionTest extends TestCase @@ -24,4 +25,18 @@ public function testConstructWithSAfeMessage() $this->assertEquals(['foo' => true], $e->getMessageData()); $this->assertEquals('SAFE MESSAGE', $e->getMessage()); } + + public function testSharedSerializedData() + { + $token = new AnonymousToken('foo', 'bar'); + + $exception = new CustomUserMessageAuthenticationException(); + $exception->setToken($token); + $exception->setSafeMessage('message', ['token' => $token]); + + $processed = unserialize(serialize($exception)); + $this->assertEquals($token, $processed->getToken()); + $this->assertEquals($token, $processed->getMessageData()['token']); + $this->assertSame($processed->getToken(), $processed->getMessageData()['token']); + } } diff --git a/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php b/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php index f4318d89a545..17e01ebf8dd0 100644 --- a/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php +++ b/src/Symfony/Component/Security/Guard/Token/PostAuthenticationGuardToken.php @@ -76,7 +76,9 @@ public function getProviderKey() */ public function serialize() { - return serialize([$this->providerKey, parent::serialize(true)]); + $serialized = [$this->providerKey, parent::serialize(true)]; + + return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null); } /**