Skip to content

Commit

Permalink
Fixes for XXE vulnerabilities (MID-5285)
Browse files Browse the repository at this point in the history
  • Loading branch information
semancik committed Apr 17, 2019
1 parent da4557b commit 18c5f46
Show file tree
Hide file tree
Showing 7 changed files with 149 additions and 10 deletions.
Expand Up @@ -187,6 +187,11 @@ public void validate(InputStream inputStream, OperationResult validatorResult, S
stream = xmlInputFactory.createXMLStreamReader(inputStream);

int eventType = stream.nextTag();
if (eventType == XMLStreamConstants.DTD || eventType == XMLStreamConstants.ENTITY_DECLARATION
|| eventType == XMLStreamConstants.ENTITY_REFERENCE || eventType == XMLStreamConstants.NOTATION_DECLARATION) {
// We do not want those, e.g. we want to void XXE vulnerabilities. Make this check explicit.
throw new SystemException("Use of "+eventType+" in XML is prohibited");
}
if (eventType == XMLStreamConstants.START_ELEMENT) {
if (!QNameUtil.match(stream.getName(), SchemaConstants.C_OBJECTS)) {
// This has to be an import file with a single objects. Try
Expand Down
Expand Up @@ -103,6 +103,14 @@ public List<RootXNode> readObjects(@NotNull ParserSource source, @NotNull Parsin
}
}
}

private XMLInputFactory getXMLInputFactory() {
XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
xmlInputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
// TODO: cache? static? prism context?
return xmlInputFactory;
}

// code taken from Validator class
@Override
Expand All @@ -111,7 +119,7 @@ public void readObjectsIteratively(@NotNull ParserSource source, @NotNull Parsin
InputStream is = source.getInputStream();
XMLStreamReader stream = null;
try {
stream = XMLInputFactory.newInstance().createXMLStreamReader(is);
stream = getXMLInputFactory().createXMLStreamReader(is);

int eventType = stream.nextTag();
if (eventType != XMLStreamConstants.START_ELEMENT) {
Expand Down
Expand Up @@ -595,10 +595,17 @@ private void init() throws ParserConfigurationException {
LOGGER.trace("Using namespace prefix mapper to serialize schema:\n{}",DebugUtil.dump(namespacePrefixMapper));
}

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
dbf.setNamespaceAware(true);
dbf.setValidating(false);
DocumentBuilder db = dbf.newDocumentBuilder();
DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
documentBuilderFactory.setNamespaceAware(true);
documentBuilderFactory.setValidating(false);
// XXE
documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
documentBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
documentBuilderFactory.setXIncludeAware(false);
documentBuilderFactory.setExpandEntityReferences(false);
DocumentBuilder db = documentBuilderFactory.newDocumentBuilder();

document = db.newDocument();
Element root = createElement(new QName(W3C_XML_SCHEMA_NS_URI, "schema"));
Expand Down
@@ -1,10 +1,18 @@
package com.evolveum.midpoint.prism;

import static com.evolveum.midpoint.prism.PrismInternalTestUtil.*;
import static org.testng.AssertJUnit.assertTrue;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.USER_JACK_ADHOC_BASENAME;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.USER_JACK_FILE_BASENAME;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.USER_JACK_OID;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.assertPropertyValue;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.assertUserJack;
import static com.evolveum.midpoint.prism.PrismInternalTestUtil.constructInitializedPrismContext;
import static org.testng.AssertJUnit.assertEquals;
import static org.testng.AssertJUnit.assertNotNull;

import java.io.IOException;

import org.testng.AssertJUnit;
import org.testng.annotations.Test;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
Expand All @@ -15,6 +23,8 @@

public class TestPrismParsingXml extends TestPrismParsing {

public static final String USER_JACK_XXE_BASENAME = "user-jack-xxe";

@Override
protected String getSubdirName() {
return "xml";
Expand Down Expand Up @@ -73,6 +83,44 @@ public void testPrismParseDomAdhoc() throws Exception {

assertUserAdhoc(user, true);
}

@Test
public void testPrismParseXxe() throws Exception {
final String TEST_NAME = "testPrismParseXxe";
PrismInternalTestUtil.displayTestTitle(TEST_NAME);

PrismContext prismContext = constructInitializedPrismContext();

try {
// WHEN
prismContext.parseObject(getFile(USER_JACK_XXE_BASENAME));

AssertJUnit.fail("Unexpected success");
} catch (IllegalStateException e) {
// THEN
System.out.println("Expected exception: "+e);
assertTrue("Unexpected exception message: "+e.getMessage(), e.getMessage().contains("DOCTYPE"));
}

}

@Test
public void testPrismParseDomXxe() throws Exception {
final String TEST_NAME = "testPrismParseDomXxe";
PrismInternalTestUtil.displayTestTitle(TEST_NAME);

try {
// WHEN
DOMUtil.parseFile(getFile(USER_JACK_XXE_BASENAME));

AssertJUnit.fail("Unexpected success");
} catch (IllegalStateException e) {
// THEN
System.out.println("Expected exception: "+e);
assertTrue("Unexpected exception message: "+e.getMessage(), e.getMessage().contains("DOCTYPE"));
}

}

@Override
protected void validateXml(String xmlString, PrismContext prismContext) throws SAXException, IOException {
Expand Down
35 changes: 35 additions & 0 deletions infra/prism/src/test/resources/common/xml/user-jack-xxe.xml
@@ -0,0 +1,35 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE ANY[ <!ENTITY file SYSTEM "file:///etc/hostname"> ]>
<!--
~ Copyright (c) 2010-2019 Evolveum
~
~ 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.
-->
<user oid="c0c010c0-d34d-b33f-f00d-111111111111" version="42"
xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance'
xmlns:xsd='http://www.w3.org/2001/XMLSchema'
xmlns='http://midpoint.evolveum.com/xml/ns/test/foo-1.xsd'
xmlns:ds="http://www.w3.org/2000/09/xmldsig#"
xmlns:enc="http://www.w3.org/2001/04/xmlenc#"
xmlns:a="http://prism.evolveum.com/xml/ns/public/annotation-3"
xmlns:t="http://prism.evolveum.com/xml/ns/public/types-3"
xmlns:q="http://prism.evolveum.com/xml/ns/public/query-3"
xmlns:adhoc="http://midpoint.evolveum.com/xml/ns/test/adhoc-1.xsd"
xmlns:ext="http://midpoint.evolveum.com/xml/ns/test/extension">
<name>jack</name>
<description>&file;</description>
<fullName>cpt. Jack Sparrow</fullName>
<givenName>Jack</givenName>
<familyName>Sparrow</familyName>

</user>
30 changes: 29 additions & 1 deletion infra/util/src/main/java/com/evolveum/midpoint/util/DOMUtil.java
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010-2013 Evolveum
* Copyright (c) 2010-2019 Evolveum
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -169,6 +169,13 @@ public class DOMUtil {
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setNamespaceAware(true);
// XXE
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
loader = factory.newDocumentBuilder();
} catch (ParserConfigurationException ex) {
throw new IllegalStateException("Error creating XML document " + ex.getMessage());
Expand Down Expand Up @@ -209,6 +216,13 @@ public static DocumentBuilder createDocumentBuilder() {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setNamespaceAware(true);
try {
// XXE
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
return factory.newDocumentBuilder();
} catch (ParserConfigurationException e) {
throw new IllegalStateException("Error creating document builder " + e.getMessage(), e);
Expand All @@ -232,6 +246,13 @@ public static Document parseFile(File file) {
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setNamespaceAware(true);
// XXE
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
DocumentBuilder loader = factory.newDocumentBuilder();
return loader.parse(file);
} catch (SAXException | IOException | ParserConfigurationException ex) {
Expand All @@ -249,6 +270,13 @@ public static Document parse(InputStream inputStream) throws IOException {
factory.setFeature("http://xml.org/sax/features/validation", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
// XXE
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setExpandEntityReferences(false);
DocumentBuilder loader = factory.newDocumentBuilder();
return loader.parse(inputStream);
} catch (SAXException | ParserConfigurationException ex) {
Expand Down
Expand Up @@ -510,9 +510,17 @@ public static String toString(PolyStringType poly) {

static {
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
factory.setNamespaceAware(true);
domDocumentBuilder = factory.newDocumentBuilder();
DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
documentBuilderFactory.setNamespaceAware(true);
// XXE
documentBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
documentBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
documentBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
documentBuilderFactory.setXIncludeAware(false);
documentBuilderFactory.setExpandEntityReferences(false);
domDocumentBuilder = documentBuilderFactory.newDocumentBuilder();

} catch (ParserConfigurationException e) {
throw new IllegalStateException("Error creating XML document " + e.getMessage());
}
Expand Down

0 comments on commit 18c5f46

Please sign in to comment.