diff --git a/release-notes/CREDITS b/release-notes/CREDITS index fcc836c5..3e693059 100644 --- a/release-notes/CREDITS +++ b/release-notes/CREDITS @@ -154,5 +154,8 @@ Christian Beikov (@beikov) (3.3.0) @Sahana2524 - * Fixed #879: Use `Locale.ROOT` for case folding in `CaseInsensitiveNameSet` + * Fixed #878: Re-apply entity/DTD hardening after JDK deserialization of + `XmlFactory` + (3.3.0) + * Fixed #879: Use `Locale.ROOT` for case folding in `CaseInsensitiveNameSet` (3.3.0) diff --git a/release-notes/VERSION b/release-notes/VERSION index fc1abdf0..ae83eb42 100644 --- a/release-notes/VERSION +++ b/release-notes/VERSION @@ -9,6 +9,8 @@ Version: 3.x (for earlier see VERSION-2.x) #873: Fix handling of `@JsonApplyView` (fix by @cowtowncoder, w/ Claude code) +#878: Re-apply entity/DTD hardening after JDK deserialization of `XmlFactory` + (fix by @Sahana2524) #879: Use `Locale.ROOT` for case folding in `CaseInsensitiveNameSet` (fix by @Sahana2524) diff --git a/src/main/java/tools/jackson/dataformat/xml/XmlFactory.java b/src/main/java/tools/jackson/dataformat/xml/XmlFactory.java index ccbb503c..9754ea5b 100644 --- a/src/main/java/tools/jackson/dataformat/xml/XmlFactory.java +++ b/src/main/java/tools/jackson/dataformat/xml/XmlFactory.java @@ -240,7 +240,12 @@ protected Object readResolve() { final XMLInputFactory inf; final XMLOutputFactory outf; try { - inf = (XMLInputFactory) Class.forName(_jdkXmlInFactory).getDeclaredConstructor().newInstance(); + // Only the factory class name survives serialization, so we get a bare + // default instance back: re-apply the entity/DTD hardening the builder + // would have set, otherwise a securely-built factory comes back with + // external entity + DTD processing re-enabled (see [dataformat-xml#190], [dataformat-xml#211]). + inf = XmlFactoryBuilder.secureXmlInputFactory( + (XMLInputFactory) Class.forName(_jdkXmlInFactory).getDeclaredConstructor().newInstance()); outf = (XMLOutputFactory) Class.forName(_jdkXmlOutFactory).getDeclaredConstructor().newInstance(); } catch (Exception e) { throw new IllegalArgumentException(e); diff --git a/src/main/java/tools/jackson/dataformat/xml/XmlFactoryBuilder.java b/src/main/java/tools/jackson/dataformat/xml/XmlFactoryBuilder.java index 1297f5fc..60851efa 100644 --- a/src/main/java/tools/jackson/dataformat/xml/XmlFactoryBuilder.java +++ b/src/main/java/tools/jackson/dataformat/xml/XmlFactoryBuilder.java @@ -119,6 +119,19 @@ protected static XMLInputFactory defaultXmlInputFactory(ClassLoader cl) { // 24-Oct-2022, tatu: as per [dataformat-xml#550] need extra care xmlIn = XMLInputFactory.newFactory(); } + return secureXmlInputFactory(xmlIn); + } + + /** + * Applies the default entity/DTD hardening to a freshly created + * {@link XMLInputFactory}. Shared so that any code path building a factory + * internally (including JDK-deserialization reconstruction in + * {@code XmlFactory.readResolve()}) applies the same protections and the two + * can not drift apart. + * + * @since 3.3 + */ + protected static XMLInputFactory secureXmlInputFactory(XMLInputFactory xmlIn) { // as per [dataformat-xml#190], disable external entity expansion by default xmlIn.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE); // and ditto wrt [dataformat-xml#211], SUPPORT_DTD diff --git a/src/test/java/tools/jackson/dataformat/xml/misc/DTDAfterSerializationTest.java b/src/test/java/tools/jackson/dataformat/xml/misc/DTDAfterSerializationTest.java new file mode 100644 index 00000000..e35d1593 --- /dev/null +++ b/src/test/java/tools/jackson/dataformat/xml/misc/DTDAfterSerializationTest.java @@ -0,0 +1,44 @@ +package tools.jackson.dataformat.xml.misc; + +import java.io.*; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import tools.jackson.core.exc.StreamReadException; +import tools.jackson.dataformat.xml.XmlMapper; +import tools.jackson.dataformat.xml.XmlTestUtil; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +// [dataformat-xml]: entity/DTD hardening must survive JDK serialization of the mapper +public class DTDAfterSerializationTest extends XmlTestUtil +{ + // Internal entity that only expands when DTD processing is enabled + private static final String ENTITY_XML = + " ]>\n" + + "&x;"; + + private XmlMapper jdkRoundtrip(XmlMapper mapper) throws Exception { + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + try (ObjectOutputStream os = new ObjectOutputStream(bytes)) { + os.writeObject(mapper); + } + try (ObjectInputStream is = new ObjectInputStream( + new ByteArrayInputStream(bytes.toByteArray()))) { + return (XmlMapper) is.readObject(); + } + } + + @Test + public void testDTDStaysDisabledAfterRoundtrip() throws Exception + { + XmlMapper mapper = jdkRoundtrip(new XmlMapper()); + // Must fail specifically because the parser refuses the DTD-declared + // entity (DTD support off), leaving `&x;` unexpanded -- not for some + // unrelated binding reason. Before the fix this expanded to "HELLO". + StreamReadException e = assertThrows(StreamReadException.class, + () -> mapper.readValue(ENTITY_XML, Map.class)); + verifyException(e, "Undeclared general entity", "entity"); + } +}