From e36b94d90562c7e927275a8e8e8a4a8a5d49af00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hynek=20Mlna=C5=99=C3=ADk?=
 <hmlnarik@users.noreply.github.com>
Date: Thu, 31 Aug 2017 05:46:26 +0200
Subject: [PATCH] KEYCLOAK-5318 Verify signature on raw query parameters
 (#4445)

---
 .../saml/JaxrsSAML2BindingBuilder.java        |  2 +-
 .../protocol/saml/SamlProtocolUtils.java      | 10 ++--
 .../keycloak/protocol/saml/SamlService.java   |  5 ++
 .../testsuite/saml/AbstractSamlTest.java      |  7 ++-
 .../testsuite/saml/BasicSamlTest.java         | 57 ++++++++++++++++++-
 5 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/services/src/main/java/org/keycloak/protocol/saml/JaxrsSAML2BindingBuilder.java b/services/src/main/java/org/keycloak/protocol/saml/JaxrsSAML2BindingBuilder.java
index d3efb2535d..e816986b98 100755
--- a/services/src/main/java/org/keycloak/protocol/saml/JaxrsSAML2BindingBuilder.java
+++ b/services/src/main/java/org/keycloak/protocol/saml/JaxrsSAML2BindingBuilder.java
@@ -69,7 +69,7 @@ public class JaxrsSAML2BindingBuilder extends BaseSAML2BindingBuilder<JaxrsSAML2
 
         private Response response(String redirectUri, boolean asRequest) throws ProcessingException, ConfigurationException, IOException {
             URI uri = generateURI(redirectUri, asRequest);
-            if (logger.isDebugEnabled()) logger.trace("redirect-binding uri: " + uri.toString());
+            logger.tracef("redirect-binding uri: %s", uri);
             CacheControl cacheControl = new CacheControl();
             cacheControl.setNoCache(true);
             return Response.status(302).location(uri)
diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java
index 7ab97a4a99..9a601739a2 100755
--- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java
+++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java
@@ -128,6 +128,7 @@ public class SamlProtocolUtils {
         String request = encodedParams.getFirst(paramKey);
         String algorithm = encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY);
         String signature = encodedParams.getFirst(GeneralConstants.SAML_SIGNATURE_REQUEST_KEY);
+        String relayState = encodedParams.getFirst(GeneralConstants.RELAY_STATE);
         String decodedAlgorithm = uriInformation.getQueryParameters(true).getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY);
 
         if (request == null) throw new VerificationException("SAM was null");
@@ -139,13 +140,12 @@ public class SamlProtocolUtils {
         // Shibboleth doesn't sign the document for redirect binding.
         // todo maybe a flag?
 
-        UriBuilder builder = UriBuilder.fromPath("/")
-                .queryParam(paramKey, request);
+        StringBuilder rawQueryBuilder = new StringBuilder().append(paramKey).append("=").append(request);
         if (encodedParams.containsKey(GeneralConstants.RELAY_STATE)) {
-            builder.queryParam(GeneralConstants.RELAY_STATE, encodedParams.getFirst(GeneralConstants.RELAY_STATE));
+            rawQueryBuilder.append("&" + GeneralConstants.RELAY_STATE + "=").append(relayState);
         }
-        builder.queryParam(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY, algorithm);
-        String rawQuery = builder.build().getRawQuery();
+        rawQueryBuilder.append("&" + GeneralConstants.SAML_SIG_ALG_REQUEST_KEY + "=").append(algorithm);
+        String rawQuery = rawQueryBuilder.toString();
 
         try {
             byte[] decodedSignature = RedirectBindingUtil.urlBase64Decode(signature);
diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java
index 6009f1c78a..8cbde8ed79 100755
--- a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java
+++ b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java
@@ -257,6 +257,11 @@ public class SamlService extends AuthorizationEndpointBase {
         protected Response loginRequest(String relayState, AuthnRequestType requestAbstractType, ClientModel client) {
             SamlClient samlClient = new SamlClient(client);
             // validate destination
+            if (requestAbstractType.getDestination() == null && samlClient.requiresClientSignature()) {
+                event.detail(Details.REASON, "invalid_destination");
+                event.error(Errors.INVALID_SAML_AUTHN_REQUEST);
+                return ErrorPage.error(session, Messages.INVALID_REQUEST);
+            }
             if (! isValidDestination(requestAbstractType.getDestination())) {
                 event.detail(Details.REASON, "invalid_destination");
                 event.error(Errors.INVALID_SAML_AUTHN_REQUEST);
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java
index 3f185e41a8..dec091b9da 100644
--- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/AbstractSamlTest.java
@@ -29,10 +29,11 @@ public abstract class AbstractSamlTest extends AbstractAuthTest {
     protected static final String SAML_ASSERTION_CONSUMER_URL_SALES_POST2 = "http://localhost:8080/sales-post2/";
     protected static final String SAML_CLIENT_ID_SALES_POST2 = "http://localhost:8081/sales-post2/";
 
-    protected static final String SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG = "http://localhost:8081/sales-post-sig/";
+    protected static final String SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG = "http://localhost:8080/sales-post-sig/";
     protected static final String SAML_CLIENT_ID_SALES_POST_SIG = "http://localhost:8081/sales-post-sig/";
-    protected static final String SAML_CLIENT_ID_SALES_POST_SIG_PRIVATE_KEY = "MIICdwIBADANBgkqhkiG9w0BAQEFAASCAmEwggJdAgEAAoGBAL72RWA8WmyGNLZpPaDnaWpO/aWpvWOC9dlWTuIUXgsA7pJ0JcpdFkjhK7MhGRU2kTVrjZXWRBGVqjOgWSWC9fmyN7y2Slr3KWr5vEZcz1FHJypG7YH6hZRXsCqrsGzRe53RZ4dBhFwSgKOqwUobLrmqQtEN1PvQVMmClIe2z6APAgMBAAECgYA1TV5+BzqiMi/Cfsux/wYAo33PYPq5LRPcj2fDWTYK0j7FaGAoBSW0QA3HmUR8FFgh1hyWJ1GmquTwNiDMBKsNhMdfOpUEl8CTVWsAamrJGr5M0wtQeReJOJIM5DpaqFf3dQYyYCBCbVGwtrr5/LUE78HbLzn5h/Y5TKWwpLpq8QJBAOyVEOI0V20SN2/I4vbS1Dv5b54pXFXEoDJDeAWMggOLMQtm34mrVbPpdX02ErUkW2VW6B4PW2Vb/4rXp8SYNhcCQQDOoqg38l4ZvbSt9CV0BRZ1BtvSOtABiWChJT/19gf1hUlPy+eR+E8FPAjD0f165/X/+VE/+MeW0d3lyHjbfRjJAkEA3es6SiW0+IAE9lum4saDBLsHA4JitaVaa6u0EuhpMK/JUpuuBfJs0vWkGs61H6u5+8ZYt5HKNrrkazW9joEFAwJBAJOw2r8yMmP/nbZ/vI1SXZzDjDaU5rtSb4h+UVsBwOqRm7a3LQq+CezZ3gHog15nkQKmNpacwDtiQVHNmeR3Y1ECQHyURjnpJyc2KChmSSO0RR3Z6N2Fk9i5L1Ip+/3MfA34j4MTa9UVnPwY+r+ItwzHXc+Rlphok1DgW1Bj+FEtKfk=";
-    protected static final String SAML_CLIENT_ID_SALES_POST_SIG_PUBLIC_KEY = "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC+9kVgPFpshjS2aT2g52lqTv2lqb1jgvXZVk7iFF4LAO6SdCXKXRZI4SuzIRkVNpE1a42V1kQRlaozoFklgvX5sje8tkpa9ylq+bxGXM9RRycqRu2B+oWUV7Aqq7Bs0Xud0WeHQYRcEoCjqsFKGy65qkLRDdT70FTJgpSHts+gDwIDAQAB";
+    protected static final String SAML_URL_SALES_POST_SIG = "http://localhost:8080/sales-post-sig/";
+    protected static final String SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY = "MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBANUbxrvEY3pkiQNt55zJLKBwN+zKmNQw08ThAmOKzwHfXoK+xlDSFxNMtTKJGkeUdnKzaTfESEcEfKYULUA41y/NnOlvjS0CEsc7Wq0Ce63TSSGMB2NHea4tV0aQz/MwLsbmz2IjAFWHA5CHL5WwacIf3UTOSNnhJUSvnkomjJAlAgMBAAECgYANpO2gb/5+g5lSIuNFYov86bJq8r2+ODIW1OE2Rljioc6HSHeiDRF1JuAjECwikRrUVTBTZbnK8jqY14neJsWAKBzGo+ToaQALsNZ9B91DxxL50K5oVOzw5shAS9TnRjN40+KIXFED4ydq4JRdoqb8+cN+N3i0+Cu7tdm+UaHTAQJBAOwFs3ZwqQEqmv9vmgmIFwFpJm1aIw25gEOf3Hy45GP4bL/j0FQgwcXYRbLE5bPqhw/liLKc1GQ97bVm6zs8SvUCQQDnJZA6TFRMiDjezinE1J4e0v4RupyDniVjbE5ArTK5/FRVkjw4Ny0AqZUEyIIqlTeZlCq45pCJy4a2hymDGVJxAj9gzfXNnmezEsZ//kYvoqHM8lPQhifaeTsigW7tuOf0GPCBw+6uksDnZM0xhZCxOoArBPoMSEbU1pGo1Y2lvhUCQF6E5sBgHAybm53Ich4Rz4LNRqWbSIstrR5F2I3sBRU2kInZXZSjQ1zE+7HUCB4/nFfJ1dp8NdiTCEg1Zw072pECQQDnxyQALmWhQbBTl0tq6CwYf9rZDwBzxuY+CXB8Ky1gOmXwan96KZvV4rK8MQQs6HIiYC/j+5lX3A3zlXTFldaz";
+    protected static final String SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY = "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDVG8a7xGN6ZIkDbeecySygcDfsypjUMNPE4QJjis8B316CvsZQ0hcTTLUyiRpHlHZys2k3xEhHBHymFC1AONcvzZzpb40tAhLHO1qtAnut00khjAdjR3muLVdGkM/zMC7G5s9iIwBVhwOQhy+VsGnCH91EzkjZ4SVEr55KJoyQJQIDAQAB";
 
     protected static final String SAML_ASSERTION_CONSUMER_URL_SALES_POST_ENC = "http://localhost:8080/sales-post-enc/";
     protected static final String SAML_CLIENT_ID_SALES_POST_ENC = "http://localhost:8081/sales-post-enc/";
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java
index 1e126e088b..7a5cba92dc 100644
--- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java
@@ -3,17 +3,24 @@ package org.keycloak.testsuite.saml;
 import org.junit.Test;
 import org.keycloak.dom.saml.v2.protocol.AuthnRequestType;
 import org.keycloak.protocol.saml.SamlProtocol;
+import org.keycloak.saml.SignatureAlgorithm;
+import org.keycloak.saml.common.constants.GeneralConstants;
 import org.keycloak.saml.common.exceptions.ConfigurationException;
 import org.keycloak.saml.common.exceptions.ParsingException;
 import org.keycloak.saml.common.exceptions.ProcessingException;
 import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request;
 import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
+import org.keycloak.saml.processing.web.util.RedirectBindingUtil;
 import org.keycloak.services.resources.RealmsResource;
+import org.keycloak.testsuite.util.KeyUtils;
 import org.keycloak.testsuite.util.SamlClient;
 import org.keycloak.testsuite.util.SamlClient.Binding;
 import org.keycloak.testsuite.util.SamlClient.RedirectStrategyWithSwitchableFollowRedirect;
 import org.keycloak.testsuite.util.SamlClientBuilder;
+import java.net.URI;
+import java.security.Signature;
 import javax.ws.rs.core.Response;
+import javax.ws.rs.core.Response.Status;
 import javax.ws.rs.core.UriBuilder;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpUriRequest;
@@ -21,6 +28,7 @@ import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.util.EntityUtils;
 import org.hamcrest.Matcher;
+import org.jboss.resteasy.util.Encode;
 import org.w3c.dom.Document;
 
 import static org.hamcrest.CoreMatchers.not;
@@ -52,6 +60,53 @@ public class BasicSamlTest extends AbstractSamlTest {
         assertThat(documentToString(document.getSamlDocument()), not(containsString("InResponseTo=\"" + System.getProperty("java.version") + "\"")));
     }
 
+    @Test
+    public void testRedirectUrlSigned() throws Exception {
+        testSpecialCharsInRelayState(null);
+    }
+
+    @Test
+    public void testRedirectUrlUnencodedSpecialChars() throws Exception {
+        testSpecialCharsInRelayState("New%20Document%20(1).doc");
+    }
+
+    @Test
+    public void testRedirectUrlEncodedSpecialChars() throws Exception {
+        testSpecialCharsInRelayState("New%20Document%20%281%29.doc");
+    }
+
+    private void testSpecialCharsInRelayState(String encodedRelayState) throws Exception {
+        AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST_SIG, SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG, getAuthServerSamlEndpoint(REALM_NAME));
+
+        Document doc = SAML2Request.convert(loginRep);
+        URI redirect = Binding.REDIRECT.createSamlUnsignedRequest(getAuthServerSamlEndpoint(REALM_NAME), null, doc).getURI();
+        String query = redirect.getRawQuery();
+        SignatureAlgorithm signatureAlgorithm = SignatureAlgorithm.RSA_SHA256;
+
+        // now add the relayState
+        String relayStatePart = encodedRelayState == null
+          ? ""
+          : ("&" + GeneralConstants.RELAY_STATE + "=" + encodedRelayState);
+        String sigAlgPart = "&" + GeneralConstants.SAML_SIG_ALG_REQUEST_KEY + "=" + Encode.encodeQueryParamAsIs(signatureAlgorithm.getXmlSignatureMethod());
+
+        Signature signature = signatureAlgorithm.createSignature();
+        byte[] sig;
+
+        signature.initSign(KeyUtils.privateKeyFromString(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY));
+        signature.update(query.getBytes(GeneralConstants.SAML_CHARSET));
+        signature.update(relayStatePart.getBytes(GeneralConstants.SAML_CHARSET));
+        signature.update(sigAlgPart.getBytes(GeneralConstants.SAML_CHARSET));
+        sig = signature.sign();
+
+        String encodedSig = RedirectBindingUtil.base64Encode(sig);
+        String sigPart = "&" + GeneralConstants.SAML_SIGNATURE_REQUEST_KEY + "=" + Encode.encodeQueryParamAsIs(encodedSig);
+
+        new SamlClientBuilder()
+          .navigateTo(redirect.toString() + relayStatePart + sigAlgPart + sigPart)
+          .assertResponse(statusCodeIsHC(Status.OK))
+          .execute();
+    }
+
     @Test
     public void testNoDestinationPost() throws Exception {
         AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, SAML_ASSERTION_CONSUMER_URL_SALES_POST, null);
@@ -85,7 +140,7 @@ public class BasicSamlTest extends AbstractSamlTest {
         AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST_SIG, SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG, null);
 
         Document doc = SAML2Request.convert(loginRep);
-        HttpUriRequest post = Binding.POST.createSamlSignedRequest(getAuthServerSamlEndpoint(REALM_NAME), null, doc, SAML_CLIENT_ID_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_ID_SALES_POST_SIG_PUBLIC_KEY);
+        HttpUriRequest post = Binding.POST.createSamlSignedRequest(getAuthServerSamlEndpoint(REALM_NAME), null, doc, SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY);
 
         try (CloseableHttpClient client = HttpClientBuilder.create().setRedirectStrategy(new RedirectStrategyWithSwitchableFollowRedirect()).build();
           CloseableHttpResponse response = client.execute(post)) {