From 92cce7a6d4398db23f6c3481d9dc2f3b96530696 Mon Sep 17 00:00:00 2001 From: James Stapleton Date: Sun, 26 Nov 2017 10:58:22 -0500 Subject: [PATCH] [KEYCLOAK-5912] Add better improper SAML assertion error handling --- .../parsers/saml/SAMLAssertionParser.java | 24 ++++++--- .../core/parsers/saml/SAMLParserTest.java | 51 +++++++++++++++++-- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java index 5dfe8b7554..80e6956266 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java @@ -24,6 +24,8 @@ import org.keycloak.dom.saml.v2.assertion.EncryptedAssertionType; import org.keycloak.dom.saml.v2.assertion.NameIDType; import org.keycloak.dom.saml.v2.assertion.SubjectType; import org.keycloak.saml.common.ErrorCodes; +import org.keycloak.saml.common.PicketLinkLogger; +import org.keycloak.saml.common.PicketLinkLoggerFactory; import org.keycloak.saml.common.constants.JBossSAMLConstants; import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; @@ -52,6 +54,7 @@ import javax.xml.stream.events.XMLEvent; * @since Oct 12, 2010 */ public class SAMLAssertionParser implements ParserNamespaceSupport { + private static final PicketLinkLogger logger = PicketLinkLoggerFactory.getLogger(); private final String ASSERTION = JBossSAMLConstants.ASSERTION.get(); @@ -157,15 +160,22 @@ public class SAMLAssertionParser implements ParserNamespaceSupport { } private AssertionType parseBaseAttributes(StartElement nextElement) throws ParsingException { - Attribute idAttribute = nextElement.getAttributeByName(new QName(JBossSAMLConstants.ID.get())); - String id = StaxParserUtil.getAttributeValue(idAttribute); + String id = StaxParserUtil.getAttributeValue(nextElement, JBossSAMLConstants.ID.get()); + if (id == null) { + throw logger.parserRequiredAttribute(JBossSAMLConstants.ID.get()); + } - Attribute versionAttribute = nextElement.getAttributeByName(new QName(JBossSAMLConstants.VERSION.get())); - String version = StaxParserUtil.getAttributeValue(versionAttribute); - StringUtil.match(JBossSAMLConstants.VERSION_2_0.get(), version); + String version = StaxParserUtil.getAttributeValue(nextElement, JBossSAMLConstants.VERSION.get()); + if (!JBossSAMLConstants.VERSION_2_0.get().equals(version)) { + throw logger.parserException(new RuntimeException( + String.format("Assertion %s required to be \"%s\"", JBossSAMLConstants.VERSION.get(), JBossSAMLConstants.VERSION_2_0.get()))); + } - Attribute issueInstantAttribute = nextElement.getAttributeByName(new QName(JBossSAMLConstants.ISSUE_INSTANT.get())); - XMLGregorianCalendar issueInstant = XMLTimeUtil.parse(StaxParserUtil.getAttributeValue(issueInstantAttribute)); + String issueInstantString = StaxParserUtil.getAttributeValue(nextElement, JBossSAMLConstants.ISSUE_INSTANT.get()); + if (issueInstantString == null) { + throw logger.parserRequiredAttribute(JBossSAMLConstants.ISSUE_INSTANT.get()); + } + XMLGregorianCalendar issueInstant = XMLTimeUtil.parse(issueInstantString); return new AssertionType(id, issueInstant); } diff --git a/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java b/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java index 68baf0ddb4..81ebbe068f 100644 --- a/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java +++ b/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java @@ -16,25 +16,28 @@ */ package org.keycloak.saml.processing.core.parsers.saml; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.instanceOf; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.not; -import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.security.PrivateKey; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; + +import org.hamcrest.CustomMatcher; import org.keycloak.common.util.Base64; import org.keycloak.common.util.DerUtils; +import org.keycloak.common.util.StreamUtil; import org.keycloak.dom.saml.v2.assertion.AssertionType; import org.keycloak.dom.saml.v2.assertion.AttributeStatementType; import org.keycloak.dom.saml.v2.assertion.AttributeType; @@ -67,6 +70,9 @@ public class SAMLParserTest { private SAMLParser parser; + @Rule + public ExpectedException thrown = ExpectedException.none(); + @Before public void initParser() { this.parser = new SAMLParser(); @@ -326,5 +332,40 @@ public class SAMLParserTest { } } + @Test + public void testSaml20AssertionsMissingId() throws IOException, ParsingException { + try (InputStream st = removeAttribute("saml20-assertion-example.xml", "ID")) { + thrown.expect(ParsingException.class); + thrown.expectMessage(endsWith("Required attribute missing: ID")); + parser.parse(st); + } + } + + @Test + public void testSaml20AssertionsMissingVersion() throws IOException, ParsingException { + try (InputStream st = removeAttribute("saml20-assertion-example.xml", "Version")) { + thrown.expect(ParsingException.class); + thrown.expectMessage(endsWith("Assertion Version required to be \"2.0\"")); + parser.parse(st); + } + } + + @Test + public void testSaml20AssertionsMissingIssueInstance() throws IOException, ParsingException { + try (InputStream st = removeAttribute("saml20-assertion-example.xml", "IssueInstant")) { + thrown.expect(ParsingException.class); + thrown.expectMessage(endsWith("Required attribute missing: IssueInstant")); + parser.parse(st); + } + } + + private InputStream removeAttribute(String resourceName, String attribute) throws IOException { + try (InputStream st = SAMLParserTest.class.getResourceAsStream(resourceName)) { + String str = StreamUtil.readString(st, StandardCharsets.UTF_8); + String processed = str.replaceAll(attribute + "=\"[^\"]+\"", ""); + return new ByteArrayInputStream(processed.getBytes()); + } + } + }