[java-opensaml] branch master updated: OSJ-232: Additional protections in unmarshaller around comments/CDATA
Brent Putman
putmanb at georgetown.edu
Fri Sep 28 19:23:55 EDT 2018
This is an automated email from the git hooks/post-receive script.
putmanb pushed a commit to branch master
in repository java-opensaml.
View the commit online:
http://git.shibboleth.net/view/?p=java-opensaml.git;a=commit;h=5ebc88bd6f9f1458fa5a19df87b0885c70a8f474
The following commit(s) were added to refs/heads/master by this push:
new 5ebc88b OSJ-232: Additional protections in unmarshaller around comments/CDATA
5ebc88b is described below
commit 5ebc88bd6f9f1458fa5a19df87b0885c70a8f474
Author: Brent Putman <putmanb at georgetown.edu>
AuthorDate: Fri Sep 28 19:23:52 2018 -0400
OSJ-232: Additional protections in unmarshaller around comments/CDATA
Throw when see comment and CDATA nodes.
Also some related Checkstyle fixes.
---
.../core/xml/io/AbstractXMLObjectUnmarshaller.java | 23 +++-
.../core/xml/UnmarshallingSecurityTest.java | 147 +++++++++++++++++++++
.../core/xml/SimpleXMLObjectWithCDATAInContent.xml | 2 +
.../SimpleXMLObjectWithCommentBetweenChildren.xml | 6 +
.../xml/SimpleXMLObjectWithCommentInContent.xml | 2 +
5 files changed, 173 insertions(+), 7 deletions(-)
diff --git a/opensaml-core/src/main/java/org/opensaml/core/xml/io/AbstractXMLObjectUnmarshaller.java b/opensaml-core/src/main/java/org/opensaml/core/xml/io/AbstractXMLObjectUnmarshaller.java
index d9d390e..ede3797 100644
--- a/opensaml-core/src/main/java/org/opensaml/core/xml/io/AbstractXMLObjectUnmarshaller.java
+++ b/opensaml-core/src/main/java/org/opensaml/core/xml/io/AbstractXMLObjectUnmarshaller.java
@@ -50,11 +50,13 @@ import org.w3c.dom.Text;
* <li>Delegating to child classes element, text, and attribute processing</li>
* </ul>
*
- * <strong>WARNING:</strong> In the case of Text nodes this unmarshaller will use {@link org.w3c.dom.Text#getWholeText()}
- * to retrieve the content. This is acceptable if and only if our XML parsing classes are used in their default (safe)
- * configuration on the Java platforms we officially support. If you need to deal with elements that contain multiple
- * text node children, or you intend to rely on your own XML parser and/or JAXP implementation, you will need to override
- * {@link #unmarshallTextContent(XMLObject, Text)} and do "the right thing" for your implementation.
+ * <strong>WARNING:</strong> In the case of Text nodes this unmarshaller will use
+ * {@link org.w3c.dom.Text#getWholeText()} * to retrieve the content. This is acceptable
+ * if and only if our XML parsing classes are used in their default (safe) configuration
+ * on the Java platforms we officially support. If you need to deal with elements that contain multiple
+ * text node children, or you intend to rely on your own XML parser and/or JAXP implementation,
+ * you will need to override {@link #unmarshallTextContent(XMLObject, Text)} and do "the right thing"
+ * for your implementation.
*
* Failure to adhere to this warning will very likely lead to security bugs.
*/
@@ -78,6 +80,7 @@ public abstract class AbstractXMLObjectUnmarshaller implements Unmarshaller {
}
/** {@inheritDoc} */
+ // Checkstyle: CyclomaticComplexity OFF
@Override
@Nonnull public XMLObject unmarshall(@Nonnull final Element domElement) throws UnmarshallingException {
log.trace("Starting to unmarshall DOM element {}", QNameSupport.getNodeQName(domElement));
@@ -107,9 +110,14 @@ public abstract class AbstractXMLObjectUnmarshaller implements Unmarshaller {
unmarshallAttribute(xmlObject, (Attr) childNode);
} else if (childNode.getNodeType() == Node.ELEMENT_NODE) {
unmarshallChildElement(xmlObject, (Element) childNode);
- } else if (childNode.getNodeType() == Node.TEXT_NODE
- || childNode.getNodeType() == Node.CDATA_SECTION_NODE) {
+ } else if (childNode.getNodeType() == Node.TEXT_NODE) {
unmarshallTextContent(xmlObject, (Text) childNode);
+ } else if (childNode.getNodeType() == Node.CDATA_SECTION_NODE) {
+ throw new UnmarshallingException("Saw illegal CDATA node in parsed DOM, "
+ + "likely due to improper parser configuration");
+ } else if (childNode.getNodeType() == Node.COMMENT_NODE) {
+ throw new UnmarshallingException("Saw illegal Comment node in parsed DOM, "
+ + "likely due to improper parser configuration");
}
childNode = childNode.getNextSibling();
@@ -118,6 +126,7 @@ public abstract class AbstractXMLObjectUnmarshaller implements Unmarshaller {
xmlObject.setDOM(domElement);
return xmlObject;
}
+ // Checkstyle: CyclomaticComplexity ON
/**
* Constructs the XMLObject that the given DOM Element will be unmarshalled into. If the DOM element has an XML
diff --git a/opensaml-core/src/test/java/org/opensaml/core/xml/UnmarshallingSecurityTest.java b/opensaml-core/src/test/java/org/opensaml/core/xml/UnmarshallingSecurityTest.java
new file mode 100644
index 0000000..de53c51
--- /dev/null
+++ b/opensaml-core/src/test/java/org/opensaml/core/xml/UnmarshallingSecurityTest.java
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the University Corporation for Advanced Internet Development,
+ * Inc. (UCAID) under one or more contributor license agreements. See the
+ * NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The UCAID licenses this file to You 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.opensaml.core.xml;
+
+import org.opensaml.core.xml.io.MarshallingException;
+import org.opensaml.core.xml.io.Unmarshaller;
+import org.opensaml.core.xml.io.UnmarshallingException;
+import org.opensaml.core.xml.mock.SimpleXMLObject;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+import org.w3c.dom.Document;
+
+import net.shibboleth.utilities.java.support.component.ComponentInitializationException;
+import net.shibboleth.utilities.java.support.xml.BasicParserPool;
+import net.shibboleth.utilities.java.support.xml.XMLParserException;
+
+/**
+ * Unit test for unmarshalling functions.
+ */
+public class UnmarshallingSecurityTest extends XMLObjectBaseTestCase {
+
+ private BasicParserPool parserPoolDefaults, parserPoolInsecure;
+
+ @BeforeClass
+ public void setup() throws ComponentInitializationException {
+ parserPoolDefaults = new BasicParserPool();
+ parserPoolDefaults.initialize();
+
+ parserPoolInsecure = new BasicParserPool();
+ parserPoolInsecure.setIgnoreComments(false);
+ parserPoolInsecure.setCoalescing(false);
+ parserPoolInsecure.initialize();
+ }
+
+ /**
+ * Tests unmarshalling an element with comment in content with default parser.
+ *
+ * @throws XMLParserException
+ * @throws UnmarshallingException
+ */
+ @Test
+ public void testUnmarshallingWithCommentInElementContentDefaults() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCommentInContent.xml";
+ Document document = parserPoolDefaults.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+
+ Assert.assertEquals(sxObject.getValue(), "Content1");
+ }
+
+ /**
+ * Tests unmarshalling an element with comment in content with insecure parser.
+ *
+ * @throws XMLParserException
+ * @throws UnmarshallingException
+ */
+ @Test(expectedExceptions=UnmarshallingException.class)
+ public void testUnmarshallingWithCommentInElementContentInsecure() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCommentInContent.xml";
+ Document document = parserPoolInsecure.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+ }
+
+ /**
+ * Tests unmarshalling an element with CDATA in content with default parser.
+ *
+ * @throws XMLParserException
+ * @throws UnmarshallingException
+ */
+ @Test
+ public void testUnmarshallingWithCDATAInElementContentDefaults() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCDATAInContent.xml";
+ Document document = parserPoolDefaults.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+
+ Assert.assertEquals(sxObject.getValue(), "Content1");
+ }
+
+ /**
+ * Tests unmarshalling an element with CDATA in content with insecure parser.
+ *
+ * @throws XMLParserException
+ * @throws UnmarshallingException
+ */
+ @Test(expectedExceptions=UnmarshallingException.class)
+ public void testUnmarshallingWithCDATAInElementContentInsecure() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCDATAInContent.xml";
+ Document document = parserPoolInsecure.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+ }
+
+ /**
+ * Tests unmarshalling an element with comment between child elements with default parser.
+ *
+ * @throws XMLParserException
+ * @throws MarshallingException
+ */
+ @Test
+ public void testUnmarshallingWithCommentBetweenChildElementsDefaults() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCommentBetweenChildren.xml";
+ Document document = parserPoolDefaults.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+
+ Assert.assertEquals(sxObject.getSimpleXMLObjects().size(), 2, "Number of children elements was not expected value");
+ }
+
+ /**
+ * Tests unmarshalling an element with comment between child elements with insecure parser.
+ *
+ * @throws XMLParserException
+ * @throws MarshallingException
+ */
+ @Test(expectedExceptions=UnmarshallingException.class)
+ public void testUnmarshallingWithCommentBetweenChildElementsInsecure() throws XMLParserException, UnmarshallingException {
+ String documentLocation = "/org/opensaml/core/xml/SimpleXMLObjectWithCommentBetweenChildren.xml";
+ Document document = parserPoolInsecure.parse(UnmarshallingSecurityTest.class.getResourceAsStream(documentLocation));
+
+ Unmarshaller unmarshaller = unmarshallerFactory.getUnmarshaller(document.getDocumentElement());
+ SimpleXMLObject sxObject = (SimpleXMLObject) unmarshaller.unmarshall(document.getDocumentElement());
+ }
+
+}
\ No newline at end of file
diff --git a/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCDATAInContent.xml b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCDATAInContent.xml
new file mode 100644
index 0000000..0a010fe
--- /dev/null
+++ b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCDATAInContent.xml
@@ -0,0 +1,2 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test:SimpleElement xmlns:test="http://www.example.org/testObjects">Con<![CDATA[tent]]>1</test:SimpleElement>
diff --git a/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentBetweenChildren.xml b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentBetweenChildren.xml
new file mode 100644
index 0000000..49ae907
--- /dev/null
+++ b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentBetweenChildren.xml
@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test:SimpleElement xmlns:test="http://www.example.org/testObjects">
+ <test:SimpleElement/>
+ <!-- blah blah -->
+ <test:SimpleElement/>
+</test:SimpleElement>
diff --git a/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentInContent.xml b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentInContent.xml
new file mode 100644
index 0000000..ba26d6e
--- /dev/null
+++ b/opensaml-core/src/test/resources/org/opensaml/core/xml/SimpleXMLObjectWithCommentInContent.xml
@@ -0,0 +1,2 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test:SimpleElement xmlns:test="http://www.example.org/testObjects">Con<!-- blarty flart -->tent1</test:SimpleElement>
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list