From 552738944a62f981fd95f59e0ee65479a0952f55 Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Wed, 16 Aug 2023 09:15:12 +0200 Subject: [PATCH] Validate the cookie value authentication time The cookie lifetime can be extended by the user. But the explicit lifetime of the cookie was not yet validated by the cookie service. Meaning you could extend the SSO lifetime by keeping your cookie alive. As of this change, the authentication lifetime is checked against the current timestamp togegher with the cookie expiration time. See: https://www.pivotaltracker.com/story/show/183402734 -> the SSO cookie's timestamp is within the configured SSO lifetime --- docs/SsoOn2Fa.md | 5 +- .../Resources/config/services.yml | 9 +- .../GatewayBundle/Sso2fa/CookieService.php | 27 ++++- .../Sso2fa/DateTime/ExpirationHelper.php | 78 +++++++++++++ .../DateTime/ExpirationHelperInterface.php | 36 ++++++ .../InvalidAuthenticationTimeException.php | 26 +++++ .../Sso2fa/ValueObject/CookieValue.php | 11 +- .../ValueObject/CookieValueInterface.php | 2 + .../Sso2fa/ValueObject/NullCookieValue.php | 5 + .../Tests/Sso2fa/CookieServiceTest.php | 21 +++- .../Sso2fa/DateTime/ExpirationHelperTest.php | 104 ++++++++++++++++++ 11 files changed, 314 insertions(+), 10 deletions(-) create mode 100644 src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelper.php create mode 100644 src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelperInterface.php create mode 100644 src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/Exception/InvalidAuthenticationTimeException.php create mode 100644 src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/DateTime/ExpirationHelperTest.php diff --git a/docs/SsoOn2Fa.md b/docs/SsoOn2Fa.md index 65dd2943..ac68fb2d 100644 --- a/docs/SsoOn2Fa.md +++ b/docs/SsoOn2Fa.md @@ -14,7 +14,10 @@ The cookie contains several values, used to ascertain if SSO can be given. These | `LoA` | The LoA of the second factor | | `Timestamp` | The timestamp taken during authentication. | -The cookie is used to verify the SSO is issued to the correct identity (user). And to check if the LoA requirement is satisfied by the SSO cookie. The timestamp is kept mainly for audit reasons. +The cookie is used to verify the SSO is issued to the correct identity (user). And to check if the LoA requirement is satisfied by the SSO cookie. +The timestamp is used to verify if the cookie value did not expire. This is determined by adding the cookie expiration time (configured in `sso_cookie_lifetime` param) +to the authentication timestamp found in the cookie. If those two exceed the current timestamp, the cookie is considered to be expired. Even tho the browser cookie itself +might still be alive. The cookie value contains sensitive data, and its contents are authenticated and encrypted for that reason. We use the Paragonie Halite library for this. Halite uses XSalsa20 for encryption and BLAKE2b for message Authentication (MAC). diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Resources/config/services.yml b/src/Surfnet/StepupGateway/GatewayBundle/Resources/config/services.yml index b51e4dc0..9dae0dfa 100644 --- a/src/Surfnet/StepupGateway/GatewayBundle/Resources/config/services.yml +++ b/src/Surfnet/StepupGateway/GatewayBundle/Resources/config/services.yml @@ -138,6 +138,11 @@ services: - "@gateway.service.sso_2fa_cookie_config" public: false + gateway.service.sso_2fa_expiration_helper: + class: Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelper + arguments: + $cookieLifetime: "%sso_cookie_lifetime%" + gateway.service.sso_2fa_cookie_helper: class: Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelper arguments: @@ -151,10 +156,10 @@ services: arguments: - "@gateway.service.sso_2fa_cookie_helper" - "@gateway.service.institution_configuration" - - "@logger" - "@gateway.service.second_factor_service" - "@surfnet_stepup.service.second_factor_type" - - "@gssp.allowed_sps" + - "@gateway.service.sso_2fa_expiration_helper" + - "@logger" gateway.service.institution_configuration: class: Surfnet\StepupGateway\GatewayBundle\Service\InstitutionConfigurationService diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/CookieService.php b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/CookieService.php index d24e6e90..8979eda9 100644 --- a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/CookieService.php +++ b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/CookieService.php @@ -27,7 +27,9 @@ use Surfnet\StepupGateway\GatewayBundle\Saml\ResponseContext; use Surfnet\StepupGateway\GatewayBundle\Service\InstitutionConfigurationService; use Surfnet\StepupGateway\GatewayBundle\Service\SecondFactorService; +use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelperInterface; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception\CookieNotFoundException; +use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Exception\InvalidAuthenticationTimeException; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelperInterface; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValue; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValueInterface; @@ -61,18 +63,24 @@ class CookieService implements CookieServiceInterface * @var SecondFactorTypeService */ private $secondFactorTypeService; + /** + * @var ExpirationHelperInterface + */ + private $expirationHelper; public function __construct( CookieHelperInterface $cookieHelper, InstitutionConfigurationService $institutionConfigurationService, - LoggerInterface $logger, SecondFactorService $secondFactorService, - SecondFactorTypeService $secondFactorTypeService + SecondFactorTypeService $secondFactorTypeService, + ExpirationHelperInterface $expirationHelper, + LoggerInterface $logger ) { $this->cookieHelper = $cookieHelper; $this->institutionConfigurationService = $institutionConfigurationService; $this->secondFactorService = $secondFactorService; $this->secondFactorTypeService = $secondFactorTypeService; + $this->expirationHelper = $expirationHelper; $this->logger = $logger; } @@ -101,7 +109,7 @@ public function handleSsoOn2faCookieStorage( if (!$secondFactor) { throw new RuntimeException(sprintf('Second Factor token not found with ID: %s', $secondFactorId)); } - // Test if the institution of the Idenity this SF belongs to has SSO on 2FA enabled + // Test if the institution of the Identity this SF belongs to has SSO on 2FA enabled $isEnabled = $this->institutionConfigurationService->ssoOn2faEnabled($secondFactor->institution); $this->logger->notice( sprintf( @@ -178,6 +186,19 @@ public function shouldSkip2faAuthentication( ); return false; } + try { + $isExpired = $this->expirationHelper->isExpired($ssoCookie); + if ($isExpired) { + $this->logger->notice( + 'The SSO on 2FA cookie has expired. Meaning [authentication time] + [cookie lifetime] is in the past' + ); + return false; + } + } catch (InvalidAuthenticationTimeException $e) { + $this->logger->notice('The SSO on 2FA cookie contained an invalid authentication time', [$e->getMessage()]); + return false; + } + /** @var SecondFactor $secondFactor */ foreach ($secondFactorCollection as $secondFactor) { $loa = $secondFactor->getLoaLevel($this->secondFactorTypeService); diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelper.php b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelper.php new file mode 100644 index 00000000..a0ec3f42 --- /dev/null +++ b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelper.php @@ -0,0 +1,78 @@ +cookieLifetime = $cookieLifetime; + if (!$now) { + $now = DateTime::now(); + } + $this->now = $now; + } + + public function isExpired(CookieValueInterface $cookieValue): bool + { + try { + $authenticationTimestamp = $cookieValue->authenticationTime(); + } catch (TypeError $error) { + throw new InvalidAuthenticationTimeException( + 'The authentication time contained a non-int value', + 0, + $error + ); + } + if ($authenticationTimestamp < 0) { + throw new InvalidAuthenticationTimeException( + 'The authentication time is from before the Unix timestamp epoch' + ); + } + if ($authenticationTimestamp > $this->now->getTimestamp()) { + throw new InvalidAuthenticationTimeException( + 'The authentication time is from the future, which indicates the clock settings ' . + 'are incorrect, or the time in the cookie value was tampered with.' + ); + } + + $expirationTimestamp = $authenticationTimestamp + $this->cookieLifetime; + $currentTimestamp = $this->now->getTimestamp(); + // Is the current time greater than the expiration time? + return $currentTimestamp > $expirationTimestamp; + } +} diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelperInterface.php b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelperInterface.php new file mode 100644 index 00000000..e6957399 --- /dev/null +++ b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/DateTime/ExpirationHelperInterface.php @@ -0,0 +1,36 @@ +tokenId = $secondFactorId; $cookieValue->identityId = $identityId; $cookieValue->loa = $loa; - $cookieValue->authenticationTime = DateTime::now()->format(DATE_ATOM); + $dateTime = new DateTime(); + $cookieValue->authenticationTime = $dateTime->format(DATE_ATOM); return $cookieValue; } @@ -86,4 +88,9 @@ public function issuedTo(string $identityNameId): bool { return strtolower($identityNameId) === strtolower($this->identityId); } + + public function authenticationTime(): int + { + return strtotime($this->authenticationTime); + } } diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/CookieValueInterface.php b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/CookieValueInterface.php index cd52aa47..83d0443e 100644 --- a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/CookieValueInterface.php +++ b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/CookieValueInterface.php @@ -25,4 +25,6 @@ public static function deserialize(string $serializedData): CookieValueInterface public function serialize(): string; public function meetsRequiredLoa(float $requiredLoa): bool; + + public function authenticationTime(): int; } diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/NullCookieValue.php b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/NullCookieValue.php index bb9d6fc8..94fcd0ca 100644 --- a/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/NullCookieValue.php +++ b/src/Surfnet/StepupGateway/GatewayBundle/Sso2fa/ValueObject/NullCookieValue.php @@ -34,4 +34,9 @@ public function meetsRequiredLoa(float $requiredLoa): bool { return false; } + + public function authenticationTime(): int + { + return -1; + } } diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/CookieServiceTest.php b/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/CookieServiceTest.php index ec017628..df015a62 100644 --- a/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/CookieServiceTest.php +++ b/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/CookieServiceTest.php @@ -35,6 +35,8 @@ use Surfnet\StepupGateway\GatewayBundle\Sso2fa\CookieService; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Crypto\CryptoHelper; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Crypto\HaliteCryptoHelper; +use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelper; +use Surfnet\StepupGateway\GatewayBundle\Sso2fa\DateTime\ExpirationHelperInterface; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\Http\CookieHelper; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\Configuration; use Surfnet\StepupGateway\GatewayBundle\Sso2fa\ValueObject\CookieValue; @@ -86,6 +88,11 @@ class CookieServiceTest extends TestCase */ private $encryptionHelper; + /** + * @var Mockery\LegacyMockInterface|Mockery\MockInterface|ExpirationHelperInterface|(ExpirationHelperInterface&Mockery\LegacyMockInterface)|(ExpirationHelperInterface&Mockery\MockInterface) + */ + private $expirationHelper; + protected function buildService(Configuration $configuration): void { // Not all dependencies are included for real, the ones not focussed on crypto and cookie storage are mocked @@ -95,13 +102,15 @@ protected function buildService(Configuration $configuration): void $this->secondFactorTypeService = Mockery::mock(SecondFactorTypeService::class); $this->configuration = $configuration; $this->encryptionHelper = new HaliteCryptoHelper($configuration); + $this->expirationHelper = Mockery::mock(ExpirationHelperInterface::class); $cookieHelper = new CookieHelper($this->configuration, $this->encryptionHelper, $logger); $this->service = new CookieService( $cookieHelper, $this->institutionService, - $logger, $this->secondFactorService, - $this->secondFactorTypeService + $this->secondFactorTypeService, + $this->expirationHelper, + $logger ); $this->responseContext = Mockery::mock(ResponseContext::class); @@ -349,6 +358,10 @@ public function test_skipping_authentication_succeeds() ->shouldReceive('saveSelectedSecondFactor') ->with($yubikey); + $this->expirationHelper + ->shouldReceive('isExpired') + ->andReturn(false); + self::assertTrue( $this->service->shouldSkip2faAuthentication( $this->responseContext, @@ -390,6 +403,10 @@ public function test_skipping_authentication_succeeds_selects_correct_token() ->shouldReceive('saveSelectedSecondFactor') ->with($yubikey); + $this->expirationHelper + ->shouldReceive('isExpired') + ->andReturn(false); + self::assertTrue( $this->service->shouldSkip2faAuthentication( $this->responseContext, diff --git a/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/DateTime/ExpirationHelperTest.php b/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/DateTime/ExpirationHelperTest.php new file mode 100644 index 00000000..d3fecde5 --- /dev/null +++ b/src/Surfnet/StepupGateway/GatewayBundle/Tests/Sso2fa/DateTime/ExpirationHelperTest.php @@ -0,0 +1,104 @@ +isExpired($cookieValue)); + } + + /** + * @dataProvider invalidTimeExpectations + */ + public function test_strange_authentication_time_values(ExpirationHelper $helper, CookieValue $cookieValue) + { + self::expectException(InvalidAuthenticationTimeException::class); + $helper->isExpired($cookieValue); + } + + public function expirationExpectations() + { + return [ + 'not expired' => [false, $this->makeExpirationHelper(3600, time()), $this->makeCookieValue(time())], + 'not expired but about to be' => [false, $this->makeExpirationHelper(3600, time() + 3600), $this->makeCookieValue(time())], + 'expired' => [true, $this->makeExpirationHelper(3600, time() + 3601), $this->makeCookieValue(time())], + 'expired more' => [true, $this->makeExpirationHelper(3600, time() + 36000), $this->makeCookieValue(time())], + ]; + } + + public function invalidTimeExpectations() + { + $goodOldHelper = $this->makeExpirationHelper(3600, time()); + return [ + 'before epoch' => [$goodOldHelper, $this->makeCookieValue(-1)], + 'from the future' => [$goodOldHelper, $this->makeCookieValue(time() + 42)], + 'invalid time input 1' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime('aint-no-time')], + 'invalid time input 2' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime('9999-01-01')], + 'invalid time input 3' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime('0001-01-01')], + 'invalid time input 4' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(-1.0)], + 'invalid time input 5' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(2.999)], + 'invalid time input 6' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(42)], + 'invalid time input 7' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(true)], + 'invalid time input 8' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(false)], + 'invalid time input 9' => [$goodOldHelper, $this->makeCookieValueUnrestrictedAuthTime(null)], + ]; + } + + private function makeExpirationHelper(int $expirationTime, int $now) : ExpirationHelper + { + $time = new \DateTime(); + $time->setTimestamp($now); + return new ExpirationHelper($expirationTime, $time); + } + + private function makeCookieValue(int $authenticationTime) : CookieValueInterface + { + $dateTime = new \DateTime(); + $dateTime->setTimestamp($authenticationTime); + $data = [ + 'tokenId' => 'tokenId', + 'identityId' => 'identityId', + 'loa' => 2.0, + 'authenticationTime' => $dateTime->format(DATE_ATOM), + ]; + return CookieValue::deserialize(json_encode($data)); + } + + private function makeCookieValueUnrestrictedAuthTime($authenticationTime) : CookieValueInterface + { + $data = [ + 'tokenId' => 'tokenId', + 'identityId' => 'identityId', + 'loa' => 2.0, + 'authenticationTime' => $authenticationTime, + ]; + return CookieValue::deserialize(json_encode($data)); + } +}