CVE-2024-22400: URL Redirection to Untrusted Site ('Open Redirect') (CWE-601)
value: Nextcloud User Saml is an app for authenticating Nextcloud users using SAML. In affected versions users can be given a link to the Nextcloud server and end up on a uncontrolled thirdparty server. It is recommended that the User Saml app is upgraded to version 5.1.5, 5.2.5, or 6.0.1. There are no known workarounds for this issue.
CWE description: A web application accepts a user-controlled input that specifies a link to an external site, and uses that link in a Redirect. This simplifies phishing attacks.
CVSS v3 base score: 6.1
Published: 2024-01-18T20:15Z
Repository: https://github.com/nextcloud/user_saml
Language: PHP
Files changed: lib/Controller/SAMLController.php, tests/unit/Controller/SAMLControllerTest.php
Commit date: 2023-11-29 12:54:07 +0100
Commit message: refactor(Controller): read parameter only once

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Diff: diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php
index 8752af2..7cb6bdf 100644
--- a/lib/Controller/SAMLController.php
+++ b/lib/Controller/SAMLController.php
@@ -41,10 +41,11 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
 use OCP\IUserSession;
 use OCP\Security\ICrypto;
+use OCP\Security\ITrustedDomainHelper;
 use OCP\Server;
 use OneLogin\Saml2\Auth;
 use OneLogin\Saml2\Error;
 use OneLogin\Saml2\Settings;
 use OneLogin\Saml2\ValidationError;
@@ -75,10 +76,11 @@ class SAMLController extends Controller {
 	private $userData;
 	/**
 	 * @var ICrypto
 	 */
 	private $crypto;
+	private ITrustedDomainHelper $trustedDomainHelper;
 
 	public function __construct(
 		string $appName,
 		IRequest $request,
 		ISession $session,
@@ -89,11 +91,12 @@ class SAMLController extends Controller {
 		IURLGenerator $urlGenerator,
 		LoggerInterface $logger,
 		IL10N $l,
 		UserResolver $userResolver,
 		UserData $userData,
-		ICrypto $crypto
+		ICrypto $crypto,
+		ITrustedDomainHelper $trustedDomainHelper
 	) {
 		parent::__construct($appName, $request);
 		$this->session = $session;
 		$this->userSession = $userSession;
 		$this->samlSettings = $samlSettings;
@@ -103,10 +106,11 @@ class SAMLController extends Controller {
 		$this->logger = $logger;
 		$this->l = $l;
 		$this->userResolver = $userResolver;
 		$this->userData = $userData;
 		$this->crypto = $crypto;
+		$this->trustedDomainHelper = $trustedDomainHelper;
 	}
 
 	/**
 	 * @throws NoUserFoundException
 	 * @throws UserFilterViolationException
@@ -190,15 +194,21 @@ class SAMLController extends Controller {
 	 * @NoCSRFRequired
 	 *
 	 * @throws \Exception
 	 */
 	public function login(int $idp = 1): Http\RedirectResponse {
+		$originalUrl = (string)$this->request->getParam('originalUrl', '');
+		if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
+			$originalUrl = '';
+		}
+
 		$type = $this->config->getAppValue($this->appName, 'type');
 		switch ($type) {
 			case 'saml':
 				$auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp));
-				$returnUrl = $this->request->getParam('originalUrl', $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login'));
+
+				$returnUrl = $originalUrl ?: $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login');
 				$ssoUrl = $auth->login($returnUrl, [], false, false, true);
 				$response = new Http\RedirectResponse($ssoUrl);
 
 				// Small hack to make user_saml work with the loginflows
 				$flowData = [];
@@ -213,11 +223,11 @@ class SAMLController extends Controller {
 				}
 
 				// Pack data as JSON so we can properly extract it later
 				$data = json_encode([
 					'AuthNRequestID' => $auth->getLastRequestID(),
-					'OriginalUrl' => $this->request->getParam('originalUrl', ''),
+					'OriginalUrl' => $originalUrl,
 					'Idp' => $idp,
 					'flow' => $flowData,
 				]);
 
 				// Encrypt it
@@ -227,11 +237,11 @@ class SAMLController extends Controller {
 				$data = base64_encode($data);
 
 				$response->addCookie('saml_data', $data, null, 'None');
 				break;
 			case 'environment-variable':
-				$ssoUrl = $this->request->getParam('originalUrl', '');
+				$ssoUrl = $originalUrl;
 				if (empty($ssoUrl)) {
 					$ssoUrl = $this->urlGenerator->getAbsoluteURL('/');
 				}
 				$this->session->set('user_saml.samlUserData', $_SERVER);
 				try {
diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php
index 6fbe97f..70c1657 100644
--- a/tests/unit/Controller/SAMLControllerTest.php
+++ b/tests/unit/Controller/SAMLControllerTest.php
@@ -37,10 +37,11 @@ use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Security\ICrypto;
+use OCP\Security\ITrustedDomainHelper;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 class SAMLControllerTest extends TestCase {
@@ -68,10 +69,11 @@ class SAMLControllerTest extends TestCase {
 	private $l;
 	/** @var ICrypto|MockObject */
 	private $crypto;
 	/** @var SAMLController */
 	private $samlController;
+	private ITrustedDomainHelper|MockObject $trustedDomainController;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->request = $this->createMock(IRequest::class);
@@ -84,10 +86,11 @@ class SAMLControllerTest extends TestCase {
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->l = $this->createMock(IL10N::class);
 		$this->userResolver = $this->createMock(UserResolver::class);
 		$this->userData = $this->createMock(UserData::class);
 		$this->crypto = $this->createMock(ICrypto::class);
+		$this->trustedDomainController = $this->createMock(ITrustedDomainHelper::class);
 
 		$this->l->expects($this->any())->method('t')->willReturnCallback(
 			function ($param) {
 				return $param;
 			}
@@ -109,11 +112,12 @@ class SAMLControllerTest extends TestCase {
 			$this->urlGenerator,
 			$this->logger,
 			$this->l,
 			$this->userResolver,
 			$this->userData,
-			$this->crypto
+			$this->crypto,
+			$this->trustedDomainController
 		);
 	}
 
 	public function testLoginWithInvalidAppValue() {
 		$this->config

Vulnerable code: 		ICrypto $crypto
				$returnUrl = $this->request->getParam('originalUrl', $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login'));
					'OriginalUrl' => $this->request->getParam('originalUrl', ''),
				$ssoUrl = $this->request->getParam('originalUrl', '');
			$this->crypto
Fixed code: use OCP\Security\ITrustedDomainHelper;
	private ITrustedDomainHelper $trustedDomainHelper;
		ICrypto $crypto,
		ITrustedDomainHelper $trustedDomainHelper
		$this->trustedDomainHelper = $trustedDomainHelper;
		$originalUrl = (string)$this->request->getParam('originalUrl', '');
		if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
			$originalUrl = '';
		}


				$returnUrl = $originalUrl ?: $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.login');
					'OriginalUrl' => $originalUrl,
				$ssoUrl = $originalUrl;
use OCP\Security\ITrustedDomainHelper;
	private ITrustedDomainHelper|MockObject $trustedDomainController;
		$this->trustedDomainController = $this->createMock(ITrustedDomainHelper::class);
			$this->crypto,
			$this->trustedDomainController