Skip to content

Commit 9c3580d

Browse files
committed
Disable DTD when parsing untrusted XML input
Issue: SPR-13136
1 parent a837cbe commit 9c3580d

File tree

12 files changed

+476
-56
lines changed

12 files changed

+476
-56
lines changed

spring-oxm/src/main/java/org/springframework/oxm/jaxb/Jaxb2Marshaller.java

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2014 the original author or authors.
2+
* Copyright 2002-2015 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -37,6 +37,7 @@
3737
import java.util.Date;
3838
import java.util.Map;
3939
import java.util.UUID;
40+
4041
import javax.activation.DataHandler;
4142
import javax.activation.DataSource;
4243
import javax.xml.XMLConstants;
@@ -177,6 +178,8 @@ public class Jaxb2Marshaller implements MimeMarshaller, MimeUnmarshaller, Generi
177178

178179
private Schema schema;
179180

181+
private boolean supportDtd = false;
182+
180183
private boolean processExternalEntities = false;
181184

182185

@@ -391,16 +394,36 @@ public void setMappedClass(Class<?> mappedClass) {
391394
this.mappedClass = mappedClass;
392395
}
393396

397+
/**
398+
* Indicates whether DTD parsing should be supported.
399+
* <p>Default is {@code false} meaning that DTD is disabled.
400+
*/
401+
public void setSupportDtd(boolean supportDtd) {
402+
this.supportDtd = supportDtd;
403+
}
404+
405+
/**
406+
* Whether DTD parsing is supported.
407+
*/
408+
public boolean isSupportDtd() {
409+
return this.supportDtd;
410+
}
411+
394412
/**
395413
* Indicates whether external XML entities are processed when unmarshalling.
396414
* <p>Default is {@code false}, meaning that external entities are not resolved.
397415
* Note that processing of external entities will only be enabled/disabled when the
398416
* {@code Source} passed to {@link #unmarshal(Source)} is a {@link SAXSource} or
399417
* {@link StreamSource}. It has no effect for {@link DOMSource} or {@link StAXSource}
400418
* instances.
419+
* <p><strong>Note:</strong> setting this option to {@code true} also
420+
* automatically sets {@link #setSupportDtd} to {@code true}.
401421
*/
402422
public void setProcessExternalEntities(boolean processExternalEntities) {
403423
this.processExternalEntities = processExternalEntities;
424+
if (processExternalEntities) {
425+
setSupportDtd(true);
426+
}
404427
}
405428

406429
/**
@@ -410,6 +433,7 @@ public boolean isProcessExternalEntities() {
410433
return this.processExternalEntities;
411434
}
412435

436+
413437
@Override
414438
public void setBeanClassLoader(ClassLoader classLoader) {
415439
this.beanClassLoader = classLoader;
@@ -754,6 +778,14 @@ else if (this.mappedClass != null) {
754778
return unmarshaller.unmarshal(source);
755779
}
756780
}
781+
catch (NullPointerException ex) {
782+
if (!isSupportDtd()) {
783+
throw new UnmarshallingFailureException("NPE while unmarshalling. " +
784+
"This can happen on JDK 1.6 due to the presence of DTD " +
785+
"declarations, which are disabled.", ex);
786+
}
787+
throw ex;
788+
}
757789
catch (JAXBException ex) {
758790
throw convertJaxbException(ex);
759791
}
@@ -809,6 +841,7 @@ else if (streamSource.getReader() != null) {
809841
if (xmlReader == null) {
810842
xmlReader = XMLReaderFactory.createXMLReader();
811843
}
844+
xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
812845
String name = "http://xml.org/sax/features/external-general-entities";
813846
xmlReader.setFeature(name, isProcessExternalEntities());
814847
if (!isProcessExternalEntities()) {

spring-oxm/src/main/java/org/springframework/oxm/support/AbstractMarshaller.java

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,23 +72,45 @@ public abstract class AbstractMarshaller implements Marshaller, Unmarshaller {
7272
/** Logger available to subclasses */
7373
protected final Log logger = LogFactory.getLog(getClass());
7474

75+
private boolean supportDtd = false;
76+
7577
private boolean processExternalEntities = false;
7678

7779
private DocumentBuilderFactory documentBuilderFactory;
7880

7981
private final Object documentBuilderFactoryMonitor = new Object();
8082

8183

84+
/**
85+
* Indicates whether DTD parsing should be supported.
86+
* <p>Default is {@code false} meaning that DTD is disabled.
87+
*/
88+
public void setSupportDtd(boolean supportDtd) {
89+
this.supportDtd = supportDtd;
90+
}
91+
92+
/**
93+
* Whether DTD parsing is supported.
94+
*/
95+
public boolean isSupportDtd() {
96+
return this.supportDtd;
97+
}
98+
8299
/**
83100
* Indicates whether external XML entities are processed when unmarshalling.
84101
* <p>Default is {@code false}, meaning that external entities are not resolved.
85102
* Note that processing of external entities will only be enabled/disabled when the
86103
* {@code Source} passed to {@link #unmarshal(Source)} is a {@link SAXSource} or
87104
* {@link StreamSource}. It has no effect for {@link DOMSource} or {@link StAXSource}
88105
* instances.
106+
* <p><strong>Note:</strong> setting this option to {@code true} also
107+
* automatically sets {@link #setSupportDtd} to {@code true}.
89108
*/
90109
public void setProcessExternalEntities(boolean processExternalEntities) {
91110
this.processExternalEntities = processExternalEntities;
111+
if (processExternalEntities) {
112+
setSupportDtd(true);
113+
}
92114
}
93115

94116
/**
@@ -133,6 +155,8 @@ protected DocumentBuilderFactory createDocumentBuilderFactory() throws ParserCon
133155
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
134156
factory.setValidating(false);
135157
factory.setNamespaceAware(true);
158+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
159+
factory.setFeature("http://xml.org/sax/features/external-general-entities", isProcessExternalEntities());
136160
return factory;
137161
}
138162

@@ -147,7 +171,11 @@ protected DocumentBuilderFactory createDocumentBuilderFactory() throws ParserCon
147171
protected DocumentBuilder createDocumentBuilder(DocumentBuilderFactory factory)
148172
throws ParserConfigurationException {
149173

150-
return factory.newDocumentBuilder();
174+
DocumentBuilder documentBuilder = factory.newDocumentBuilder();
175+
if (!isProcessExternalEntities()) {
176+
documentBuilder.setEntityResolver(NO_OP_ENTITY_RESOLVER);
177+
}
178+
return documentBuilder;
151179
}
152180

153181
/**
@@ -157,6 +185,7 @@ protected DocumentBuilder createDocumentBuilder(DocumentBuilderFactory factory)
157185
*/
158186
protected XMLReader createXmlReader() throws SAXException {
159187
XMLReader xmlReader = XMLReaderFactory.createXMLReader();
188+
xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
160189
xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", isProcessExternalEntities());
161190
if (!isProcessExternalEntities()) {
162191
xmlReader.setEntityResolver(NO_OP_ENTITY_RESOLVER);
@@ -343,7 +372,17 @@ protected Object unmarshalDomSource(DOMSource domSource) throws XmlMappingExcept
343372
if (domSource.getNode() == null) {
344373
domSource.setNode(buildDocument());
345374
}
346-
return unmarshalDomNode(domSource.getNode());
375+
try {
376+
return unmarshalDomNode(domSource.getNode());
377+
}
378+
catch (NullPointerException ex) {
379+
if (!isSupportDtd()) {
380+
throw new UnmarshallingFailureException("NPE while unmarshalling. " +
381+
"This can happen on JDK 1.6 due to the presence of DTD " +
382+
"declarations, which are disabled.", ex);
383+
}
384+
throw ex;
385+
}
347386
}
348387

349388
/**
@@ -391,7 +430,17 @@ protected Object unmarshalSaxSource(SAXSource saxSource) throws XmlMappingExcept
391430
if (saxSource.getInputSource() == null) {
392431
saxSource.setInputSource(new InputSource());
393432
}
394-
return unmarshalSaxReader(saxSource.getXMLReader(), saxSource.getInputSource());
433+
try {
434+
return unmarshalSaxReader(saxSource.getXMLReader(), saxSource.getInputSource());
435+
}
436+
catch (NullPointerException ex) {
437+
if (!isSupportDtd()) {
438+
throw new UnmarshallingFailureException("NPE while unmarshalling. " +
439+
"This can happen on JDK 1.6 due to the presence of DTD " +
440+
"declarations, which are disabled.");
441+
}
442+
throw ex;
443+
}
395444
}
396445

397446
/**
@@ -404,7 +453,7 @@ protected Object unmarshalSaxSource(SAXSource saxSource) throws XmlMappingExcept
404453
*/
405454
protected Object unmarshalStreamSource(StreamSource streamSource) throws XmlMappingException, IOException {
406455
if (streamSource.getInputStream() != null) {
407-
if (isProcessExternalEntities()) {
456+
if (isProcessExternalEntities() && isSupportDtd()) {
408457
return unmarshalInputStream(streamSource.getInputStream());
409458
}
410459
else {
@@ -414,7 +463,7 @@ protected Object unmarshalStreamSource(StreamSource streamSource) throws XmlMapp
414463
}
415464
}
416465
else if (streamSource.getReader() != null) {
417-
if (isProcessExternalEntities()) {
466+
if (isProcessExternalEntities() && isSupportDtd()) {
418467
return unmarshalReader(streamSource.getReader());
419468
}
420469
else {

spring-oxm/src/test/java/org/springframework/oxm/castor/CastorUnmarshallerTests.java

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,18 @@
1616

1717
package org.springframework.oxm.castor;
1818

19+
import static junit.framework.Assert.assertEquals;
20+
import static org.hamcrest.CoreMatchers.*;
21+
import static org.junit.Assert.assertNotNull;
22+
import static org.junit.Assert.assertNull;
23+
import static org.junit.Assert.assertSame;
24+
import static org.junit.Assert.*;
25+
1926
import java.io.ByteArrayInputStream;
2027
import java.io.IOException;
2128
import java.io.StringReader;
2229
import java.util.concurrent.atomic.AtomicReference;
30+
2331
import javax.xml.transform.sax.SAXSource;
2432
import javax.xml.transform.stream.StreamSource;
2533

@@ -33,9 +41,6 @@
3341
import org.springframework.oxm.MarshallingException;
3442
import org.springframework.oxm.Unmarshaller;
3543

36-
import static org.hamcrest.CoreMatchers.*;
37-
import static org.junit.Assert.*;
38-
3944
/**
4045
* @author Arjen Poutsma
4146
* @author Jakub Narloch
@@ -86,7 +91,7 @@ protected Unmarshaller createUnmarshaller() throws Exception {
8691
@Test
8792
public void unmarshalTargetClass() throws Exception {
8893
CastorMarshaller unmarshaller = new CastorMarshaller();
89-
unmarshaller.setTargetClasses(new Class[] { Flights.class } );
94+
unmarshaller.setTargetClasses(new Class[] {Flights.class});
9095
unmarshaller.afterPropertiesSet();
9196
StreamSource source = new StreamSource(new ByteArrayInputStream(INPUT_STRING.getBytes("UTF-8")));
9297
Object flights = unmarshaller.unmarshal(source);
@@ -97,7 +102,7 @@ public void unmarshalTargetClass() throws Exception {
97102
public void testSetBothTargetClassesAndMapping() throws IOException {
98103
CastorMarshaller unmarshaller = new CastorMarshaller();
99104
unmarshaller.setMappingLocation(new ClassPathResource("order-mapping.xml", CastorMarshaller.class));
100-
unmarshaller.setTargetClasses(new Class[] { Order.class } );
105+
unmarshaller.setTargetClasses(new Class[] {Order.class});
101106
unmarshaller.afterPropertiesSet();
102107

103108
String xml = "<order>" +
@@ -183,7 +188,7 @@ public void testClearCollectionsTrue() throws Exception {
183188
@Ignore("Fails on the build server for some reason")
184189
public void testClearCollectionsFalse() throws Exception {
185190
Flights flights = new Flights();
186-
flights.setFlight(new Flight[]{new Flight(), null});
191+
flights.setFlight(new Flight[] {new Flight(), null});
187192
getCastorUnmarshaller().setRootObject(flights);
188193
getCastorUnmarshaller().setClearCollections(false);
189194
Object result = unmarshalFlights();
@@ -196,7 +201,7 @@ public void testClearCollectionsFalse() throws Exception {
196201
}
197202

198203
@Test
199-
public void unmarshalStreamSourceExternalEntities() throws Exception {
204+
public void unmarshalStreamSourceWithXmlOptions() throws Exception {
200205
final AtomicReference<XMLReader> result = new AtomicReference<XMLReader>();
201206
CastorMarshaller marshaller = new CastorMarshaller() {
202207
@Override
@@ -206,21 +211,24 @@ protected Object unmarshalSaxReader(XMLReader xmlReader, InputSource inputSource
206211
}
207212
};
208213

209-
// 1. external-general-entities disabled (default)
214+
// 1. external-general-entities and dtd support disabled (default)
210215
marshaller.unmarshal(new StreamSource("1"));
211216
assertNotNull(result.get());
217+
assertEquals(true, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
212218
assertEquals(false, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
213219

214-
// 2. external-general-entities disabled (default)
220+
// 2. external-general-entities and dtd support enabled
215221
result.set(null);
222+
marshaller.setSupportDtd(true);
216223
marshaller.setProcessExternalEntities(true);
217224
marshaller.unmarshal(new StreamSource("1"));
218225
assertNotNull(result.get());
226+
assertEquals(false, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
219227
assertEquals(true, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
220228
}
221229

222230
@Test
223-
public void unmarshalSaxSourceExternalEntities() throws Exception {
231+
public void unmarshalSaxSourceWithXmlOptions() throws Exception {
224232
final AtomicReference<XMLReader> result = new AtomicReference<XMLReader>();
225233
CastorMarshaller marshaller = new CastorMarshaller() {
226234
@Override
@@ -230,16 +238,19 @@ protected Object unmarshalSaxReader(XMLReader xmlReader, InputSource inputSource
230238
}
231239
};
232240

233-
// 1. external-general-entities disabled (default)
241+
// 1. external-general-entities and dtd support disabled (default)
234242
marshaller.unmarshal(new SAXSource(new InputSource("1")));
235243
assertNotNull(result.get());
244+
assertEquals(true, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
236245
assertEquals(false, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
237246

238-
// 2. external-general-entities disabled (default)
247+
// 2. external-general-entities and dtd support enabled
239248
result.set(null);
249+
marshaller.setSupportDtd(true);
240250
marshaller.setProcessExternalEntities(true);
241251
marshaller.unmarshal(new SAXSource(new InputSource("1")));
242252
assertNotNull(result.get());
253+
assertEquals(false, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
243254
assertEquals(true, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
244255
}
245256

0 commit comments

Comments
 (0)