Validate AuthState before processing it.

thijskh · thijskh · commit ce01a59037b9 · 2023-03-01T14:37:00.000+01:00
We accept AuthState via untrusted URL parameters. As a defense in depth measure, validate that it conforms to our expected format before we output it again in HTML, feed it to a database, use the URL or do any further processing with it. Closes: #1706
diff --git a/modules/core/src/Controller/Login.php b/modules/core/src/Controller/Login.php
@@ -110,6 +110,7 @@ public function loginuserpass(Request $request): Template
             throw new Error\BadRequest('Missing AuthState parameter.');
         }
         $authStateId = $request->query->get('AuthState');
+        $this->authState::validateStateId($authStateId);
 
         $state = $this->authState::loadState($authStateId, UserPassBase::STAGEID);
 
@@ -137,6 +138,7 @@ private function handleLogin(Request $request, $source, array $state): Template
     {
         Assert::isInstanceOfAny($source, [UserPassBase::class, UserPassOrgBase::class]);
         $authStateId = $request->query->get('AuthState');
+        $this->authState::validateStateId($authStateId);
 
         $organizations = $organization = null;
         if ($source instanceof UserPassOrgBase) {
@@ -325,6 +327,7 @@ public function loginuserpassorg(Request $request): Template
             throw new Error\BadRequest('Missing AuthState parameter.');
         }
         $authStateId = $request->query->get('AuthState');
+        $this->authState::validateStateId($authStateId);
 
         $state = $this->authState::loadState($authStateId, UserPassOrgBase::STAGEID);
 
diff --git a/src/SimpleSAML/Auth/State.php b/src/SimpleSAML/Auth/State.php
@@ -4,13 +4,17 @@
 
 namespace SimpleSAML\Auth;
 
+use Exception;
 use SimpleSAML\Assert\Assert;
 use SimpleSAML\Configuration;
 use SimpleSAML\Error;
 use SimpleSAML\Logger;
 use SimpleSAML\Session;
 use SimpleSAML\Utils;
 
+use function filter_var;
+use function preg_match;
+
 /**
  * This is a helper class for saving and loading state information.
  *
@@ -171,6 +175,22 @@ public static function getStateId(array &$state, bool $rawId = false): string
         return $id . ':' . $state[self::RESTART];
     }
 
+    /**
+     * Perform syntactic validation of an incoming state ID.
+     *
+     * @throws \Exception If the syntax of the supplied state ID is unexpected.
+     */
+    public static function validateStateId(string $stateId): void
+    {
+        $parts = explode(':', $stateId, 2);
+
+        if (!preg_match('/^_[0-9a-f]+$/', $parts[0])) {
+            throw new Exception("Invalid AuthState ID syntax: " . $parts[0]);
+        }
+        if (!empty($parts[1]) && filter_var($parts[1], FILTER_VALIDATE_URL) === false) {
+            throw new Exception("Invalid AuthState return URL syntax: " . $parts[1]);
+        }
+    }
 
     /**
      * Retrieve state timeout.
@@ -303,7 +323,7 @@ public static function loadState(string $id, string $stage, bool $allowMissing =
             Logger::warning($msg);
 
             if ($sid['url'] === null) {
-                throw new \Exception($msg);
+                throw new Exception($msg);
             }
 
             $httpUtils->redirectUntrustedURL($sid['url']);
@@ -368,7 +388,7 @@ public static function throwException(array $state, Error\Exception $exception):
              */
             throw $exception;
         }
-        throw new \Exception(); // This should never happen
+        throw new Exception(); // This should never happen
     }
 
 
diff --git a/tests/src/SimpleSAML/Auth/StateTest.php b/tests/src/SimpleSAML/Auth/StateTest.php
@@ -4,6 +4,7 @@
 
 namespace SimpleSAML\Test\Auth;
 
+use Exception;
 use PHPUnit\Framework\TestCase;
 use SimpleSAML\Auth;
 
@@ -83,4 +84,36 @@ public function testGetPersistentAuthData(): void
             'Some error occurred with additional, persistent parameters, and no mandatory ones'
         );
     }
+
+
+    public function testValidateStateIdSimple(): void
+    {
+        Auth\State::validateStateId('_aaabb');
+        Auth\State::validateStateId('_aad12abb');
+        Auth\State::validateStateId('_6938b6453edfcb8c68055555d22c346857d6cd55fa');
+        $this->assertTrue(true);
+    }
+
+
+    public function testValidateStateIdWithReturnURL(): void
+    {
+        Auth\State::validateStateId('_aaabb:https://loeki.tv/wp-login.php?example=testsomething&urn=urn:example:org');
+        $this->assertTrue(true);
+    }
+
+
+    public function testValidateStateIdSimpleInvalid(): void
+    {
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Invalid AuthState ID syntax');
+        Auth\State::validateStateId('aaabb');
+    }
+
+
+    public function testValidateStateIdWithReturnURLWhitespaceInvalid(): void
+    {
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Invalid AuthState return URL syntax');
+        Auth\State::validateStateId("_aaabb:http://loeki.tv/\nnot-allowed");
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -110,6 +110,7 @@ public function loginuserpass(Request $request): Template`
`110`	`110`	`throw new Error\BadRequest('Missing AuthState parameter.');`
`111`	`111`	`}`
`112`	`112`	`$authStateId = $request->query->get('AuthState');`
	`113`	`+ $this->authState::validateStateId($authStateId);`
`113`	`114`
`114`	`115`	`$state = $this->authState::loadState($authStateId, UserPassBase::STAGEID);`
`115`	`116`
`@@ -137,6 +138,7 @@ private function handleLogin(Request $request, $source, array $state): Template`
`137`	`138`	`{`
`138`	`139`	`Assert::isInstanceOfAny($source, [UserPassBase::class, UserPassOrgBase::class]);`
`139`	`140`	`$authStateId = $request->query->get('AuthState');`
	`141`	`+ $this->authState::validateStateId($authStateId);`
`140`	`142`
`141`	`143`	`$organizations = $organization = null;`
`142`	`144`	`if ($source instanceof UserPassOrgBase) {`
`@@ -325,6 +327,7 @@ public function loginuserpassorg(Request $request): Template`
`325`	`327`	`throw new Error\BadRequest('Missing AuthState parameter.');`
`326`	`328`	`}`
`327`	`329`	`$authStateId = $request->query->get('AuthState');`
	`330`	`+ $this->authState::validateStateId($authStateId);`
`328`	`331`
`329`	`332`	`$state = $this->authState::loadState($authStateId, UserPassOrgBase::STAGEID);`
`330`	`333`