Skip to content

Commit 5a711c0

Browse files
rstoyanchevrstoyanchev
committed
Disable DTD when parsing untrusted XML input
Issue: SPR-13136
1 parent 1e42464 commit 5a711c0

File tree

11 files changed

+372
-27
lines changed

11 files changed

+372
-27
lines changed

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

Lines changed: 32 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.
@@ -176,6 +176,8 @@ public class Jaxb2Marshaller implements MimeMarshaller, MimeUnmarshaller, Generi
176176

177177
private Schema schema;
178178

179+
private boolean supportDtd = false;
180+
179181
private boolean processExternalEntities = false;
180182

181183

@@ -390,16 +392,36 @@ public void setMappedClass(Class<?> mappedClass) {
390392
this.mappedClass = mappedClass;
391393
}
392394

395+
/**
396+
* Indicates whether DTD parsing should be supported.
397+
* <p>Default is {@code false} meaning that DTD is disabled.
398+
*/
399+
public void setSupportDtd(boolean supportDtd) {
400+
this.supportDtd = supportDtd;
401+
}
402+
403+
/**
404+
* Whether DTD parsing is supported.
405+
*/
406+
public boolean isSupportDtd() {
407+
return this.supportDtd;
408+
}
409+
393410
/**
394411
* Indicates whether external XML entities are processed when unmarshalling.
395412
* <p>Default is {@code false}, meaning that external entities are not resolved.
396413
* Note that processing of external entities will only be enabled/disabled when the
397414
* {@code Source} passed to {@link #unmarshal(Source)} is a {@link SAXSource} or
398415
* {@link StreamSource}. It has no effect for {@link DOMSource} or {@link StAXSource}
399416
* instances.
417+
* <p><strong>Note:</strong> setting this option to {@code true} also
418+
* automatically sets {@link #setSupportDtd} to {@code true}.
400419
*/
401420
public void setProcessExternalEntities(boolean processExternalEntities) {
402421
this.processExternalEntities = processExternalEntities;
422+
if (processExternalEntities) {
423+
setSupportDtd(true);
424+
}
403425
}
404426

405427
/**
@@ -746,6 +768,14 @@ else if (this.mappedClass != null) {
746768
return unmarshaller.unmarshal(source);
747769
}
748770
}
771+
catch (NullPointerException ex) {
772+
if (!isSupportDtd()) {
773+
throw new UnmarshallingFailureException("NPE while unmarshalling. " +
774+
"This can happen on JDK 1.6 due to the presence of DTD " +
775+
"declarations, which are disabled.", ex);
776+
}
777+
throw ex;
778+
}
749779
catch (JAXBException ex) {
750780
throw convertJaxbException(ex);
751781
}
@@ -801,6 +831,7 @@ else if (streamSource.getReader() != null) {
801831
if (xmlReader == null) {
802832
xmlReader = XMLReaderFactory.createXMLReader();
803833
}
834+
xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
804835
String name = "http://xml.org/sax/features/external-general-entities";
805836
xmlReader.setFeature(name, isProcessExternalEntities());
806837
if (!isProcessExternalEntities()) {

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

Lines changed: 55 additions & 6 deletions
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.
@@ -71,23 +71,45 @@ public abstract class AbstractMarshaller implements Marshaller, Unmarshaller {
7171
/** Logger available to subclasses */
7272
protected final Log logger = LogFactory.getLog(getClass());
7373

74+
private boolean supportDtd = false;
75+
7476
private boolean processExternalEntities = false;
7577

7678
private DocumentBuilderFactory documentBuilderFactory;
7779

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

8082

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

93115
/**
@@ -111,6 +133,8 @@ protected DocumentBuilderFactory createDocumentBuilderFactory() throws ParserCon
111133
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
112134
factory.setValidating(false);
113135
factory.setNamespaceAware(true);
136+
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
137+
factory.setFeature("http://xml.org/sax/features/external-general-entities", isProcessExternalEntities());
114138
return factory;
115139
}
116140

@@ -125,7 +149,11 @@ protected DocumentBuilderFactory createDocumentBuilderFactory() throws ParserCon
125149
protected DocumentBuilder createDocumentBuilder(DocumentBuilderFactory factory)
126150
throws ParserConfigurationException {
127151

128-
return factory.newDocumentBuilder();
152+
DocumentBuilder documentBuilder = factory.newDocumentBuilder();
153+
if (!isProcessExternalEntities()) {
154+
documentBuilder.setEntityResolver(NO_OP_ENTITY_RESOLVER);
155+
}
156+
return documentBuilder;
129157
}
130158

131159
/**
@@ -135,6 +163,7 @@ protected DocumentBuilder createDocumentBuilder(DocumentBuilderFactory factory)
135163
*/
136164
protected XMLReader createXmlReader() throws SAXException {
137165
XMLReader xmlReader = XMLReaderFactory.createXMLReader();
166+
xmlReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", !isSupportDtd());
138167
xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", isProcessExternalEntities());
139168
if (!isProcessExternalEntities()) {
140169
xmlReader.setEntityResolver(NO_OP_ENTITY_RESOLVER);
@@ -343,7 +372,17 @@ protected Object unmarshalDomSource(DOMSource domSource) throws XmlMappingExcept
343372
"Could not create document placeholder for DOMSource: " + ex.getMessage(), ex);
344373
}
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.", ex);
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: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,21 +206,24 @@ protected Object unmarshalSaxReader(XMLReader xmlReader, InputSource inputSource
206206
}
207207
};
208208

209-
// 1. external-general-entities disabled (default)
209+
// 1. external-general-entities and dtd support disabled (default)
210210
marshaller.unmarshal(new StreamSource("1"));
211211
assertNotNull(result.get());
212+
assertEquals(true, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
212213
assertEquals(false, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
213214

214-
// 2. external-general-entities disabled (default)
215+
// 2. external-general-entities and dtd support enabled
215216
result.set(null);
217+
marshaller.setSupportDtd(true);
216218
marshaller.setProcessExternalEntities(true);
217219
marshaller.unmarshal(new StreamSource("1"));
218220
assertNotNull(result.get());
221+
assertEquals(false, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
219222
assertEquals(true, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
220223
}
221224

222225
@Test
223-
public void unmarshalSaxSourceExternalEntities() throws Exception {
226+
public void unmarshalSaxSourceWithXmlOptions() throws Exception {
224227
final AtomicReference<XMLReader> result = new AtomicReference<XMLReader>();
225228
CastorMarshaller marshaller = new CastorMarshaller() {
226229
@Override
@@ -230,16 +233,19 @@ protected Object unmarshalSaxReader(XMLReader xmlReader, InputSource inputSource
230233
}
231234
};
232235

233-
// 1. external-general-entities disabled (default)
236+
// 1. external-general-entities and dtd support disabled (default)
234237
marshaller.unmarshal(new SAXSource(new InputSource("1")));
235238
assertNotNull(result.get());
239+
assertEquals(true, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
236240
assertEquals(false, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
237241

238-
// 2. external-general-entities disabled (default)
242+
// 2. external-general-entities and dtd support enabled
239243
result.set(null);
244+
marshaller.setSupportDtd(true);
240245
marshaller.setProcessExternalEntities(true);
241246
marshaller.unmarshal(new SAXSource(new InputSource("1")));
242247
assertNotNull(result.get());
248+
assertEquals(false, result.get().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
243249
assertEquals(true, result.get().getFeature("http://xml.org/sax/features/external-general-entities"));
244250
}
245251

‎spring-oxm/src/test/java/org/springframework/oxm/jaxb/Jaxb2MarshallerTests.java

Lines changed: 12 additions & 6 deletions
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.
@@ -308,7 +308,7 @@ public void marshalAWrappedObjectHoldingAnXmlElementDeclElement() throws Excepti
308308
// SPR-10806
309309

310310
@Test
311-
public void unmarshalStreamSourceExternalEntities() throws Exception {
311+
public void unmarshalStreamSourceWithXmlOptions() throws Exception {
312312

313313
final javax.xml.bind.Unmarshaller unmarshaller = mock(javax.xml.bind.Unmarshaller.class);
314314
Jaxb2Marshaller marshaller = new Jaxb2Marshaller() {
@@ -318,24 +318,27 @@ protected javax.xml.bind.Unmarshaller createUnmarshaller() {
318318
}
319319
};
320320

321-
// 1. external-general-entities disabled (default)
321+
// 1. external-general-entities and dtd support disabled (default)
322322

323323
marshaller.unmarshal(new StreamSource("1"));
324324
ArgumentCaptor<SAXSource> sourceCaptor = ArgumentCaptor.forClass(SAXSource.class);
325325
verify(unmarshaller).unmarshal(sourceCaptor.capture());
326326

327327
SAXSource result = sourceCaptor.getValue();
328+
assertEquals(true, result.getXMLReader().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
328329
assertEquals(false, result.getXMLReader().getFeature("http://xml.org/sax/features/external-general-entities"));
329330

330-
// 2. external-general-entities enabled
331+
// 2. external-general-entities and dtd support enabled
331332

332333
reset(unmarshaller);
334+
marshaller.setSupportDtd(true);
333335
marshaller.setProcessExternalEntities(true);
334336

335337
marshaller.unmarshal(new StreamSource("1"));
336338
verify(unmarshaller).unmarshal(sourceCaptor.capture());
337339

338340
result = sourceCaptor.getValue();
341+
assertEquals(false, result.getXMLReader().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
339342
assertEquals(true, result.getXMLReader().getFeature("http://xml.org/sax/features/external-general-entities"));
340343
}
341344

@@ -352,24 +355,27 @@ protected javax.xml.bind.Unmarshaller createUnmarshaller() {
352355
}
353356
};
354357

355-
// 1. external-general-entities disabled (default)
358+
// 1. external-general-entities and dtd support disabled (default)
356359

357360
marshaller.unmarshal(new SAXSource(new InputSource("1")));
358361
ArgumentCaptor<SAXSource> sourceCaptor = ArgumentCaptor.forClass(SAXSource.class);
359362
verify(unmarshaller).unmarshal(sourceCaptor.capture());
360363

361364
SAXSource result = sourceCaptor.getValue();
365+
assertEquals(true, result.getXMLReader().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
362366
assertEquals(false, result.getXMLReader().getFeature("http://xml.org/sax/features/external-general-entities"));
363367

364-
// 2. external-general-entities enabled
368+
// 2. external-general-entities and dtd support enabled
365369

366370
reset(unmarshaller);
371+
marshaller.setSupportDtd(true);
367372
marshaller.setProcessExternalEntities(true);
368373

369374
marshaller.unmarshal(new SAXSource(new InputSource("1")));
370375
verify(unmarshaller).unmarshal(sourceCaptor.capture());
371376

372377
result = sourceCaptor.getValue();
378+
assertEquals(false, result.getXMLReader().getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
373379
assertEquals(true, result.getXMLReader().getFeature("http://xml.org/sax/features/external-general-entities"));
374380
}
375381

‎spring-web/src/main/java/org/springframework/http/converter/xml/Jaxb2CollectionHttpMessageConverter.java

Lines changed: 2 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.
@@ -226,6 +226,7 @@ protected void writeToResult(T t, HttpHeaders headers, Result result) throws IOE
226226
*/
227227
protected XMLInputFactory createXmlInputFactory() {
228228
XMLInputFactory inputFactory = XMLInputFactory.newInstance();
229+
inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
229230
inputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
230231
inputFactory.setXMLResolver(NO_OP_XML_RESOLVER);
231232
return inputFactory;

0 commit comments

Comments
 (0)