From 7dc67360cc891741c1d4114b91c601ef6e662380 Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Wed, 22 Nov 2023 11:55:17 +0100
Subject: [PATCH] Stepup ACS: add check that returned NameID matches the one we
 requested.

This should always be the case, we request a LoA from Stepup for NameID A,
then we should receive a response that contains that same NameID A. This
is defense in depth, should at some point in the entire chain another
check fail and an adversary be able to switch or replay assertions,
this will guard against them being accepted by Engineblock as valid.
---
 .../Service/StepupAssertionConsumer.php       | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php
index 0f417cde0..084c790a0 100644
--- a/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php
+++ b/library/EngineBlock/Corto/Module/Service/StepupAssertionConsumer.php
@@ -126,14 +126,20 @@ public function serve($serviceName, Request $httpRequest)
 
         // Get active request
         $processStep = $this->_processingStateHelper->getStepByRequestId($receivedRequest->getId(), ProcessingStateHelperInterface::STEP_STEPUP);
-        $receivedResponse = $processStep->getResponse();
+        $originalReceivedResponse = $processStep->getResponse();
+
+        // Only non-error responses will have a NameID in them
+        if ($checkResponseSignature) {
+            $this->verifyReceivedNameID($originalReceivedResponse, $receivedResponse);
+        }
 
         $nextProcessStep = $this->_processingStateHelper->getStepByRequestId(
             $receivedRequest->getId(),
             ProcessingStateHelperInterface::STEP_CONSENT
         );
 
-        $this->_server->sendConsentAuthenticationRequest($receivedResponse, $receivedRequest, $nextProcessStep->getRole(), $this->getAuthenticationState());
+
+        $this->_server->sendConsentAuthenticationRequest($originalReceivedResponse, $receivedRequest, $nextProcessStep->getRole(), $this->getAuthenticationState());
 
         return;
     }
@@ -282,4 +288,20 @@ private function verifyReceivedLoa(
         }
         $log->info('Received a suitable LoA response from the stepup gateway.');
     }
+
+    /**
+     * The NameID that the assertion from Stepup reports back, must always match the one we
+     * requested Stepup for when sending the request to Stepup. As a defense in depth against
+     * any gaps elsewhere, we doublecheck that this indeed matches.
+     */
+    private function verifyReceivedNameID(
+        EngineBlock_Saml2_ResponseAnnotationDecorator $originalReceivedResponse,
+        EngineBlock_Saml2_ResponseAnnotationDecorator $stepupReceivedResponse
+    ): void {
+        if ($originalReceivedResponse->getNameID()->getValue() !== $stepupReceivedResponse->getNameID()->getValue()) {
+            throw new EngineBlock_Exception(
+                'Stepup authentication failed, the received NameID from Stepup does not match the one we sent out.'
+            );
+        }
+    }
 }