Skip to content

Commit d9507dd

Browse files
thboileauthboileau
authored
Merge pull request #1 from restlet/block_expansion_of_entities
Block entities expansion that leads to reveal local file system data.
2 parents cff0578 + dbfeabc commit d9507dd

File tree

8 files changed

+156
-14
lines changed

8 files changed

+156
-14
lines changed

‎.classpath

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,27 @@
11
<?xml version="1.0" encoding="UTF-8"?>
22
<classpath>
3-
<classpathentry kind="src" output="target/classes" path="src/main/java"/>
4-
<classpathentry kind="src" output="target/test-classes" path="src/test/java"/>
5-
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/J2SE-1.5"/>
6-
<classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER"/>
3+
<classpathentry kind="src" output="target/classes" path="src/main/java">
4+
<attributes>
5+
<attribute name="optional" value="true"/>
6+
<attribute name="maven.pomderived" value="true"/>
7+
</attributes>
8+
</classpathentry>
9+
<classpathentry kind="src" path="src/test/resources"/>
10+
<classpathentry kind="src" output="target/test-classes" path="src/test/java">
11+
<attributes>
12+
<attribute name="optional" value="true"/>
13+
<attribute name="maven.pomderived" value="true"/>
14+
</attributes>
15+
</classpathentry>
16+
<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/J2SE-1.5">
17+
<attributes>
18+
<attribute name="maven.pomderived" value="true"/>
19+
</attributes>
20+
</classpathentry>
21+
<classpathentry kind="con" path="org.eclipse.m2e.MAVEN2_CLASSPATH_CONTAINER">
22+
<attributes>
23+
<attribute name="maven.pomderived" value="true"/>
24+
</attributes>
25+
</classpathentry>
726
<classpathentry kind="output" path="target/classes"/>
827
</classpath>

‎.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
/target
2+
/.settings
3+

‎pom.xml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
22
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
33
<modelVersion>4.0.0</modelVersion>
4-
<groupId>org.simpleframework</groupId>
5-
<artifactId>simple-xml</artifactId>
4+
<groupId>org.restlet.lib</groupId>
5+
<artifactId>org.simpleframework.simple-xml</artifactId>
66
<packaging>jar</packaging>
7-
<version>2.7.1</version>
7+
<version>2.7.2</version>
88
<name>Simple XML</name>
99
<url>http://simple.sourceforge.net</url>
1010
<description>Simple is a high performance XML serialization and configuration framework for Java</description>
@@ -36,7 +36,7 @@
3636
<dependency>
3737
<groupId>junit</groupId>
3838
<artifactId>junit</artifactId>
39-
<version>3.8.1</version>
39+
<version>4.12</version>
4040
<scope>test</scope>
4141
</dependency>
4242
<dependency>

‎src/main/java/org/simpleframework/xml/stream/DocumentProvider.java

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,16 @@
1818

1919
package org.simpleframework.xml.stream;
2020

21+
import java.io.IOException;
2122
import java.io.InputStream;
2223
import java.io.Reader;
2324

2425
import javax.xml.parsers.DocumentBuilder;
2526
import javax.xml.parsers.DocumentBuilderFactory;
27+
import javax.xml.parsers.ParserConfigurationException;
2628

27-
import org.w3c.dom.Document;
2829
import org.xml.sax.InputSource;
30+
import org.xml.sax.SAXException;
2931

3032
/**
3133
* The <code>DocumentProvider</code> object is used to provide event
@@ -54,6 +56,41 @@ class DocumentProvider implements Provider {
5456
public DocumentProvider() {
5557
this.factory = DocumentBuilderFactory.newInstance();
5658
this.factory.setNamespaceAware(true);
59+
60+
// Security issue: block entities expansion that can lead to expose local files
61+
// cf https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#Java
62+
String FEATURE = null;
63+
try {
64+
// This is the PRIMARY defense. If DTDs (doctypes) are disallowed, almost all XML entity attacks are prevented
65+
// Xerces 2 only - http://xerces.apache.org/xerces2-j/features.html#disallow-doctype-decl
66+
FEATURE = "http://apache.org/xml/features/disallow-doctype-decl";
67+
factory.setFeature(FEATURE, true);
68+
69+
// If you can't completely disable DTDs, then at least do the following:
70+
// Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-general-entities
71+
// Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-general-entities
72+
// JDK7+ - http://xml.org/sax/features/external-general-entities
73+
FEATURE = "http://xml.org/sax/features/external-general-entities";
74+
factory.setFeature(FEATURE, false);
75+
76+
// Xerces 1 - http://xerces.apache.org/xerces-j/features.html#external-parameter-entities
77+
// Xerces 2 - http://xerces.apache.org/xerces2-j/features.html#external-parameter-entities
78+
// JDK7+ - http://xml.org/sax/features/external-parameter-entities
79+
FEATURE = "http://xml.org/sax/features/external-parameter-entities";
80+
factory.setFeature(FEATURE, false);
81+
82+
// Disable external DTDs as well
83+
FEATURE = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
84+
factory.setFeature(FEATURE, false);
85+
86+
// and these as well, per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks"
87+
factory.setXIncludeAware(false);
88+
factory.setExpandEntityReferences(false);
89+
} catch (ParserConfigurationException e) {
90+
throw new RuntimeException("ParserConfigurationException was thrown. The feature '" + FEATURE
91+
+ "' is probably not supported by your XML processor.", e);
92+
}
93+
5794
}
5895

5996
/**
@@ -93,9 +130,16 @@ public EventReader provide(Reader source) throws Exception {
93130
* @return this is used to return the event reader implementation
94131
*/
95132
private EventReader provide(InputSource source) throws Exception {
96-
DocumentBuilder builder = factory.newDocumentBuilder();
97-
Document document = builder.parse(source);
98-
99-
return new DocumentReader(document);
100-
}
133+
DocumentBuilder builder = factory.newDocumentBuilder();
134+
135+
try {
136+
return new DocumentReader(builder.parse(source));
137+
} catch (SAXException e) {
138+
// On Apache, this should be thrown when disallowing DOCTYPE
139+
throw new RuntimeException("A DOCTYPE was passed into the XML document, and this is blocked on purpose due to security issues", e);
140+
} catch (IOException e) {
141+
// XXE that points to a file that doesn't exist
142+
throw new RuntimeException("IOException occurred, XXE may still possible", e);
143+
}
144+
}
101145
}

‎src/main/java/org/simpleframework/xml/stream/PullProvider.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121
import java.io.InputStream;
2222
import java.io.Reader;
2323

24+
import javax.xml.parsers.ParserConfigurationException;
25+
import javax.xml.stream.XMLInputFactory;
26+
2427
import org.xmlpull.v1.XmlPullParser;
2528
import org.xmlpull.v1.XmlPullParserFactory;
2629

@@ -50,6 +53,8 @@ class PullProvider implements Provider {
5053
public PullProvider() throws Exception {
5154
this.factory = XmlPullParserFactory.newInstance();
5255
this.factory.setNamespaceAware(true);
56+
// Security issue: block entities expansion that can lead to expose local files
57+
this.factory.setFeature(XmlPullParser.FEATURE_PROCESS_DOCDECL, false);
5358
}
5459

5560
/**

‎src/main/java/org/simpleframework/xml/stream/StreamProvider.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ class StreamProvider implements Provider {
5050
*/
5151
public StreamProvider() {
5252
this.factory = XMLInputFactory.newInstance();
53+
// Security issue: block entities expansion that can lead to expose local files
54+
this.factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
5355
}
5456

5557
/**

‎src/test/java/org/simpleframework/xml/stream/XxeSecurityTest.java

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
package org.simpleframework.xml.stream;
2+
3+
import org.junit.Assert;
4+
import org.junit.Test;
5+
import org.simpleframework.xml.Element;
6+
import org.simpleframework.xml.Root;
7+
import org.simpleframework.xml.Serializer;
8+
import org.simpleframework.xml.core.Persister;
9+
import org.simpleframework.xml.core.ValueRequiredException;
10+
import org.xmlpull.v1.XmlPullParserException;
11+
12+
public class XxeSecurityTest {
13+
14+
@Test
15+
public void when_using_general_persister_should_fail() throws Exception {
16+
Serializer serializer = new Persister();
17+
18+
try {
19+
serializer.read(MyObject.class, XxeSecurityTest.class.getResourceAsStream("xxeSecurityTest.xml"));
20+
Assert.fail("An exception should be thrown");
21+
} catch (Exception e) {
22+
// fine, test is ok
23+
}
24+
}
25+
26+
@Test(expected = ValueRequiredException.class)
27+
public void when_using_streamProvider_should_fail() throws Exception {
28+
checkProvider(new StreamProvider());
29+
}
30+
31+
@Test(expected = XmlPullParserException.class)
32+
public void when_using_pullProvider_should_fail() throws Exception {
33+
checkProvider(new PullProvider());
34+
}
35+
36+
@Test(expected = RuntimeException.class)
37+
public void when_using_documentProvider_should_fail() throws Exception {
38+
checkProvider(new DocumentProvider());
39+
}
40+
41+
private void checkProvider(Provider provider) throws Exception {
42+
Serializer serializer = new Persister();
43+
44+
EventReader eventReader = provider.provide(XxeSecurityTest.class.getResourceAsStream("xxeSecurityTest.xml"));
45+
InputNode inputNode = new NodeReader(eventReader).readRoot();
46+
47+
serializer.read(MyObject.class, inputNode);
48+
}
49+
50+
@Root
51+
public static class MyObject {
52+
53+
@Element
54+
public String message;
55+
56+
public MyObject() {
57+
this.message = "aaa";
58+
}
59+
60+
public MyObject(String message) {
61+
this.message = message;
62+
}
63+
}
64+
}

‎src/test/resources/org/simpleframework/xml/stream/xxeSecurityTest.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?xml version="1.0" encoding="ISO-8859-1"?>
2+
<!DOCTYPE bbb [<!ELEMENT foo ANY>
3+
<!ENTITY xxe SYSTEM "file:///etc" >
4+
]>
5+
<myObject><message>&xxe;</message></myObject>

0 commit comments

Comments
 (0)