diff --git a/jollyday-jaxb/src/main/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapper.java b/jollyday-jaxb/src/main/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapper.java index 38bb2a55d..068405c9c 100644 --- a/jollyday-jaxb/src/main/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapper.java +++ b/jollyday-jaxb/src/main/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapper.java @@ -12,6 +12,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamReader; import java.io.InputStream; public class JaxbXMLMapper { @@ -25,6 +28,15 @@ public class JaxbXMLMapper { private static final JAXBContext jaxbContext = new JAXBContextCreator().create(); + /** + * StAX factory hardened against XXE. DTD processing and external entity resolution are + * disabled, so a {@code } declaration (and any external or parameter entity it + * might carry) is rejected rather than resolved. The protection is configured explicitly here + * instead of relying on the defaults of whichever StAX/JAXB provider happens to be on the + * classpath, which could change if a consumer swaps or downgrades that provider. + */ + private static final XMLInputFactory XML_INPUT_FACTORY = createHardenedXmlInputFactory(); + /** * Unmarshalls the configuration from the stream. Uses JAXB for * this. @@ -38,14 +50,26 @@ public class JaxbXMLMapper { } try { - final Unmarshaller um = jaxbContext.createUnmarshaller(); - @SuppressWarnings("unchecked") final JAXBElement jaxbElement = (JAXBElement) um.unmarshal(stream); - return jaxbElement.getValue(); - } catch (JAXBException exception) { + final XMLStreamReader xmlStreamReader = XML_INPUT_FACTORY.createXMLStreamReader(stream); + try { + final Unmarshaller um = jaxbContext.createUnmarshaller(); + final JAXBElement jaxbElement = um.unmarshal(xmlStreamReader, Configuration.class); + return jaxbElement.getValue(); + } finally { + xmlStreamReader.close(); + } + } catch (JAXBException | XMLStreamException exception) { throw new IllegalStateException("Cannot parse holidays XML file.", exception); } } + private static @NonNull XMLInputFactory createHardenedXmlInputFactory() { + final XMLInputFactory factory = XMLInputFactory.newFactory(); + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + return factory; + } + private static class JAXBContextCreator { private JAXBContext create() { return createJAXBContext(); diff --git a/jollyday-jaxb/src/test/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapperTest.java b/jollyday-jaxb/src/test/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapperTest.java index fafdf6c37..b8eaca8cd 100644 --- a/jollyday-jaxb/src/test/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapperTest.java +++ b/jollyday-jaxb/src/test/java/de/focus_shift/jollyday/jaxb/JaxbXMLMapperTest.java @@ -5,7 +5,9 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import java.io.ByteArrayInputStream; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -19,6 +21,18 @@ void testUnmarshallConfigurationNullCheck() { assertThatThrownBy(() -> sut.unmarshallConfiguration(null)).isInstanceOf(IllegalArgumentException.class); } + @Test + void rejectsXmlWithDoctypeToPreventXxe() { + final String maliciousXml = + "\n" + + "]>\n" + + ""; + final InputStream inputStream = new ByteArrayInputStream(maliciousXml.getBytes(StandardCharsets.UTF_8)); + + assertThatThrownBy(() -> sut.unmarshallConfiguration(inputStream)) + .isInstanceOf(IllegalStateException.class); + } + @ParameterizedTest @ValueSource(strings = {"Holidays_at.xml", "Holidays_de.xml", "Holidays_gb.xml", "Holidays_ua.xml", "Holidays_tr.xml", "Holidays_za.xml"}) void unmarshalRealResource(String holidayFileName) {