Skip to content

Commit

Permalink
Stepup ACS: add check that returned NameID matches the one we requested.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
thijskh authored and MKodde committed Sep 10, 2024
1 parent df18f20 commit 069c30d
Showing 1 changed file with 24 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,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;
}
Expand Down Expand Up @@ -309,4 +315,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.'
);
}
}
}

0 comments on commit 069c30d

Please sign in to comment.