Bugfix for not validating the authn requests against signature attackes

OpenConext · Dec 19, 2023 · 29bb322 · 29bb322
1 parent 01d34d8
commit 29bb322
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 32 deletions.
diff --git a/src/main/java/saml/DefaultSAMLService.java b/src/main/java/saml/DefaultSAMLService.java
@@ -28,6 +28,7 @@
 import org.opensaml.saml.saml2.core.*;
 import org.opensaml.saml.saml2.metadata.Extensions;
 import org.opensaml.saml.saml2.metadata.*;
+import org.opensaml.saml.security.impl.SAMLSignatureProfileValidator;
 import org.opensaml.security.credential.Credential;
 import org.opensaml.security.credential.UsageType;
 import org.opensaml.security.credential.impl.KeyStoreCredentialResolver;
@@ -90,6 +91,7 @@ public class DefaultSAMLService implements SAMLService {
     private static final Logger LOG = LoggerFactory.getLogger(DefaultSAMLService.class);
 
     private final OpenSamlVelocityEngine velocityEngine = new OpenSamlVelocityEngine();
+    private final SAMLSignatureProfileValidator samlSignatureProfileValidator = new SAMLSignatureProfileValidator();
     private final BasicParserPool parserPool;
     private final Map<String, SAMLServiceProvider> serviceProviders;
     private final SAMLConfiguration configuration;
@@ -172,7 +174,7 @@ private void validateSignature(SignableSAMLObject target, Credential credential,
                 throw new SignatureException("Signature element not found.");
             }
         } else {
-            SignatureValidator.validate(signature, credential);
+            this.samlSignatureProfileValidator.validate(signature);
         }
     }
 

diff --git a/src/test/java/saml/DefaultSAMLServiceTest.java b/src/test/java/saml/DefaultSAMLServiceTest.java
@@ -27,6 +27,7 @@
 import saml.crypto.KeyStoreLocator;
 import saml.model.*;
 
+import java.io.File;
 import java.io.InputStream;
 import java.nio.charset.Charset;
 import java.security.KeyStore;
@@ -37,21 +38,17 @@
 import java.util.Map;
 import java.util.UUID;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
-import java.io.File;
 import static org.junit.jupiter.api.Assertions.*;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import static saml.parser.EncodingUtils.deflatedBase64encoded;
 
 class DefaultSAMLServiceTest {
 
     private static final SimpleDateFormat issueFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss");
     private static final String spEntityId = "https://engine.test.surfconext.nl/authentication/sp/metadata";
     private static final Credential signingCredential;
-
-    private static final Logger LOG = LoggerFactory.getLogger(DefaultSAMLServiceTest.class);
 
     @RegisterExtension
     WireMockExtension mockServer = new WireMockExtension(8999);
@@ -317,60 +314,50 @@ void createAuthnRequest() {
         String authnRequestXML = this.defaultSAMLService.createAuthnRequest(serviceProvider,
                 "https://mujina-idp.test.surfconext.nl/SingleSignOnService",
                 true, true, "https://refeds.org/profile/mfa");
-        
+
         AuthnRequest authnRequest = this.defaultSAMLService.parseAuthnRequest(authnRequestXML, true, true);
         assertEquals(serviceProvider.getEntityId(), authnRequest.getIssuer().getValue());
         assertEquals(serviceProvider.getAcsLocation(), authnRequest.getAssertionConsumerServiceURL());
     }
-    
+
     /**
      * Tests signature wrapping attacks in the authentication requests, with the following message modifications:
      * - Wrapped content with/without signature
      * - Wrapped content in Object / RequestedAuthnContext
      * - Processed content with equal/modified/missing ID
-     * 
-     * This leads to 12 different message combinations. 
-     * The destination in every message is modified to hackmanit.de. 
-     * 
+     *
+     * This leads to 12 different message combinations.
+     * The destination in every message is modified to hackmanit.de.
+     *
      * If any of the message is successfully validated AND hackmanit.de is processed as a valid destination,
      * the test fails.
-     * 
-     * Note that all messages were generated statically so there is a need for an update once the keys/certs 
+     *
+     * Note that all messages were generated statically so there is a need for an update once the keys/certs
      * are updated.
      */
     @Test
     void testSignatureWrappingAttacks() {
-
         File[] files = new File(DefaultSAMLService.class.getClassLoader().getResource("req-wrapping").getPath()).listFiles();
-
-        for (File file : files) {
+        Stream.of(files).forEach(file -> {
             String authnRequestXML = readFile("req-wrapping/" + file.getName());
-            try {
-                AuthnRequest authnRequest = defaultSAMLService.parseAuthnRequest(authnRequestXML, false, false);
-                String destination = authnRequest.getDestination();
-                LOG.warn("Signature valid for " + file.getName() + " with destination: " + destination);
-                assertEquals("https://mujina-idp.test.surfconext.nl/SingleSignOnService", destination);
-            } catch (Exception ex) {
-                LOG.debug("Exception successfully thrown for " + file.getName(), ex);
-            }
-        }
-
+            assertThrows(SignatureException.class, () -> defaultSAMLService.parseAuthnRequest(authnRequestXML, false, false));
+        });
     }
-    
+
     /**
      * Tests for node splitting attacks with CDATA and comments.
-     * 
+     *
      * If any of the message is successfully split the issuer text content, an error is thrown.
-     * 
-     * Note that all messages were generated statically so there is a need for an update once the keys/certs 
+     *
+     * Note that all messages were generated statically so there is a need for an update once the keys/certs
      * are updated.
      */
     @Test
     void testNodeSplitting() {
         String authnRequestXML = readFile("node-splitting/comment.xml");
         AuthnRequest authnRequest = defaultSAMLService.parseAuthnRequest(authnRequestXML, false, false);
         assertEquals("https://engine.test.surfconext.nl/authentication/sp/metadata", authnRequest.getIssuer().getValue());
-        
+
         authnRequestXML = readFile("node-splitting/cdata.xml");
         authnRequest = defaultSAMLService.parseAuthnRequest(authnRequestXML, false, false);
         assertEquals("https://engine.test.surfconext.nl/authentication/sp/metadata", authnRequest.getIssuer().getValue());