Compliant SAML Response destination check (#31175)

Make SAML Response Destination check compliant Only validate the Destination element of an incoming SAML Response if Destination is present and the SAML Response is signed. The standard [1] - 3.5.5.2 and [2] - 3.2.2 does mention that the Destination element is optional and should only be verified when the SAML Response is signed. Some Identity Provider implementations are known to not set a Destination XML Attribute in their SAML responses when those are not signed, so this change also aims to enhance interoperability. [1] https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf [2] https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
elastic · Jun 14, 2018 · 65455e0 · 65455e0
1 parent 00b5e8c
commit 65455e0
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 2 deletions.
diff --git a/...security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java b/...security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java
@@ -151,8 +151,10 @@ private String getSessionIndex(Assertion assertion) {
     private void checkResponseDestination(Response response) {
         final String asc = getSpConfiguration().getAscUrl();
         if (asc.equals(response.getDestination()) == false) {
-            throw samlException("SAML response " + response.getID() + " is for destination " + response.getDestination()
+            if (response.isSigned() || Strings.hasText(response.getDestination())) {
+                throw samlException("SAML response " + response.getID() + " is for destination " + response.getDestination()
                     + " but this realm uses " + asc);
+            }
         }
     }
 

diff --git a/...ity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java b/...ity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java
@@ -523,13 +523,59 @@ public void testIncorrectDestinationIsRejected() throws Exception {
                 "</assert:Attribute></assert:AttributeStatement>" +
                 "</assert:Assertion>" +
                 "</proto:Response>";
-        SamlToken token = token(signDoc(xml));
+        SamlToken token = randomBoolean() ? token(signDoc(xml)) : token(signAssertions(xml, idpSigningCertificatePair));
         final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
         assertThat(exception.getMessage(), containsString("destination"));
         assertThat(exception.getCause(), nullValue());
         assertThat(SamlUtils.isSamlException(exception), is(true));
     }
 
+    public void testMissingDestinationIsNotRejectedForNotSignedResponse() throws Exception {
+        Instant now = clock.instant();
+        Instant validUntil = now.plusSeconds(30);
+        String sessionindex = randomId();
+        final String xml = "<?xml version='1.0' encoding='UTF-8'?>\n" +
+            "<proto:Response ID='" + randomId() + "' InResponseTo='" + requestId +
+            "' IssueInstant='" + now + "' Version='2.0'" +
+            " xmlns:proto='urn:oasis:names:tc:SAML:2.0:protocol'" +
+            " xmlns:assert='urn:oasis:names:tc:SAML:2.0:assertion'" +
+            " xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance'" +
+            " xmlns:xs='http://www.w3.org/2001/XMLSchema'" +
+            " xmlns:ds='http://www.w3.org/2000/09/xmldsig#' >" +
+            "<assert:Issuer>" + IDP_ENTITY_ID + "</assert:Issuer>" +
+            "<proto:Status><proto:StatusCode Value='urn:oasis:names:tc:SAML:2.0:status:Success'/></proto:Status>" +
+            "<assert:Assertion ID='" + sessionindex + "' IssueInstant='" + now + "' Version='2.0'>" +
+            "<assert:Issuer>" + IDP_ENTITY_ID + "</assert:Issuer>" +
+            "<assert:Subject>" +
+            "<assert:NameID SPNameQualifier='" + SP_ENTITY_ID + "' Format='" + TRANSIENT + "'>randomopaquestring</assert:NameID>" +
+            "<assert:SubjectConfirmation Method='" + METHOD_BEARER + "'>" +
+            "<assert:SubjectConfirmationData NotOnOrAfter='" + validUntil + "' Recipient='" + SP_ACS_URL + "' " +
+            "   InResponseTo='" + requestId + "'/>" +
+            "</assert:SubjectConfirmation>" +
+            "</assert:Subject>" +
+            "<assert:AuthnStatement AuthnInstant='" + now + "' SessionNotOnOrAfter='" + validUntil +
+            "' SessionIndex='" + sessionindex + "'>" +
+            "<assert:AuthnContext>" +
+            "<assert:AuthnContextClassRef>" + PASSWORD_AUTHN_CTX + "</assert:AuthnContextClassRef>" +
+            "</assert:AuthnContext>" +
+            "</assert:AuthnStatement>" +
+            "<assert:AttributeStatement><assert:Attribute " +
+            "   NameFormat='urn:oasis:names:tc:SAML:2.0:attrname-format:uri' Name='urn:oid:0.9.2342.19200300.100.1.1'>" +
+            "<assert:AttributeValue xsi:type='xs:string'>daredevil</assert:AttributeValue>" +
+            "</assert:Attribute></assert:AttributeStatement>" +
+            "</assert:Assertion>" +
+            "</proto:Response>";
+        SamlToken token = token(signAssertions(xml, idpSigningCertificatePair));
+        final SamlAttributes attributes = authenticator.authenticate(token);
+        assertThat(attributes, notNullValue());
+        assertThat(attributes.attributes(), iterableWithSize(1));
+        final List<String> uid = attributes.getAttributeValues("urn:oid:0.9.2342.19200300.100.1.1");
+        assertThat(uid, contains("daredevil"));
+        assertThat(uid, iterableWithSize(1));
+        assertThat(attributes.name(), notNullValue());
+        assertThat(attributes.name().format, equalTo(TRANSIENT));
+    }
+
     public void testIncorrectRequestIdIsRejected() throws Exception {
         Instant now = clock.instant();
         Instant validUntil = now.plusSeconds(30);