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;