From 5c2122d36f1a41449ab414e0fcdebc92b04015db Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Mon, 23 Nov 2020 21:50:07 +0100 Subject: [PATCH] KEYCLOAK-16444 Initialize JAXP components consistently --- .../core/util/JAXPValidationUtil.java | 52 +++++++++-- .../core/util/JAXPValidationUtilTest.java | 76 ++++++++++++++++ .../undertow/UndertowAppServer.java | 9 +- .../testsuite/saml/SamlXMLAttacksTest.java | 89 +++++++++++++++++++ 4 files changed, 218 insertions(+), 8 deletions(-) create mode 100644 saml-core/src/test/java/org/keycloak/saml/processing/core/util/JAXPValidationUtilTest.java diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/JAXPValidationUtil.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/JAXPValidationUtil.java index 49d531cb53..a1405a6d62 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/JAXPValidationUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/JAXPValidationUtil.java @@ -22,6 +22,7 @@ import org.keycloak.saml.common.constants.GeneralConstants; import org.keycloak.saml.common.exceptions.ProcessingException; import org.keycloak.saml.common.util.DocumentUtil; import org.keycloak.saml.common.util.SecurityActions; +import org.keycloak.saml.common.util.StaxParserUtil; import org.keycloak.saml.common.util.SystemPropertiesUtil; import org.w3c.dom.Node; import org.xml.sax.ErrorHandler; @@ -37,6 +38,14 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; import java.util.List; +import javax.xml.XMLConstants; +import javax.xml.stream.XMLStreamException; +import javax.xml.transform.stax.StAXSource; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; +import static org.keycloak.saml.common.util.DocumentUtil.feature_disallow_doctype_decl; +import static org.keycloak.saml.common.util.DocumentUtil.feature_external_general_entities; +import static org.keycloak.saml.common.util.DocumentUtil.feature_external_parameter_entities; /** * Utility class associated with JAXP Validation @@ -52,12 +61,12 @@ public class JAXPValidationUtil { protected static SchemaFactory schemaFactory; - public static void validate(String str) throws SAXException, IOException { - validator().validate(new StreamSource(str)); - } - public static void validate(InputStream stream) throws SAXException, IOException { - validator().validate(new StreamSource(stream)); + try { + validator().validate(new StAXSource(StaxParserUtil.getXMLEventReader(stream))); + } catch (XMLStreamException ex) { + throw new IOException(ex); + } } /** @@ -86,11 +95,42 @@ public class JAXPValidationUtil { throw logger.nullValueError("schema"); validator = schema.newValidator(); + // Do not optimize the following into setProperty(...) && setProperty(...). + // This way if it fails in the first setProperty, it will try the subsequent setProperty anyway + // which it would not due to short-circuiting in case of an && expression. + boolean successful1 = setProperty(validator, XMLConstants.ACCESS_EXTERNAL_DTD, ""); + successful1 &= setProperty(validator, XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + boolean successful2 = setFeature(validator, feature_disallow_doctype_decl, true); + successful2 &= setFeature(validator, feature_external_general_entities, false); + successful2 &= setFeature(validator, feature_external_parameter_entities, false); + if (! successful1 && ! successful2) { + logger.warn("Cannot disable external access in XML validator"); + } validator.setErrorHandler(new CustomErrorHandler()); } return validator; } + private static boolean setProperty(Validator v, String property, String value) { + try { + v.setProperty(property, value); + } catch (SAXNotRecognizedException | SAXNotSupportedException ex) { + logger.debug("Cannot set " + property); + return false; + } + return true; + } + + private static boolean setFeature(Validator v, String feature, boolean value) { + try { + v.setFeature(feature, value); + } catch (SAXNotRecognizedException | SAXNotSupportedException ex) { + logger.debug("Cannot set " + feature); + return false; + } + return true; + } + private static Schema getSchema() throws IOException { boolean tccl_jaxp = SystemPropertiesUtil.getSystemProperty(GeneralConstants.TCCL_JAXP, "false").equalsIgnoreCase("true"); @@ -99,7 +139,7 @@ public class JAXPValidationUtil { if (tccl_jaxp) { SecurityActions.setTCCL(JAXPValidationUtil.class.getClassLoader()); } - schemaFactory = SchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema"); + schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); schemaFactory.setResourceResolver(new IDFedLSInputResolver()); schemaFactory.setErrorHandler(new CustomErrorHandler()); diff --git a/saml-core/src/test/java/org/keycloak/saml/processing/core/util/JAXPValidationUtilTest.java b/saml-core/src/test/java/org/keycloak/saml/processing/core/util/JAXPValidationUtilTest.java new file mode 100644 index 0000000000..9e5f7aa4e7 --- /dev/null +++ b/saml-core/src/test/java/org/keycloak/saml/processing/core/util/JAXPValidationUtilTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2020 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.processing.core.util; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import org.hamcrest.Matcher; +import org.hamcrest.Matchers; +import org.junit.Test; +import org.xml.sax.SAXException; +import static org.junit.Assert.assertThat; + +/** + * + * @author hmlnarik + */ +public class JAXPValidationUtilTest { + + private static final String REQUEST_VALID = "" + + "urn:test" + + ""; + + private static final String REQUEST_FLAWED = "" + + "urn:test" + + ""; + + private static final String REQUEST_FLAWED_LOCAL = "" + + "urn:test" + + ""; + + private static final String REQUEST_INVALID = "" + + "urn:test" + + ""; + + + @Test + public void testServerSideValidator() throws Exception { + String preamble = "" + + "" + + "]>"; + + assertInputValidation(REQUEST_VALID, Matchers.nullValue()); + + assertInputValidation(REQUEST_INVALID, Matchers.notNullValue()); + assertInputValidation(preamble + REQUEST_FLAWED, Matchers.notNullValue()); + assertInputValidation(preamble + REQUEST_FLAWED_LOCAL, Matchers.notNullValue()); + assertInputValidation(preamble + "", Matchers.notNullValue()); + } + + private void assertInputValidation(String s, Matcher matcher) { + String validationResult = null; + try { + JAXPValidationUtil.validate(new ByteArrayInputStream(s.getBytes())); + } catch (SAXException | IOException ex) { + validationResult = ex.getMessage(); + } +// log.debugf("Validation result: '%s' for: %s", validationResult, s); + assertThat(s, validationResult, matcher); + } + +} diff --git a/testsuite/integration-arquillian/servers/app-server/undertow/src/main/java/org/keycloak/testsuite/arquillian/undertow/UndertowAppServer.java b/testsuite/integration-arquillian/servers/app-server/undertow/src/main/java/org/keycloak/testsuite/arquillian/undertow/UndertowAppServer.java index 4c0c46e95c..2c54684116 100644 --- a/testsuite/integration-arquillian/servers/app-server/undertow/src/main/java/org/keycloak/testsuite/arquillian/undertow/UndertowAppServer.java +++ b/testsuite/integration-arquillian/servers/app-server/undertow/src/main/java/org/keycloak/testsuite/arquillian/undertow/UndertowAppServer.java @@ -61,6 +61,7 @@ import org.keycloak.common.util.reflections.Reflections; import org.keycloak.testsuite.arquillian.undertow.saml.util.RestSamlApplicationConfig; import org.keycloak.testsuite.utils.undertow.UndertowDeployerHelper; import org.keycloak.testsuite.utils.undertow.UndertowWarClassLoader; +import java.io.InputStream; /** * @author Vlasta Ramik @@ -229,8 +230,12 @@ public class UndertowAppServer implements DeployableContainer\n" + + "\">\n" + + "%eval;\n" + + "%error;"; + + return ShrinkWrap.create(WebArchive.class, "dtd.war") + .add(new StringAsset(attackerDtd), "/attacker.dtd"); + } + + private void assertBlackboxInputValidation(String s, Matcher matcher) throws IOException, RuntimeException { + try (CloseableHttpClient client = HttpClientBuilder.create().build()) { + HttpPost post = new HttpPost(getAuthServerSamlEndpoint(REALM_NAME)); + + List parameters = new LinkedList<>(); + String encoded = PostBindingUtil.base64Encode(s); + parameters.add(new BasicNameValuePair(GeneralConstants.SAML_REQUEST_KEY, encoded)); + + UrlEncodedFormEntity formEntity; + try { + formEntity = new UrlEncodedFormEntity(parameters, "UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + + post.setEntity(formEntity); + + try (CloseableHttpResponse response = client.execute(post)) { + assertThat(response, matcher); + } + } + } + + @Test + public void testValidator(@ArquillianResource @OperateOnDeployment("DTD") URL attackerDtdUrl) throws Exception { + String preamble = "" + + "%sp;" + + "" + + "]>".replaceAll("//attacker", "/attacker"); + + assertBlackboxInputValidation(REQUEST_VALID, statusCodeIsHC(Response.Status.FOUND)); + + assertBlackboxInputValidation(REQUEST_INVALID, bodyHC(containsString("Invalid Request"))); + assertBlackboxInputValidation(preamble + REQUEST_VALID, bodyHC(containsString("Invalid Request"))); + assertBlackboxInputValidation(preamble + REQUEST_FLAWED, bodyHC(containsString("Invalid Request"))); + assertBlackboxInputValidation(preamble + REQUEST_FLAWED_LOCAL, bodyHC(containsString("Invalid Request"))); + assertBlackboxInputValidation(preamble + "", bodyHC(containsString("Invalid Request"))); + } + + private static final String REQUEST_VALID = "" + + "" + SAML_CLIENT_ID_SALES_POST + "" + + ""; + + private static final String REQUEST_FLAWED = "" + + "" + SAML_CLIENT_ID_SALES_POST + "" + + ""; + + private static final String REQUEST_FLAWED_LOCAL = "" + + "" + SAML_CLIENT_ID_SALES_POST + "" + + ""; + + private static final String REQUEST_INVALID = "" + + "" + SAML_CLIENT_ID_SALES_POST + "" + + ""; + }