Prevent XXE injection during CycloneDX validation and parsing

Ports DependencyTrack/dependency-track#3871 from Dependency-Track v4.11.4.

Signed-off-by: nscuro <[email protected]>

src/main/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidator.java

@@ -22,13 +22,13 @@
 import com.fasterxml.jackson.core.JsonParseException;
 import com.fasterxml.jackson.core.JsonToken;
 import com.fasterxml.jackson.databind.json.JsonMapper;
-import org.codehaus.stax2.XMLInputFactory2;
 import org.cyclonedx.Version;
 import org.cyclonedx.exception.ParseException;
 import org.cyclonedx.parsers.JsonParser;
 import org.cyclonedx.parsers.Parser;
 import org.cyclonedx.parsers.XmlParser;
 
+import javax.xml.XMLConstants;
 import javax.xml.stream.XMLInputFactory;
 import javax.xml.stream.XMLStreamException;
 import javax.xml.stream.XMLStreamReader;
@@ -94,10 +94,13 @@ public void validate(final byte[] bomBytes) {
     }
 
     private FormatAndVersion detectFormatAndSchemaVersion(final byte[] bomBytes) {
+        final var suppressedExceptions = new ArrayList<Exception>(2);
+
         try {
             final Version version = detectSchemaVersionFromJson(bomBytes);
             return new FormatAndVersion(Format.JSON, version);
         } catch (JsonParseException e) {
+            suppressedExceptions.add(e);
             if (LOGGER.isDebugEnabled()) {
                 LOGGER.debug("Failed to parse BOM as JSON", e);
             }
@@ -109,12 +112,15 @@ private FormatAndVersion detectFormatAndSchemaVersion(final byte[] bomBytes) {
             final Version version = detectSchemaVersionFromXml(bomBytes);
             return new FormatAndVersion(Format.XML, version);
         } catch (XMLStreamException e) {
+            suppressedExceptions.add(e);
             if (LOGGER.isDebugEnabled()) {
                 LOGGER.debug("Failed to parse BOM as XML", e);
             }
         }
 
-        throw new InvalidBomException("BOM is neither valid JSON nor XML");
+        final var exception = new InvalidBomException("BOM is neither valid JSON nor XML");
+        suppressedExceptions.forEach(exception::addSuppressed);
+        throw exception;
     }
 
     private Version detectSchemaVersionFromJson(final byte[] bomBytes) throws IOException {
@@ -157,7 +163,13 @@ private Version detectSchemaVersionFromJson(final byte[] bomBytes) throws IOExce
     }
 
     private Version detectSchemaVersionFromXml(final byte[] bomBytes) throws XMLStreamException {
-        final XMLInputFactory xmlInputFactory = XMLInputFactory2.newFactory();
+        final XMLInputFactory xmlInputFactory = XMLInputFactory.newFactory();
+        xmlInputFactory.setProperty(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        // NB: Setting XMLConstants.ACCESS_EXTERNAL_DTD to empty string is recommended by SAST tools,
+        // but Woodstox does not support it: https://github.com/FasterXML/woodstox/issues/51
+        // Setting IS_SUPPORTING_EXTERNAL_ENTITIES to false achieves the same:
+        // https://github.com/FasterXML/woodstox/issues/50#issuecomment-388842419
+        xmlInputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
         final var bomBytesStream = new ByteArrayInputStream(bomBytes);
         final XMLStreamReader xmlStreamReader = xmlInputFactory.createXMLStreamReader(bomBytesStream);
 

src/test/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidatorTest.java

@@ -18,6 +18,7 @@
  */
 package org.dependencytrack.parser.cyclonedx;
 
+import com.github.tomakehurst.wiremock.WireMockServer;
 import junitparams.JUnitParamsRunner;
 import junitparams.Parameters;
 import org.junit.Before;
@@ -34,8 +35,13 @@
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
 
+import static com.github.tomakehurst.wiremock.client.WireMock.anyRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
+import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.Assertions.catchThrowableOfType;
 
 @RunWith(JUnitParamsRunner.class)
 public class CycloneDxValidatorTest {
@@ -241,4 +247,35 @@ public void testValidateJsonWithUrlContainingEncodedBrackets() {
                         """.getBytes()));
     }
 
+    @Test
+    public void testValidateXmlWithXxeProtection() {
+        final var wireMock = new WireMockServer(options().dynamicPort());
+        wireMock.start();
+
+        try {
+            final Throwable throwable = catchThrowableOfType(
+                    () -> validator.validate("""
+                            <?xml version="1.0" encoding="UTF-8"?>
+                            <!DOCTYPE bom [<!ENTITY %% sp SYSTEM "http://localhost:%d/does-not-exist/file.dtd"> %%sp;]>
+                            <bom xmlns="http://cyclonedx.org/schema/bom/1.5"/>
+                            """.formatted(wireMock.port()).getBytes()),
+                    InvalidBomException.class
+            );
+
+            // Ensure we failed for the right reason.
+            assertThat(throwable.getSuppressed()).hasSize(2);
+            assertThat(throwable.getSuppressed()).anySatisfy(suppressed -> assertThat(suppressed)
+                    .hasMessageContaining("""
+                            Encountered a reference to external entity "sp", but stream reader has feature \
+                            "javax.xml.stream.isSupportingExternalEntities" disabled"""
+                    )
+            );
+
+            // Ensure that in fact no request was performed.
+            wireMock.verify(0, anyRequestedFor(anyUrl()));
+        } finally {
+            wireMock.stop();
+        }
+    }
+
 }