From f4c90f5d7a3e0687411e77f7e42ba791b4345d42 Mon Sep 17 00:00:00 2001 From: Jared Dreyer Date: Wed, 28 Feb 2024 12:04:52 +1300 Subject: [PATCH] Fix BackURL redirect with strict or lax session cookie security. See #55 PR for more context. This is merely a cherry pick / copy of that commit but for the 2 branch so in theory should work in SS 4.x --- src/Control/SAMLController.php | 10 ++++++++++ src/Helpers/SAMLHelper.php | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Control/SAMLController.php b/src/Control/SAMLController.php index e8ca519..8222f4f 100644 --- a/src/Control/SAMLController.php +++ b/src/Control/SAMLController.php @@ -260,6 +260,16 @@ protected function getRedirect() return $this->redirect($this->getRequest()->getSession()->get('BackURL')); } + // In SAMLHelper, we use RelayState to convey BackURL because in a HTTP POST flow + // with lax or strict cookie security the session will not be there for us. RelayState + // will be reflected back in the acs POST request. + // Note if only assertion is signed, RelayState cannot be trusted. Prevent open relay + // as in https://github.com/SAML-Toolkits/php-saml#avoiding-open-redirect-attacks + $relayState = $this->owner->getRequest()->postVar('RelayState'); + if ($relayState && Director::is_site_url($relayState)) { + return $this->redirect($relayState); + } + // Spoofing attack, redirect to homepage instead of spoofing url if ($this->getRequest()->getSession()->get('BackURL') && !Director::is_site_url($this->getRequest()->getSession()->get('BackURL'))) { diff --git a/src/Helpers/SAMLHelper.php b/src/Helpers/SAMLHelper.php index 0249e7b..28ddaae 100644 --- a/src/Helpers/SAMLHelper.php +++ b/src/Helpers/SAMLHelper.php @@ -75,7 +75,8 @@ public function redirect(RequestHandler $requestHandler = null, HTTPRequest $req $additionalGetQueryParams = $this->getAdditionalGETQueryParameters(); try { - $auth->login(Director::absoluteBaseURL() . 'saml/', $additionalGetQueryParams); + /** Use RelayState to convey BackURL (will be handled in {@see SAMLController}). */ + $auth->login($backURL, $additionalGetQueryParams); } catch (Exception $e) { /** @var LoggerInterface $logger */ $logger = Injector::inst()->get(LoggerInterface::class);