From 2a4f4e262ff65a4cad900a56f785bde4fd66810b Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 25 May 2016 09:55:24 +0200 Subject: [PATCH 1/9] Add configuration option used to mitigate clock skew --- DependencyInjection/Security/Factory/Factory.php | 9 ++++++--- Security/Core/Authentication/Provider/Provider.php | 12 ++++++++---- .../Security/Factory/FactoryTest.php | 4 +++- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/DependencyInjection/Security/Factory/Factory.php b/DependencyInjection/Security/Factory/Factory.php index 00fc5c4..f87e38b 100644 --- a/DependencyInjection/Security/Factory/Factory.php +++ b/DependencyInjection/Security/Factory/Factory.php @@ -54,7 +54,8 @@ public function create(ContainerBuilder $container, $id, $config, $userProviderI ->replaceArgument(3, new Reference($this->encoderId)) ->replaceArgument(4, new Reference($this->nonceCacheId)) ->replaceArgument(5, $config['lifetime']) - ->replaceArgument(6, $config['date_format']); + ->replaceArgument(6, $config['date_format']) + ->replaceArgument(7, $config['clock_skew']); $entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint); @@ -98,8 +99,10 @@ public function addConfiguration(NodeDefinition $node) ->scalarNode('lifetime')->defaultValue(300)->end() ->scalarNode('date_format')->defaultValue( '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/' - )->end() - ->arrayNode('encoder') + ) + ->scalarNode('clock_skew')->defaultValue(60)->end() + ->end() + ->arrayNode('encoder') ->children() ->scalarNode('algorithm')->end() ->scalarNode('encodeHashAsBase64')->end() diff --git a/Security/Core/Authentication/Provider/Provider.php b/Security/Core/Authentication/Provider/Provider.php index c066ffe..4e49612 100644 --- a/Security/Core/Authentication/Provider/Provider.php +++ b/Security/Core/Authentication/Provider/Provider.php @@ -25,6 +25,7 @@ class Provider implements AuthenticationProviderInterface private $nonceCache; private $lifetime; private $dateFormat; + private $clockSkew; /** * Constructor. @@ -36,7 +37,8 @@ class Provider implements AuthenticationProviderInterface * @param Cache $nonceCache The nonce cache * @param int $lifetime The lifetime * @param string $dateFormat The date format - */ + * @param int $clockSkew The margin in seconds to mitigate clock skew + */ public function __construct( UserCheckerInterface $userChecker, UserProviderInterface $userProvider, @@ -44,7 +46,8 @@ public function __construct( PasswordEncoderInterface $encoder, Cache $nonceCache, $lifetime=300, - $dateFormat='/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/' + $dateFormat='/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/', + $clockSkew = 60 ) { if(empty($providerKey)) @@ -59,6 +62,7 @@ public function __construct( $this->nonceCache = $nonceCache; $this->lifetime = $lifetime; $this->dateFormat = $dateFormat; + $this->clockSkew = $clockSkew; } public function authenticate(TokenInterface $token) @@ -119,7 +123,7 @@ protected function validateDigest($digest, $nonce, $created, $secret, $salt) } //expire timestamp after specified lifetime - if(strtotime($this->getCurrentTime()) - strtotime($created) > $this->lifetime) + if(strtotime($this->getCurrentTime()) - strtotime($created) > ($this->lifetime - $this->clockSkew)) { throw new CredentialsExpiredException('Token has expired.'); } @@ -158,7 +162,7 @@ protected function getCurrentTime() protected function isTokenFromFuture($created) { - return strtotime($created) > strtotime($this->getCurrentTime()); + return strtotime($created) - $this->clockSkew > strtotime($this->getCurrentTime()); } protected function isFormattedCorrectly($created) diff --git a/Tests/DependencyInjection/Security/Factory/FactoryTest.php b/Tests/DependencyInjection/Security/Factory/FactoryTest.php index d7e73c3..3b950b1 100644 --- a/Tests/DependencyInjection/Security/Factory/FactoryTest.php +++ b/Tests/DependencyInjection/Security/Factory/FactoryTest.php @@ -46,6 +46,7 @@ public function testCreate() $profile = 'someprofile'; $lifetime = 300; $date_format = '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'; + $clock_skew = 60; $algorithm = 'sha1'; $encodeHashAsBase64 = true; @@ -68,7 +69,8 @@ public function testCreate() 'profile' => $profile, 'encoder' => $encoder, 'lifetime' => $lifetime, - 'date_format' => $date_format + 'date_format' => $date_format, + 'clock_skew' => $clock_skew ), 'user_provider', 'entry_point' From d544bfd1fd5f84465340088da197483242b0c7f6 Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 25 May 2016 11:21:36 +0200 Subject: [PATCH 2/9] Fixed lost ->end() call in Factory.php --- DependencyInjection/Security/Factory/Factory.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/DependencyInjection/Security/Factory/Factory.php b/DependencyInjection/Security/Factory/Factory.php index f87e38b..90792d2 100644 --- a/DependencyInjection/Security/Factory/Factory.php +++ b/DependencyInjection/Security/Factory/Factory.php @@ -99,9 +99,8 @@ public function addConfiguration(NodeDefinition $node) ->scalarNode('lifetime')->defaultValue(300)->end() ->scalarNode('date_format')->defaultValue( '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/' - ) + )->end() ->scalarNode('clock_skew')->defaultValue(60)->end() - ->end() ->arrayNode('encoder') ->children() ->scalarNode('algorithm')->end() From 846769623bcb364b54da2123d02bb2ec65707062 Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 25 May 2016 11:24:18 +0200 Subject: [PATCH 3/9] commit forgotten service.yml update, default clock_skew to 0 --- Resources/config/services.yml | 10 +++++++++- Security/Core/Authentication/Provider/Provider.php | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/Resources/config/services.yml b/Resources/config/services.yml index 1377769..3e16a63 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -1,7 +1,15 @@ services: escape_wsse_authentication.provider: class: %escape_wsse_authentication.provider.class% - arguments: ['@security.user_checker', null, null, null, null, 300, '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'] + arguments: + - '@security.user_checker' + - null + - null + - null + - null + - 300 + - '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'] + - 0 escape_wsse_authentication.listener: class: %escape_wsse_authentication.listener.class% diff --git a/Security/Core/Authentication/Provider/Provider.php b/Security/Core/Authentication/Provider/Provider.php index 4e49612..d0559fc 100644 --- a/Security/Core/Authentication/Provider/Provider.php +++ b/Security/Core/Authentication/Provider/Provider.php @@ -47,7 +47,7 @@ public function __construct( Cache $nonceCache, $lifetime=300, $dateFormat='/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/', - $clockSkew = 60 + $clockSkew = 0 ) { if(empty($providerKey)) From 756d38f4795b20eee3fec3feeba8ab4d48ad07f2 Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 25 May 2016 11:25:25 +0200 Subject: [PATCH 4/9] Fixed typo is services.yml --- Resources/config/services.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Resources/config/services.yml b/Resources/config/services.yml index 3e16a63..599d063 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -8,7 +8,7 @@ services: - null - null - 300 - - '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'] + - '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/' - 0 escape_wsse_authentication.listener: From 323f8104028b0aa2f32ae4a5640307f8beb6f4dd Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Fri, 17 Jun 2016 16:34:58 +0200 Subject: [PATCH 5/9] Added clock_skew argument to FactoryTest when comparing created objects. --- Tests/DependencyInjection/Security/Factory/FactoryTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Tests/DependencyInjection/Security/Factory/FactoryTest.php b/Tests/DependencyInjection/Security/Factory/FactoryTest.php index 3b950b1..d2f1e6e 100644 --- a/Tests/DependencyInjection/Security/Factory/FactoryTest.php +++ b/Tests/DependencyInjection/Security/Factory/FactoryTest.php @@ -110,7 +110,8 @@ public function testCreate() 'index_3' => new Reference($encoderId), 'index_4' => new Reference($nonceCacheId), 'index_5' => $lifetime, - 'index_6' => $date_format + 'index_6' => $date_format, + 'index_7' => $clock_skew ), $definition->getArguments() ); From 0b1c36e8151049aba7beeaa27045dd19ca30d65d Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Fri, 17 Jun 2016 23:08:17 +0200 Subject: [PATCH 6/9] Thanks to sagikazarmark for pointing out that substracting the clockskew from lifetime fixes nothing for out of sync clients and only shortness the lifetime for correctly synced clocks. --- Security/Core/Authentication/Provider/Provider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Security/Core/Authentication/Provider/Provider.php b/Security/Core/Authentication/Provider/Provider.php index d0559fc..7076dd4 100644 --- a/Security/Core/Authentication/Provider/Provider.php +++ b/Security/Core/Authentication/Provider/Provider.php @@ -123,7 +123,7 @@ protected function validateDigest($digest, $nonce, $created, $secret, $salt) } //expire timestamp after specified lifetime - if(strtotime($this->getCurrentTime()) - strtotime($created) > ($this->lifetime - $this->clockSkew)) + if(strtotime($this->getCurrentTime()) - strtotime($created) > $this->lifetime) { throw new CredentialsExpiredException('Token has expired.'); } From 0611766d78fbe83074ab954521fdb85f677db492 Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 19 Oct 2016 19:11:33 +0200 Subject: [PATCH 7/9] Set the default clock skew setting to 0 so default behaviour remains the same --- DependencyInjection/Security/Factory/Factory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DependencyInjection/Security/Factory/Factory.php b/DependencyInjection/Security/Factory/Factory.php index 90792d2..4b1bae5 100644 --- a/DependencyInjection/Security/Factory/Factory.php +++ b/DependencyInjection/Security/Factory/Factory.php @@ -100,7 +100,7 @@ public function addConfiguration(NodeDefinition $node) ->scalarNode('date_format')->defaultValue( '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/' )->end() - ->scalarNode('clock_skew')->defaultValue(60)->end() + ->scalarNode('clock_skew')->defaultValue(0)->end() ->arrayNode('encoder') ->children() ->scalarNode('algorithm')->end() From 7be6b9edb75aadbd23d391f36314813114c9cfb2 Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 19 Oct 2016 19:53:10 +0200 Subject: [PATCH 8/9] Removed ^M's that got in the last commit somehow --- Resources/config/services.yml | 44 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/Resources/config/services.yml b/Resources/config/services.yml index 4cdb091..c74ef26 100644 --- a/Resources/config/services.yml +++ b/Resources/config/services.yml @@ -1,22 +1,22 @@ -services: - escape_wsse_authentication.provider: - class: '%escape_wsse_authentication.provider.class%' - arguments: ['@security.user_checker', null, null, null, null, 300, '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/',0] - - escape_wsse_authentication.listener: - class: '%escape_wsse_authentication.listener.class%' - arguments: - - null # replaced by @security.token_storage for SF >= 2.6, @security.context otherwise - - '@security.authentication.manager' - - escape_wsse_authentication.entry_point: - class: '%escape_wsse_authentication.entry_point.class%' - arguments: ['@logger', null, 'UsernameToken'] - - escape_wsse_authentication.encoder: - class: '%escape_wsse_authentication.encoder.class%' - arguments: ['sha1', true, 1] - - escape_wsse_authentication.nonce_cache: - class: '%escape_wsse_authentication.nonce_cache.class%' - arguments: ['%kernel.cache_dir%/security/nonces'] +services: + escape_wsse_authentication.provider: + class: '%escape_wsse_authentication.provider.class%' + arguments: ['@security.user_checker', null, null, null, null, 300, '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/',0] + + escape_wsse_authentication.listener: + class: '%escape_wsse_authentication.listener.class%' + arguments: + - null # replaced by @security.token_storage for SF >= 2.6, @security.context otherwise + - '@security.authentication.manager' + + escape_wsse_authentication.entry_point: + class: '%escape_wsse_authentication.entry_point.class%' + arguments: ['@logger', null, 'UsernameToken'] + + escape_wsse_authentication.encoder: + class: '%escape_wsse_authentication.encoder.class%' + arguments: ['sha1', true, 1] + + escape_wsse_authentication.nonce_cache: + class: '%escape_wsse_authentication.nonce_cache.class%' + arguments: ['%kernel.cache_dir%/security/nonces'] From a7c39f14bffcf7f0e94a103e1dc5371e75eaff1a Mon Sep 17 00:00:00 2001 From: Martijn Lina Date: Wed, 19 Oct 2016 19:56:34 +0200 Subject: [PATCH 9/9] Set clockSkew=0 in the original FactoryTest --- Tests/DependencyInjection/Security/Factory/FactoryTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/DependencyInjection/Security/Factory/FactoryTest.php b/Tests/DependencyInjection/Security/Factory/FactoryTest.php index d2f1e6e..a63adf3 100644 --- a/Tests/DependencyInjection/Security/Factory/FactoryTest.php +++ b/Tests/DependencyInjection/Security/Factory/FactoryTest.php @@ -46,7 +46,7 @@ public function testCreate() $profile = 'someprofile'; $lifetime = 300; $date_format = '/^([\+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24\:?00)([\.,]\d+(?!:))?)?(\17[0-5]\d([\.,]\d+)?)?([zZ]|([\+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/'; - $clock_skew = 60; + $clock_skew = 0; $algorithm = 'sha1'; $encodeHashAsBase64 = true;