From 97811fdd51950305b512e325c050dd3eecd8ba7f Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Wed, 10 Jul 2019 08:52:55 +0200 Subject: [PATCH] KEYCLOAK-10786 Check signature presence in SAML broker (cherry picked from commit ba9f73aaff22eb34c7dec16f4b76d36d855d569b) --- .../keycloak/broker/saml/SAMLEndpoint.java | 27 ++- .../broker/KcSamlSignedBrokerTest.java | 175 +++++++++++++++++- 2 files changed, 188 insertions(+), 14 deletions(-) diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index 3759f17819..2ad1a01d12 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -94,6 +94,7 @@ import java.security.cert.CertificateException; import org.w3c.dom.Element; import java.util.*; +import javax.ws.rs.core.MultivaluedMap; import javax.xml.crypto.dsig.XMLSignature; import org.w3c.dom.NodeList; @@ -216,6 +217,7 @@ public class SAMLEndpoint { } protected abstract String getBindingType(); + protected abstract boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder); protected abstract void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException; protected abstract SAMLDocumentHolder extractRequestDocument(String samlRequest); protected abstract SAMLDocumentHolder extractResponseDocument(String response); @@ -384,8 +386,11 @@ public class SAMLEndpoint { } boolean signed = AssertionUtil.isSignedElement(assertionElement); - if ((config.isWantAssertionsSigned() && !signed) - || (signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator()))) { + final boolean assertionSignatureNotExistsWhenRequired = config.isWantAssertionsSigned() && !signed; + final boolean signatureNotValid = signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator()); + final boolean hasNoSignatureWhenRequired = ! signed && config.isValidateSignature() && ! containsUnencryptedSignature(holder); + + if (assertionSignatureNotExistsWhenRequired || signatureNotValid || hasNoSignatureWhenRequired) { logger.error("validation failed"); event.event(EventType.IDENTITY_PROVIDER_RESPONSE); event.error(Errors.INVALID_SIGNATURE); @@ -545,10 +550,14 @@ public class SAMLEndpoint { protected class PostBinding extends Binding { @Override - protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException { + protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) { NodeList nl = documentHolder.getSamlDocument().getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); - boolean anyElementSigned = (nl != null && nl.getLength() > 0); - if ((! anyElementSigned) && (documentHolder.getSamlObject() instanceof ResponseType)) { + return (nl != null && nl.getLength() > 0); + } + + @Override + protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException { + if ((! containsUnencryptedSignature(documentHolder)) && (documentHolder.getSamlObject() instanceof ResponseType)) { ResponseType responseType = (ResponseType) documentHolder.getSamlObject(); List assertions = responseType.getAssertions(); if (! assertions.isEmpty() ) { @@ -577,6 +586,14 @@ public class SAMLEndpoint { } protected class RedirectBinding extends Binding { + @Override + protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) { + MultivaluedMap encodedParams = session.getContext().getUri().getQueryParameters(false); + String algorithm = encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY); + String signature = encodedParams.getFirst(GeneralConstants.SAML_SIGNATURE_REQUEST_KEY); + return algorithm != null && signature != null; + } + @Override protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException { KeyLocator locator = getIDPKeyLocator(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java index 56ba994448..bbe1deda6c 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java @@ -10,6 +10,8 @@ import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.util.DocumentUtil; import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request; +import org.keycloak.saml.processing.core.parsers.saml.assertion.SAMLAssertionQNames; +import org.keycloak.saml.processing.core.parsers.saml.protocol.SAMLProtocolQNames; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.testsuite.arquillian.SuiteContext; @@ -21,28 +23,44 @@ import org.keycloak.testsuite.util.KeyUtils; import org.keycloak.testsuite.util.SamlClient; import org.keycloak.testsuite.util.SamlClient.Binding; import org.keycloak.testsuite.util.SamlClientBuilder; +import org.keycloak.testsuite.util.saml.SamlDocumentStepBuilder.Saml2DocumentTransformer; import java.io.Closeable; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; import javax.ws.rs.core.Response.Status; +import javax.xml.crypto.dsig.XMLSignature; +import javax.xml.namespace.QName; +import org.apache.http.HttpResponse; +import org.hamcrest.Matcher; import org.hamcrest.Matchers; import org.junit.Assert; import org.junit.Test; +import org.w3c.dom.DOMException; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NamedNodeMap; import org.w3c.dom.Node; import org.w3c.dom.NodeList; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; import static org.keycloak.testsuite.broker.BrokerTestConstants.*; +import static org.keycloak.testsuite.util.Matchers.bodyHC; import static org.keycloak.testsuite.util.Matchers.isSamlResponse; public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { + private static final String PRIVATE_KEY = "MIIBVQIBADANBgkqhkiG9w0BAQEFAASCAT8wggE7AgEAAkEAs46ICYPRIkmr8diECmyT59cChTWIEiXYBY3T6OLlZrF8ofVCzbEeoUOmhrtHijxxuKSoqLWP4nNOt3rINtQNBQIDAQABAkBL2nyxuFQTLhhLdPJjDPd2y6gu6ixvrjkSL5ZEHgZXWRHzhTzBT0eRxg/5rJA2NDRMBzTTegaEGkWUt7lF5wDJAiEA5pC+h9NEgqDJSw42I52BOml3II35Z6NlNwl6OMfnD1sCIQDHXUiOIJy4ZcSgv5WGue1KbdNVOT2gop1XzfuyWgtjHwIhAOCjLb9QC3PqC7Tgx8azcnDiyHojWVesTrTsuvQPcAP5AiAkX5OeQrr1NbQTNAEe7IsrmjAFi4T/6stUOsOiPaV4NwIhAJIeyh4foIXIVQ+M4To2koaDFRssxKI9/O72vnZSJ+uA"; + private static final String PUBLIC_KEY = "MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBALOOiAmD0SJJq/HYhApsk+fXAoU1iBIl2AWN0+ji5WaxfKH1Qs2xHqFDpoa7R4o8cbikqKi1j+JzTrd6yDbUDQUCAwEAAQ=="; + public class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration { @Override @@ -115,7 +133,7 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { return new KcSamlSignedBrokerConfiguration(); } - public void withSignedEncryptedAssertions(Runnable testBody, boolean signedAssertion, boolean encryptedAssertion) throws Exception { + public void withSignedEncryptedAssertions(Runnable testBody, boolean signedDocument, boolean signedAssertion, boolean encryptedAssertion) throws Exception { String providerCert = KeyUtils.getActiveKey(adminClient.realm(bc.providerRealmName()).keys().getKeyMetadata(), Algorithm.RS256).getCertificate(); Assert.assertThat(providerCert, Matchers.notNullValue()); @@ -123,18 +141,20 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { Assert.assertThat(consumerCert, Matchers.notNullValue()); try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource) - .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(signedAssertion)) + .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(signedAssertion || signedDocument)) .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, Boolean.toString(signedAssertion)) .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, Boolean.toString(encryptedAssertion)) .setAttribute(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, "false") + .setAttribute(SAMLIdentityProviderConfig.ENCRYPTION_PUBLIC_KEY, PUBLIC_KEY) .setAttribute(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, providerCert) .update(); Closeable clientUpdater = ClientAttributeUpdater.forClient(adminClient, bc.providerRealmName(), bc.getIDPClientIdInProviderRealm(suiteContext)) .setAttribute(SamlConfigAttributes.SAML_ENCRYPT, Boolean.toString(encryptedAssertion)) .setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_CERTIFICATE_ATTRIBUTE, consumerCert) - .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "false") // only sign assertions + .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, Boolean.toString(signedDocument)) .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, Boolean.toString(signedAssertion)) - .setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false") + .setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_PRIVATE_KEY_ATTRIBUTE, PRIVATE_KEY) + .setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false") // Do not require client signature .update()) { testBody.run(); @@ -143,12 +163,12 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { @Test public void testSignedEncryptedAssertions() throws Exception { - withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, true); + withSignedEncryptedAssertions(this::testAssertionSignatureRespected, false, true, true); } @Test public void testSignedAssertion() throws Exception { - withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, false); + withSignedEncryptedAssertions(this::testAssertionSignatureRespected, false, true, false); } private void testAssertionSignatureRespected() { @@ -245,17 +265,17 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { @Test public void loginUserAllNamespacesInTopElementSignedEncryptedAssertion() throws Exception { - withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, true); + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true, true); } @Test public void loginUserAllNamespacesInTopElementSignedAssertion() throws Exception { - withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, false); + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true, false); } @Test public void loginUserAllNamespacesInTopElementEncryptedAssertion() throws Exception { - withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true); + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, false, true); } @Test @@ -289,4 +309,141 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { } } + + @Test + public void testSignatureTampering_NOsignDoc_NOsignAssert_NOencAssert() throws Exception { + loginAttackChangeSignature(false, false, false); + } + + @Test + public void testSignatureTampering_NOsignDoc_NOsignAssert_encAssert() throws Exception { + loginAttackChangeSignature(false, false, true); + } + + @Test + public void testSignatureTampering_NOsignDoc_signAssert_NOencAssert() throws Exception { + loginAttackChangeSignature(false, true, false); + } + + @Test + public void testSignatureTampering_NOsignDoc_signAssert_encAssert() throws Exception { + loginAttackChangeSignature(false, true, true); + } + + @Test + public void testSignatureTampering_signDoc_NOsignAssert_NOencAssert() throws Exception { + loginAttackChangeSignature(true, false, false); + } + + @Test + public void testSignatureTampering_signDoc_NOsignAssert_encAssert() throws Exception { + loginAttackChangeSignature(true, false, true); + } + + @Test + public void testSignatureTampering_signDoc_signAssert_NOencAssert() throws Exception { + loginAttackChangeSignature(true, true, false); + } + + @Test + public void testSignatureTampering_signDoc_signAssert_encAssert() throws Exception { + loginAttackChangeSignature(true, true, true); + } + + private Document removeDocumentSignature(Document orig) { + return removeSignatureTag(orig, Collections.singleton(SAMLProtocolQNames.RESPONSE.getQName())); + } + + private Document removeAssertionSignature(Document orig) { + return removeSignatureTag(orig, Collections.singleton(SAMLAssertionQNames.ASSERTION.getQName())); + } + + private Document removeDocumentAndAssertionSignature(Document orig) { + return removeSignatureTag(orig, + new HashSet<>(Arrays.asList(SAMLProtocolQNames.RESPONSE.getQName(), SAMLAssertionQNames.ASSERTION.getQName())) + ); + } + + private Document removeSignatureTag(Document orig, final Set qNames) throws DOMException { + NodeList sigElements = orig.getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); + LinkedList nodesToRemove = new LinkedList<>(); + for (int i = 0; i < sigElements.getLength(); i ++) { + Node n = sigElements.item(i); + final Node p = n.getParentNode(); + QName q = new QName(p.getNamespaceURI(), p.getLocalName()); + if (qNames.contains(q)) { + nodesToRemove.add(n); + } + } + nodesToRemove.forEach(n -> n.getParentNode().removeChild(n)); + return orig; + } + + private void loginAttackChangeSignature(boolean producerSignDocument, boolean producerSignAssertions, boolean producerEncryptAssertions) throws Exception { + log.debug(""); + + loginAttackChangeSignature("No changes to SAML document", producerSignDocument, producerSignAssertions, producerEncryptAssertions, + t -> t, true); + + // TODO: producerSignAssertions should be removed once there would be option to force check SAML document signature + boolean validAfterTamperingWithDocumentSignature = ! producerSignDocument || producerSignAssertions; + + loginAttackChangeSignature("Remove document signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions, + this::removeDocumentSignature, validAfterTamperingWithDocumentSignature); + + + // Tests for assertion signature manipulation follow. Tampering with assertion signature is + // relevant only if assertion is not encrypted since signature is part of encrypted data, + // hence skipped in the opposite case. + if (producerEncryptAssertions) { + return; + } + + // When assertion signature is removed, the expected document validation passes only + // if neither the document was not signed + // (otherwise document signature is invalidated by removing signature from the assertion) + // nor the assertion is + boolean validAfterTamperingWithAssertionSignature = ! producerSignAssertions; + + // When both assertion and document signatures are removed, the document validation passes only + // if neiter the document nor assertion were signed + boolean validAfterTamperingWithBothDocumentAndAssertionSignature = ! producerSignDocument && ! producerSignAssertions; + + loginAttackChangeSignature("Remove assertion signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions, + this::removeAssertionSignature, validAfterTamperingWithAssertionSignature); + loginAttackChangeSignature("Remove both document and assertion signature", producerSignDocument, producerSignAssertions, producerEncryptAssertions, + this::removeDocumentAndAssertionSignature, validAfterTamperingWithBothDocumentAndAssertionSignature); + } + + private void loginAttackChangeSignature(String description, + boolean producerSignDocument, boolean producerSignAssertions, boolean producerEncryptAssertions, + Saml2DocumentTransformer tr, boolean shouldSucceed) throws Exception { + log.infof("producerSignDocument: %s, producerSignAssertions: %s, producerEncryptAssertions: %s", producerSignDocument, producerSignAssertions, producerEncryptAssertions); + + Matcher responseFromConsumerMatcher = shouldSucceed + ? bodyHC(containsString("Update Account Information")) + : not(bodyHC(containsString("Update Account Information"))); + + AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, AUTH_SERVER_SCHEME + "://localhost:" + AUTH_SERVER_PORT + "/sales-post/saml", null); + Document doc = SAML2Request.convert(loginRep); + + withSignedEncryptedAssertions(() -> { + new SamlClientBuilder() + .authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, Binding.POST).build() // Request to consumer IdP + + .login().idp(bc.getIDPAlias()).build() + + .processSamlResponse(Binding.POST).build() // AuthnRequest to producer IdP + + .login().user(bc.getUserLogin(), bc.getUserPassword()).build() + + .processSamlResponse(Binding.POST) // Response from producer IdP + .transformDocument(tr) + .build() + + // first-broker flow: if valid request, it displays an update profile page on consumer realm + .execute(currentResponse -> assertThat(description, currentResponse, responseFromConsumerMatcher)); + }, producerSignDocument, producerSignAssertions, producerEncryptAssertions); + } + }