Skip to content

Harden XML parsing via commons-secure-xml #393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ppkarwasz wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Harden XML parsing via commons-secure-xml #393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/codeql-analysis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ jobs:
# If this step fails, then you should remove it and run the build manually (see below)
- name: Autobuild
uses: github/codeql-action/autobuild@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9
env:
MAVEN_ARGS: -Puse-apache-snapshots

# 鈩癸笍 Command-line programs to run using the OS shell.
# 馃摎 https://git.io/JvXDl
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/maven.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,4 @@ jobs:
distribution: 'temurin'
java-version: ${{ matrix.java }}
- name: Build with Maven
run: mvn --errors --show-version --batch-mode --no-transfer-progress
run: mvn --errors --show-version --batch-mode --no-transfer-progress -Puse-apache-snapshots
5 changes: 5 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@
</contributors>

<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-secure-xml</artifactId>
<version>1.0.0-SNAPSHOT</version>
</dependency>
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
Expand Down
2 changes: 2 additions & 0 deletions src/changes/changes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
[18-10-2018] Before executing invoke handlers after a macrostep all internal events must have been processed
</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Fix Apache RAT plugin console warnings.</action>
<action type="fix" dev="pkarwasz">Create XML parsers, stream readers and transformers through org.apache.commons:commons-secure-xml, so external entities and DTDs are no longer fetched by default.</action>
<action type="fix" dev="pkarwasz">ContentParser.parseXml now parses its argument as XML content instead of interpreting it as a URI.</action>
<!-- UPDATE -->
<action dev="woonsan" type="update" issue="SCXML-284" due-to="Allon Mureinik">
[10-10-2018] Clear up exception handling in tests
Expand Down
11 changes: 7 additions & 4 deletions src/main/java/org/apache/commons/scxml2/io/ContentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,16 @@

import java.io.IOException;
import java.io.InputStream;
import java.io.StringReader;
import java.io.StringWriter;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.Properties;

import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;

Expand All @@ -37,8 +36,11 @@
import org.apache.commons.scxml2.model.NodeValue;
import org.apache.commons.scxml2.model.ParsedValue;
import org.apache.commons.scxml2.model.TextValue;
import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
import org.apache.commons.xml.secure.SecureTransformerFactory;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;

import com.fasterxml.jackson.core.JsonParser;
Expand Down Expand Up @@ -224,7 +226,8 @@ public ParsedValue parseResource(final String resourceURL) throws IOException {
public Node parseXml(final String xmlString) throws IOException {
Document doc;
try {
doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(xmlString);
// Wrap in an InputSource: DocumentBuilder.parse(String) would interpret the content as a URI.
doc = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new InputSource(new StringReader(xmlString)));
} catch (SAXException | ParserConfigurationException e) {
throw new IOException(e);
}
Expand Down Expand Up @@ -252,7 +255,7 @@ public String toJson(final Object jsonObject) throws IOException {
public String toXml(final Node node) throws IOException {
try {
final StringWriter writer = new StringWriter();
final Transformer transformer = TransformerFactory.newInstance().newTransformer();
final Transformer transformer = SecureTransformerFactory.newInstance().newTransformer();
final Properties outputProps = new Properties();
outputProps.put(OutputKeys.OMIT_XML_DECLARATION, "no");
outputProps.put(OutputKeys.STANDALONE, "no");
Expand Down
12 changes: 7 additions & 5 deletions src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import java.util.List;
import java.util.Map;

import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.stream.Location;
import javax.xml.stream.XMLInputFactory;
Expand Down Expand Up @@ -98,6 +97,9 @@
import org.apache.commons.scxml2.model.TransitionType;
import org.apache.commons.scxml2.model.TransitionalState;
import org.apache.commons.scxml2.model.Var;
import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
import org.apache.commons.xml.secure.SecureSchemaFactory;
import org.apache.commons.xml.secure.SecureXMLInputFactory;
import org.w3c.dom.Attr;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
Expand Down Expand Up @@ -589,9 +591,9 @@ private static XMLStreamReader getReader(final Configuration configuration, fina
throws IOException, XMLStreamException {

// Instantiate the XMLInputFactory
XMLInputFactory factory = XMLInputFactory.newInstance();
XMLInputFactory factory = SecureXMLInputFactory.newInstance();
if (configuration.factoryId != null && configuration.factoryClassLoader != null) {
factory = XMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader);
factory = SecureXMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader);
}
factory.setEventAllocator(configuration.allocator);
if (factory.isPropertySupported(XMLInputFactory_JDK_PROP_REPORT_CDATA)) {
Expand Down Expand Up @@ -623,7 +625,7 @@ private static XMLStreamReader getReader(final Configuration configuration, fina
// Validation requires us to use a Source

final URL scxmlSchema = new URL("TODO"); // TODO, point to appropriate location
final SchemaFactory schemaFactory = SchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema");
final SchemaFactory schemaFactory = SecureSchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema");
Schema schema;
try {
schema = schemaFactory.newSchema(scxmlSchema);
Expand Down Expand Up @@ -1341,7 +1343,7 @@ private static Element readElement(final XMLStreamReader reader)
// Create a document in which to build the DOM node
Document document;
try {
document = DocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument();
document = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument();
} catch (final ParserConfigurationException pce) {
throw new XMLStreamException(ERR_PARSER_CFG);
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
import org.apache.commons.scxml2.model.Transition;
import org.apache.commons.scxml2.model.TransitionTarget;
import org.apache.commons.scxml2.model.Var;
import org.apache.commons.xml.secure.SecureTransformerFactory;
import org.w3c.dom.Node;

/**
Expand Down Expand Up @@ -339,7 +340,7 @@ private static Transformer getTransformer() {
outputProps.put(OutputKeys.STANDALONE, "no");
outputProps.put(OutputKeys.INDENT, "yes");
try {
final TransformerFactory tfFactory = TransformerFactory.newInstance();
final TransformerFactory tfFactory = SecureTransformerFactory.newInstance();
transformer = tfFactory.newTransformer();
transformer.setOutputProperties(outputProps);
} catch (TransformerFactoryConfigurationError | TransformerConfigurationException t) {
Expand Down Expand Up @@ -1130,7 +1131,7 @@ private static void writePretty(final Configuration configuration, final OutputS
prettyPrintResult = scxmlResult;
}

final TransformerFactory factory = TransformerFactory.newInstance();
final TransformerFactory factory = SecureTransformerFactory.newInstance();
try {
final Transformer transformer = factory.newTransformer();
if (configuration.encoding != null) {
Expand Down
Loading