diff --git a/library/EngineBlock/Corto/Module/Bindings.php b/library/EngineBlock/Corto/Module/Bindings.php index 66780d625..b52361ef4 100644 --- a/library/EngineBlock/Corto/Module/Bindings.php +++ b/library/EngineBlock/Corto/Module/Bindings.php @@ -49,6 +49,8 @@ class EngineBlock_Corto_Module_Bindings extends EngineBlock_Corto_Module_Abstrac const SAML_STATUS_MESSAGE_EMPTY = '(No message provided)'; + const OLD_ISSUE_INSTANT_THRESHOLD_SECONDS = 86400; // 24 hours + protected static $ASSERTION_SEQUENCE = array( 'saml:Issuer', 'ds:Signature', @@ -598,10 +600,22 @@ protected function _verifyKnownIdP($messageIssuer, $destination = '') */ protected function _checkIssueInstant($issueInstant, $type, $entityid) { - // check the IssueInstant against our own time to see if the SP's clock is getting out of sync - // Ssp has a hard-coded limit of 60 seconds; use 30 here to catch an IdP's drifting clock early $time = EngineBlock_ApplicationSingleton::getInstance()->getDiContainer()->getTimeProvider()->time(); $timeDelta = $time - $issueInstant; + + if ($type === 'SP' && $timeDelta > self::OLD_ISSUE_INSTANT_THRESHOLD_SECONDS) { + $this->_logger->warning( + sprintf( + 'IssueInstant of SAMLRequest from SP "%s" is %d seconds (more than 24 hours) in the past; possible replay or severely out-of-sync SP clock', + $entityid, + $timeDelta + ) + ); + return; + } + + // check the IssueInstant against our own time to see if the SP's clock is getting out of sync + // Ssp has a hard-coded limit of 60 seconds; use 30 here to catch an IdP's drifting clock early if (abs($timeDelta) > 30) { $this->_logger->notice( sprintf( diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php index 5b8ef858c..5cc885034 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -238,4 +238,56 @@ public function testAzureDomainHintWhrParamIsCorrectlyUrlEncoded() $this->assertArrayHasKey('whr', $params, 'whr query parameter must be present'); $this->assertSame('example.nl', $params['whr']); } + + public function testOldSpIssueInstantLogsWarningOnly() + { + $logger = $this->mockLogger(); + $logger->shouldReceive('warning')->once()->with(m::pattern('/is \d+ seconds \(more than 24 hours\) in the past/')); + $logger->shouldNotReceive('notice'); + + $this->checkIssueInstant(time() - 90000, 'SP', 'https://sp.example.edu'); + } + + public function testRecentSpIssueInstantLogsNoticeOnly() + { + $logger = $this->mockLogger(); + $logger->shouldReceive('notice')->once()->with(m::pattern('/clock synchronization issues/')); + $logger->shouldNotReceive('warning'); + + $this->checkIssueInstant(time() - 600, 'SP', 'https://sp.example.edu'); + } + + public function testOldIdpIssueInstantLogsGenericNoticeNotWarning() + { + $logger = $this->mockLogger(); + $logger->shouldReceive('notice')->once()->with(m::pattern('/clock synchronization issues/')); + $logger->shouldNotReceive('warning'); + + $this->checkIssueInstant(time() - 90000, 'IdP', 'https://idp.example.edu'); + } + + public function testFreshSpIssueInstantLogsNothing() + { + $logger = $this->mockLogger(); + $logger->shouldNotReceive('warning'); + $logger->shouldNotReceive('notice'); + + $this->checkIssueInstant(time(), 'SP', 'https://sp.example.edu'); + } + + private function mockLogger() + { + $logger = m::mock(); + + $loggerProperty = new ReflectionProperty(EngineBlock_Corto_Module_Bindings::class, '_logger'); + $loggerProperty->setValue($this->bindings, $logger); + + return $logger; + } + + private function checkIssueInstant($issueInstant, $type, $entityId) + { + $method = new ReflectionMethod(EngineBlock_Corto_Module_Bindings::class, '_checkIssueInstant'); + $method->invoke($this->bindings, $issueInstant, $type, $entityId); + } }