From e6a64e234b276f49503f9b4bdbe03a342d9b9018 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Mon, 11 Dec 2017 13:45:00 +0100 Subject: [PATCH] KEYCLOAK-5644 Skip Advice tag in SAML messages --- .../common/constants/JBossSAMLConstants.java | 2 +- saml-core/pom.xml | 6 + .../saml/common/DefaultPicketLinkLogger.java | 2 +- .../saml/common/util/StaxParserUtil.java | 49 ++++- .../parsers/saml/SAMLAssertionParser.java | 5 +- .../parsers/saml/SAMLSloResponseParser.java | 1 - .../saml/common/util/StaxParserUtilTest.java | 178 ++++++++++++++++++ .../core/parsers/saml/SAMLParserTest.java | 8 +- .../parsers/saml/saml20-assertion-advice.xml | 145 ++++++++++++++ 9 files changed, 383 insertions(+), 13 deletions(-) create mode 100644 saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java create mode 100644 saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml diff --git a/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java b/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java index fd6acd4e4b..0a26062a7d 100755 --- a/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java +++ b/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java @@ -23,7 +23,7 @@ package org.keycloak.saml.common.constants; * @since Dec 10, 2008 */ public enum JBossSAMLConstants { - ADDRESS("Address"), ADDITIONAL_METADATA_LOCATION("AdditionalMetadataLocation"), AFFILIATION_DESCRIPTOR( + ADDRESS("Address"), ADVICE("Advice"), ADDITIONAL_METADATA_LOCATION("AdditionalMetadataLocation"), AFFILIATION_DESCRIPTOR( "AffiliationDescriptor"), ALLOW_CREATE("AllowCreate"), ARTIFACT("Artifact"), ARTIFACT_RESOLVE("ArtifactResolve"), ARTIFACT_RESPONSE( "ArtifactResponse"), ARTIFACT_RESOLUTION_SERVICE("ArtifactResolutionService"), ASSERTION("Assertion"), ASSERTION_CONSUMER_SERVICE( "AssertionConsumerService"), ASSERTION_CONSUMER_SERVICE_URL("AssertionConsumerServiceURL"), ASSERTION_CONSUMER_SERVICE_INDEX( diff --git a/saml-core/pom.xml b/saml-core/pom.xml index 77e283e2c1..f3041f7da6 100755 --- a/saml-core/pom.xml +++ b/saml-core/pom.xml @@ -65,6 +65,12 @@ 1.3 test + + commons-io + commons-io + 2.6 + test + diff --git a/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java b/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java index 2f58911090..b25b55b517 100755 --- a/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java +++ b/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java @@ -407,7 +407,7 @@ public class DefaultPicketLinkLogger implements PicketLinkLogger { */ @Override public ParsingException parserExpectedEndTag(String tagName) { - return new ParsingException(ErrorCodes.EXPECTED_END_TAG + "RequestAbstract or XACMLAuthzDecisionQuery"); + return new ParsingException(ErrorCodes.EXPECTED_END_TAG + tagName); } /* diff --git a/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java b/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java index f2fa728025..8667a90183 100755 --- a/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java @@ -49,6 +49,7 @@ import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; import java.io.InputStream; +import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -75,7 +76,9 @@ public class StaxParserUtil { } /** - * Bypass an entire XML element block from startElement to endElement + * Bypass an entire XML element block from startElement to endElement. + * It is expected that the {@code xmlEventReader} is positioned at (has not yet read) + * the start element of the block it should bypass. * * @param xmlEventReader * @param tag Tag of the XML element that we need to bypass @@ -83,16 +86,48 @@ public class StaxParserUtil { * @throws org.keycloak.saml.common.exceptions.ParsingException */ public static void bypassElementBlock(XMLEventReader xmlEventReader, String tag) throws ParsingException { - while (xmlEventReader.hasNext()) { - EndElement endElement = getNextEndElement(xmlEventReader); - if (endElement == null) - return; + XMLEvent xmlEvent = bypassElementBlock(xmlEventReader); - if (StaxParserUtil.matches(endElement, tag)) - return; + if (! (xmlEvent instanceof EndElement) || ! Objects.equals(((EndElement) xmlEvent).getName().getLocalPart(), tag)) { + throw logger.parserExpectedEndTag(tag); } } + /** + * Bypass an entire XML element block. + * It is expected that the {@code xmlEventReader} is positioned at (has not yet read) + * the start element of the block it should bypass. + * + * @param xmlEventReader + * @returns Last XML event which is {@link EndElement} corresponding to the first startElement when no error occurs ({@code null} if not available) + * + * @throws org.keycloak.saml.common.exceptions.ParsingException + */ + public static XMLEvent bypassElementBlock(XMLEventReader xmlEventReader) throws ParsingException { + XMLEvent xmlEvent; + int levelOfNesting = 0; + if (! xmlEventReader.hasNext()) { + return null; + } + + try { + do { + xmlEvent = xmlEventReader.nextEvent(); + if (xmlEvent instanceof StartElement) { + levelOfNesting++; + } else if (xmlEvent instanceof EndElement) { + levelOfNesting--; + } + } while (levelOfNesting > 0 && xmlEventReader.hasNext()); + } catch (XMLStreamException e) { + throw logger.parserException(e); + } + + return xmlEvent; + } + + + /** * Advances reader if character whitespace encountered * 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 80e6956266..057a2b7c31 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 @@ -34,7 +34,6 @@ import org.keycloak.saml.common.exceptions.ProcessingException; import org.keycloak.saml.common.parsers.ParserNamespaceSupport; import org.keycloak.saml.common.util.DocumentUtil; import org.keycloak.saml.common.util.StaxParserUtil; -import org.keycloak.saml.common.util.StringUtil; import org.keycloak.saml.processing.core.parsers.util.SAMLParserUtil; import org.keycloak.saml.processing.core.saml.v2.util.XMLTimeUtil; import org.w3c.dom.Element; @@ -42,7 +41,6 @@ import org.w3c.dom.Element; import javax.xml.datatype.XMLGregorianCalendar; import javax.xml.namespace.QName; import javax.xml.stream.XMLEventReader; -import javax.xml.stream.events.Attribute; import javax.xml.stream.events.EndElement; import javax.xml.stream.events.StartElement; import javax.xml.stream.events.XMLEvent; @@ -131,6 +129,9 @@ public class SAMLAssertionParser implements ParserNamespaceSupport { ConditionsType conditions = (ConditionsType) conditionsParser.parse(xmlEventReader); assertion.setConditions(conditions); + } else if (JBossSAMLConstants.ADVICE.get().equalsIgnoreCase(tag)) { + StaxParserUtil.bypassElementBlock(xmlEventReader); + logger.debug("SAML Advice tag is ignored"); } else if (JBossSAMLConstants.AUTHN_STATEMENT.get().equalsIgnoreCase(tag)) { AuthnStatementType authnStatementType = SAMLParserUtil.parseAuthnStatement(xmlEventReader); assertion.addStatement(authnStatementType); diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java index abee01e146..e63860cbb4 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java @@ -58,7 +58,6 @@ public class SAMLSloResponseParser extends SAMLStatusResponseTypeParser implemen issuer.setValue(StaxParserUtil.getElementText(xmlEventReader)); response.setIssuer(issuer); } else if (JBossSAMLConstants.SIGNATURE.get().equals(elementName)) { - startElement = StaxParserUtil.getNextStartElement(xmlEventReader); StaxParserUtil.bypassElementBlock(xmlEventReader, JBossSAMLConstants.SIGNATURE.get()); } else if (JBossSAMLConstants.EXTENSIONS.get().equals(elementName)) { SAMLExtensionsParser extensionsParser = new SAMLExtensionsParser(); diff --git a/saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java b/saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java new file mode 100644 index 0000000000..77bf1a84b5 --- /dev/null +++ b/saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java @@ -0,0 +1,178 @@ +/* + * Copyright 2017 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.saml.common.util; + +import org.keycloak.saml.common.exceptions.ParsingException; +import java.nio.charset.Charset; +import javax.xml.stream.XMLEventReader; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.events.Characters; +import javax.xml.stream.events.EndElement; +import javax.xml.stream.events.StartDocument; +import javax.xml.stream.events.StartElement; +import javax.xml.stream.events.XMLEvent; +import org.apache.commons.io.IOUtils; +import org.hamcrest.Matcher; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import static org.junit.Assert.assertThat; +import static org.hamcrest.CoreMatchers.*; + +/** + * + * @author hmlnarik + */ +public class StaxParserUtilTest { + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + private void assertStartTag(XMLEvent event, String tagName) { + assertThat(event, instanceOf(StartElement.class)); + assertThat(((StartElement) event).getName().getLocalPart(), is(tagName)); + } + + private void assertEndTag(XMLEvent event, String tagName) { + assertThat(event, instanceOf(EndElement.class)); + assertThat(((EndElement) event).getName().getLocalPart(), is(tagName)); + } + + private void assertCharacters(XMLEvent event, Matcher matcher) { + assertThat(event, instanceOf(Characters.class)); + assertThat(((Characters) event).getData(), matcher); + } + + @Test + public void testBypassElementBlock() throws XMLStreamException, ParsingException { + String xml = "test" + + "aa"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + StaxParserUtil.bypassElementBlock(reader, "d"); + + assertEndTag(reader.nextEvent(), "b"); + assertEndTag(reader.nextEvent(), "a"); + } + + @Test + public void testBypassElementBlockAnon() throws XMLStreamException, ParsingException { + String xml = "test" + + "aa"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + StaxParserUtil.bypassElementBlock(reader); + + assertEndTag(reader.nextEvent(), "b"); + assertEndTag(reader.nextEvent(), "a"); + } + + @Test + public void testBypassElementBlockNested() throws XMLStreamException, ParsingException { + String xml = "test" + + "aanestedD"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + StaxParserUtil.bypassElementBlock(reader, "d"); + + assertEndTag(reader.nextEvent(), "b"); + assertEndTag(reader.nextEvent(), "a"); + } + + @Test + public void testBypassElementBlockNestedAnon() throws XMLStreamException, ParsingException { + String xml = "test" + + "aanestedD"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + StaxParserUtil.bypassElementBlock(reader); + + assertEndTag(reader.nextEvent(), "b"); + assertEndTag(reader.nextEvent(), "a"); + } + + @Test + public void testBypassElementBlockWrongPairing() throws XMLStreamException, ParsingException { + String xml = "test" + + "aanestedD"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + expectedException.expect(ParsingException.class); + StaxParserUtil.bypassElementBlock(reader, "d"); + } + + @Test + public void testBypassElementBlockNestedPrematureEnd() throws XMLStreamException, ParsingException { + String xml = "test" + + "aanestedD"; + XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset())); + + assertThat(reader.nextEvent(), instanceOf(StartDocument.class)); + + assertStartTag(reader.nextEvent(), "a"); + assertStartTag(reader.nextEvent(), "b"); + assertStartTag(reader.nextEvent(), "c"); + assertCharacters(reader.nextEvent(), is("test")); + assertEndTag(reader.nextEvent(), "c"); + + StaxParserUtil.bypassElementBlock(reader, "d"); + + expectedException.expect(XMLStreamException.class); + reader.nextEvent(); + } + +} 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 81ebbe068f..5990ec4e12 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 @@ -34,7 +34,6 @@ 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; @@ -359,6 +358,13 @@ public class SAMLParserTest { } } + @Test + public void testSaml20AssertionsAdviceTag() throws IOException, ParsingException { + try (InputStream st = SAMLParserTest.class.getResourceAsStream("saml20-assertion-advice.xml")) { + 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); diff --git a/saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml b/saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml new file mode 100644 index 0000000000..54b5c33734 --- /dev/null +++ b/saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml @@ -0,0 +1,145 @@ + + + https://www.salesforce.com + + + + + + + + + + + + + + + vzR9Hfp8d16576tEDeq/zhpmLoo= + + + + + AzID5hhJeJlG2llUDvZswNUrlrPtR7S37QYH2W+Un1n8c6kTC + Xr/lihEKPcA2PZt86eBntFBVDWTRlh/W3yUgGOqQBJMFOVbhK + M/CbLHbBUVT5TcxIqvsNvIFdjIGNkf1W0SBqRKZOJ6tzxCcLo + 9dXqAyAUkqDpX5+AyltwrdCPNmncUM4dtRPjI05CL1rRaGeyX + 3kkqOL8p0vjm0fazU5tCAJLbYuYgU1LivPSahWNcpvRSlCI4e + Pn2oiVDyrcc4et12inPMTc2lGIWWWWJyHOPSiXRSkEAIwQVjf + Qm5cpli44Pv8FCrdGWpEE0yXsPBvDkM9jIzwCYGG2fKaLBag== + + + + + MIIEATCCAumgAwIBAgIBBTANBgkqhkiG9w0BAQ0FADCBgzELM + [Certificate truncated for readability...] + + + + + + + + saml01@salesforce.com + + + + + + + + + + + https://saml.salesforce.com + + + + + NCName + + + + NCName + + nested + element text + + + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified + + + + + + + + 060D00000000SHZ + + + + + + + 00DD0000000F7L5 + + + + + + + + + XYZ + + + + + + + true + + + + + + val2 + + + + + + value_no_xml + + + + + + http://www.salesforce.com/security/saml/saml20-gen.jsp + + + + + + + http://www.salesforce.com/security/del_auth/SsoLogoutPage.html + + + + + + + + + +