Merge commit from fork

* Prevent duplicate parameters from being provided

* Use strict see what (is/will) be signed verification for $message

* Fix parseQuery() Exception namspaces and add tests for them

* parseQuery may now throw an Exception

* Tie the results from parseQuery to those extracted from the SignedQuery string

This way both methods of arriving at the signed message must agree and complement each other

* Use same (===) instead of Eq (==)

* Fix coding style

---------

Co-authored-by: ahacker1 <alex@securesaml.com>
Co-authored-by: Pieter van der Meulen <pieter.vandermeulen@surf.nl>

Author: 3 people

src/SAML2/HTTPRedirect.php

@@ -94,7 +94,7 @@ public function send(Message $message) : void
     /**
      * Receive a SAML 2 message sent using the HTTP-Redirect binding.
      *
-     * Throws an exception if it is unable receive the message.
+     * Throws an exception if it is unable to receive the message.
      *
      * @throws \Exception
      * @return \SAML2\Message The received message.
@@ -104,10 +104,36 @@ public function send(Message $message) : void
     public function receive(): Message
     {
         $data = self::parseQuery();
-        if (array_key_exists('SAMLRequest', $data)) {
-            $message = $data['SAMLRequest'];
-        } elseif (array_key_exists('SAMLResponse', $data)) {
-            $message = $data['SAMLResponse'];
+        $signedQuery = $data['SignedQuery'];
+
+        /**
+         * Get the SAMLRequest/SAMLResponse from the exact same signed data that will be verified later in
+         * validateSignature into $res using the actual SignedQuery
+         */
+        $res = [];
+        foreach (explode('&', $signedQuery) as $e) {
+            $tmp = explode('=', $e, 2);
+            $name = $tmp[0];
+            if (count($tmp) === 2) {
+                $value = $tmp[1];
+            } else {
+                /* No value for this parameter. */
+                $value = '';
+            }
+            $name = urldecode($name);
+            $res[$name] = urldecode($value);
+        }
+
+        /**
+         * Put the SAMLRequest/SAMLResponse from the actual query string into $message,
+         * and assert that the result from parseQuery() in $data and the parsing of the SignedQuery in $res agree
+         */
+        if (array_key_exists('SAMLRequest', $res)) {
+            Assert::same($res['SAMLRequest'], $data['SAMLRequest'], 'Parse failure.');
+            $message = $res['SAMLRequest'];
+        } elseif (array_key_exists('SAMLResponse', $res)) {
+            Assert::same($res['SAMLResponse'], $data['SAMLResponse'], 'Parse failure.');
+            $message = $res['SAMLResponse'];
         } else {
             throw new \Exception('Missing SAMLRequest or SAMLResponse parameter.');
         }
@@ -157,7 +183,7 @@ public function receive(): Message
         $signData = [
             'Signature' => $data['Signature'],
             'SigAlg'    => $data['SigAlg'],
-            'Query'     => $data['SignedQuery'],
+            'Query'     => $signedQuery,
         ];
 
         $message->addValidator([get_class($this), 'validateSignature'], $signData);
@@ -174,6 +200,7 @@ public function receive(): Message
      * signed.
      *
      * @return array The query data that is signed.
+     * @throws \Exception
      */
     private static function parseQuery() : array
     {
@@ -195,7 +222,12 @@ private static function parseQuery() : array
                 /* No value for this parameter. */
                 $value = '';
             }
+
             $name = urldecode($name);
+            // Prevent keys from being set more than once
+            if (array_key_exists($name, $data)) {
+                throw new \Exception('Duplicate parameter.');
+            }
             $data[$name] = urldecode($value);
 
             switch ($name) {
@@ -211,6 +243,9 @@ private static function parseQuery() : array
                     break;
             }
         }
+        if (array_key_exists('SAMLRequest', $data) && array_key_exists('SAMLResponse', $data)) {
+                throw new \Exception('Both SAMLRequest and SAMLResponse provided.');
+        }
 
         $data['SignedQuery'] = $sigQuery.$relayState.$sigAlg;
 

tests/SAML2/HTTPRedirectTest.php

@@ -253,4 +253,37 @@ public function testSendAuthnResponseBespokeDestination() : void
         $hr->setDestination('gopher://myurl');
         $hr->send($response);
     }
+
+
+    /**
+     * Test that multiple query parameters with the same name are not allowed.
+     *
+     */
+    public function testDuplicateQueryParameters() : void
+    {
+        // SAMLRequest appears twice
+        $qs = 'SAMLRequest=first&RelayState=somestate&SAMLRequest=second&Signature=sig&&SigAlg=alg';
+        $_SERVER['QUERY_STRING'] = $qs;
+
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('Duplicate parameter.');
+        $hr = new HTTPRedirect();
+        $hr->receive();
+    }
+
+    /**
+     * Test that providing both SAMLRequest and SAMLResponse is not allowed
+     */
+    public function testSAMLResponseAndSAMLRequestConfusion() : void
+    {
+        // Both SAMLRequest and SAML Response appear
+        $qs = 'SAMLRequest=first&RelayState=somestate&SAMLResponse=second&Signature=sig&&SigAlg=alg';
+        $_SERVER['QUERY_STRING'] = $qs;
+
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('Both SAMLRequest and SAMLResponse provided.');
+        $hr = new HTTPRedirect();
+        $hr->receive();
+    }
+
 }