diff --git a/src/main/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidator.java b/src/main/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidator.java index c98e24ce4f..991ebcb579 100644 --- a/src/main/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidator.java +++ b/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(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); diff --git a/src/test/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidatorTest.java b/src/test/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidatorTest.java index fc783623d0..1e2e439163 100644 --- a/src/test/java/org/dependencytrack/parser/cyclonedx/CycloneDxValidatorTest.java +++ b/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(""" + + %%sp;]> + + """.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(); + } + } + } \ No newline at end of file