From 2327f4fdcc67934e9ba9a6c8920f108c3393b902 Mon Sep 17 00:00:00 2001 From: Thomas Nymand Date: Thu, 13 Aug 2026 12:41:12 +0200 Subject: [PATCH] REF-26: Validate the SAML signature profile before trusting an assertion signature validateSignature only called SignatureValidator.validate, a raw cryptographic check that the signature verifies against the IdP signing certificate from the configured metadata. It did not establish that the signature is bound to the assertion the SP goes on to consume, so a verifying signature was not on its own sufficient grounds for trusting the subject, attributes and NSIS level read from that assertion. Run SAMLSignatureProfileValidator before the cryptographic check. It requires exactly one reference, restricts the transforms to enveloped signature plus canonicalization, rejects Object children, and checks that the reference resolves to the element the signature is a child of, which is the binding that was missing. Trust is still anchored in the signing certificate from the configured IdP metadata. Also guard against a null signature up front: assertion.isSigned() was only checked at the end of validateAssertion and behind the profile validation flag, so an unsigned assertion reached SignatureValidator.validate(null, ...) and failed with an uncaught NullPointerException instead of a validation error. Signing is now enforced unconditionally in validateSignature, and that late, now unreachable check has been removed. Regression tests cover assertions where the signature reference does not resolve to the consumed element. They assert that the signature still verifies cryptographically, so they fail for the right reason and not because of a broken signature. --- .../AssertionValidationService.java | 23 +++- .../AssertionValidationServiceTest.java | 109 ++++++++++++++++++ 2 files changed, 126 insertions(+), 6 deletions(-) diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/service/validation/AssertionValidationService.java b/oiosaml/src/main/java/dk/gov/oio/saml/service/validation/AssertionValidationService.java index 75262b6..2a66897 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/service/validation/AssertionValidationService.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/service/validation/AssertionValidationService.java @@ -31,8 +31,10 @@ import org.opensaml.saml.saml2.core.Subject; import org.opensaml.saml.saml2.core.SubjectConfirmation; import org.opensaml.saml.saml2.core.SubjectConfirmationData; +import org.opensaml.saml.security.impl.SAMLSignatureProfileValidator; import org.opensaml.security.credential.UsageType; import org.opensaml.security.x509.BasicX509Credential; +import org.opensaml.xmlsec.signature.Signature; import org.opensaml.xmlsec.signature.support.SignatureException; import org.opensaml.xmlsec.signature.support.SignatureValidator; @@ -188,11 +190,6 @@ private void validateAssertion(Assertion assertion, AuthnRequestWrapper authnReq String nameIDValue = assertion.getSubject().getNameID().getValue(); validateAttributeStatement(attributeValues, nameIDValue.startsWith("https://data.gov.dk/model/core/eid/professional")); validateAssurance(attributeValues, authnRequest); - - // The Assertion within the response MUST be directly signed - if (!assertion.isSigned()) { - throw new AssertionValidationException("The Assertion within the response MUST be directly signed"); - } } private void validateAttributeStatement(Map attributes, boolean isProfessional) throws AssertionValidationException { @@ -278,13 +275,27 @@ private void validateInResponseTo(Response response, AuthnRequestWrapper authnRe } private void validateSignature(Assertion assertion) throws ExternalException, InternalException, AssertionValidationException { + // The assertion MUST be directly signed, an unsigned assertion has no signature to validate + Signature signature = assertion.getSignature(); + if (!assertion.isSigned() || signature == null) { + throw new AssertionValidationException("The Assertion within the response MUST be directly signed"); + } + + // Establishes that the signature is bound to this assertion. Validating it cryptographically only + // proves that some element in the document was signed with the IdP key. + try { + new SAMLSignatureProfileValidator().validate(signature); + } catch (SignatureException e) { + throw new AssertionValidationException("Assertion signature does not follow the SAML signature profile", e); + } + // Get Signing credential X509Certificate x509Certificate = IdPMetadataService.getInstance().getIdPMetadata().getValidX509Certificate(UsageType.SIGNING); BasicX509Credential credential = new BasicX509Credential(x509Certificate); // Validate Signature try { - SignatureValidator.validate(assertion.getSignature(), credential); + SignatureValidator.validate(signature, credential); } catch (SignatureException e) { throw new AssertionValidationException("Could not validate assertion signature", e); } diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/service/validation/AssertionValidationServiceTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/service/validation/AssertionValidationServiceTest.java index 070fa10..6d465eb 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/service/validation/AssertionValidationServiceTest.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/service/validation/AssertionValidationServiceTest.java @@ -4,12 +4,15 @@ import dk.gov.oio.saml.service.AssertionService; import dk.gov.oio.saml.service.AuthnRequestService; import dk.gov.oio.saml.service.BaseServiceTest; +import dk.gov.oio.saml.service.IdPMetadataService; import dk.gov.oio.saml.session.AuthnRequestWrapper; import dk.gov.oio.saml.util.ExternalException; import dk.gov.oio.saml.util.IdpUtil; import dk.gov.oio.saml.util.SamlHelper; import dk.gov.oio.saml.util.TestConstants; +import java.security.cert.X509Certificate; import java.util.List; +import java.util.UUID; import javax.servlet.http.HttpServletRequest; import org.joda.time.DateTime; import org.junit.jupiter.api.Assertions; @@ -17,9 +20,11 @@ import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.opensaml.core.config.InitializationException; +import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.messaging.context.MessageContext; import org.opensaml.saml.common.SAMLObject; import org.opensaml.saml.common.assertion.AssertionValidationException; +import org.opensaml.saml.common.xml.SAMLConstants; import org.opensaml.saml.saml2.core.Assertion; import org.opensaml.saml.saml2.core.Audience; import org.opensaml.saml.saml2.core.AudienceRestriction; @@ -29,6 +34,12 @@ import org.opensaml.saml.saml2.core.Response; import org.opensaml.saml.saml2.core.impl.EncryptedAssertionMarshaller; import org.opensaml.saml.saml2.core.impl.EncryptedAssertionUnmarshaller; +import org.opensaml.security.credential.UsageType; +import org.opensaml.security.x509.BasicX509Credential; +import org.opensaml.xmlsec.signature.support.SignatureConstants; +import org.opensaml.xmlsec.signature.support.SignatureValidator; +import org.w3c.dom.Document; +import org.w3c.dom.Element; public class AssertionValidationServiceTest extends BaseServiceTest { @@ -326,6 +337,104 @@ public void testFailWrongSubject() throws Exception { }); } + @DisplayName("Test that validator will fail an assertion whose signature reference resolves to another element") + @Test + public void testFailSignatureNotBoundToAssertion() throws Exception { + AssertionValidationService validationService = new AssertionValidationService(); + + // Mock HttpServletRequest + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getRequestURL()).thenReturn(new StringBuffer(TestConstants.SP_ASSERTION_CONSUMER_URL)); + + // Create AuthnRequest + AuthnRequestService authnRequestService = AuthnRequestService.getInstance(); + AuthnRequest authnRequest = getAuthnRequest(authnRequestService); + String inResponseToId = authnRequest.getID(); + + // Create MessageContext, Response and a correctly signed Assertion + String nameID = "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7"; + MessageContext messageContext = IdpUtil.createMessageWithAssertion(true, true, true, nameID, TestConstants.SP_ENTITY_ID, TestConstants.SP_ASSERTION_CONSUMER_URL, inResponseToId); + Response response = (Response) messageContext.getMessage(); + + Assertion signedAssertion = new AssertionService().getAssertion(response); + + // Assertion with a different ID and subject than the one the signature reference resolves to + String substituteNameID = "https://data.gov.dk/model/core/eid/person/uuid/11111111-2222-3333-4444-555555555555"; + Assertion unboundAssertion = buildAssertionWithUnboundSignature(signedAssertion, "_copy" + UUID.randomUUID().toString().replace("-", ""), substituteNameID); + + // Only interesting while the signature itself still verifies, otherwise the test would pass for the + // wrong reason + X509Certificate idpCertificate = IdPMetadataService.getInstance().getIdPMetadata().getValidX509Certificate(UsageType.SIGNING); + SignatureValidator.validate(unboundAssertion.getSignature(), new BasicX509Credential(idpCertificate)); + + // Validate, should fail because the signature is not bound to the assertion being consumed + AssertionValidationException exception = Assertions.assertThrows(AssertionValidationException.class, () -> { + validationService.validate(request, messageContext, response, unboundAssertion, new AuthnRequestWrapper(authnRequest, NSISLevel.SUBSTANTIAL, "")); + }); + Assertions.assertTrue(exception.getMessage().toLowerCase().contains("signature"), "Expected the signature validation to reject the assertion, but failed with: " + exception.getMessage()); + } + + @DisplayName("Test that validator will fail an assertion whose signature reference is ambiguous") + @Test + public void testFailSignatureNotBoundToAssertionWithReusedId() throws Exception { + AssertionValidationService validationService = new AssertionValidationService(); + + // Mock HttpServletRequest + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getRequestURL()).thenReturn(new StringBuffer(TestConstants.SP_ASSERTION_CONSUMER_URL)); + + // Create AuthnRequest + AuthnRequestService authnRequestService = AuthnRequestService.getInstance(); + AuthnRequest authnRequest = getAuthnRequest(authnRequestService); + String inResponseToId = authnRequest.getID(); + + // Create MessageContext, Response and a correctly signed Assertion + String nameID = "https://data.gov.dk/model/core/eid/person/uuid/37a5a1aa-67ce-4f70-b7c0-b8e678d585f7"; + MessageContext messageContext = IdpUtil.createMessageWithAssertion(true, true, true, nameID, TestConstants.SP_ENTITY_ID, TestConstants.SP_ASSERTION_CONSUMER_URL, inResponseToId); + Response response = (Response) messageContext.getMessage(); + + Assertion signedAssertion = new AssertionService().getAssertion(response); + + // Same document shape, but the consumed assertion keeps the ID of the element the reference resolves + // to, so comparing the reference URI to the ID of its parent element is not enough to tell them apart + String substituteNameID = "https://data.gov.dk/model/core/eid/person/uuid/11111111-2222-3333-4444-555555555555"; + Assertion unboundAssertion = buildAssertionWithUnboundSignature(signedAssertion, signedAssertion.getID(), substituteNameID); + + // Validate, should fail because the reference does not resolve to the assertion being consumed + AssertionValidationException exception = Assertions.assertThrows(AssertionValidationException.class, () -> { + validationService.validate(request, messageContext, response, unboundAssertion, new AuthnRequestWrapper(authnRequest, NSISLevel.SUBSTANTIAL, "")); + }); + Assertions.assertTrue(exception.getMessage().toLowerCase().contains("signature"), "Expected the signature validation to reject the assertion, but failed with: " + exception.getMessage()); + } + + /** + * Build an assertion carrying a signature that verifies but is not bound to it: a copy of the signed + * assertion with the given ID and subject NameID, holding the signature, while the element the signature + * reference resolves to sits further down the same document. + */ + private static Assertion buildAssertionWithUnboundSignature(Assertion signedAssertion, String id, String nameID) throws Exception { + Element signedElement = signedAssertion.getDOM(); + Document document = signedElement.getOwnerDocument(); + + Element copy = (Element) signedElement.cloneNode(true); + copy.setAttributeNS(null, "ID", id); + ((Element) copy.getElementsByTagNameNS(SAMLConstants.SAML20_NS, "NameID").item(0)).setTextContent(nameID); + + // Only the copy keeps the signature, so the referenced element still digests to the signed value + Element signature = (Element) signedElement.getElementsByTagNameNS(SignatureConstants.XMLSIG_NS, "Signature").item(0); + signedElement.removeChild(signature); + + // The copy becomes the document element and the referenced element is nested inside it + document.removeChild(signedElement); + document.appendChild(copy); + + Element advice = document.createElementNS(SAMLConstants.SAML20_NS, "saml2:Advice"); + copy.appendChild(advice); + advice.appendChild(signedElement); + + return (Assertion) XMLObjectProviderRegistrySupport.getUnmarshallerFactory().getUnmarshaller(copy).unmarshall(copy); + } + private static AuthnRequest getAuthnRequest(AuthnRequestService authnRequestService) throws InitializationException { AuthnRequest authnRequest = authnRequestService.createAuthnRequest( TestConstants.SP_ASSERTION_CONSUMER_URL, false, false, NSISLevel.SUBSTANTIAL, null); return authnRequest;