Skip to content

Commit 2863a49

Browse files
cekiceki
committed
prevent Server-Side Request Forgery (SSRF) attacks by ignoring external DTD files in DOCTYPE
Signed-off-by: Ceki Gulcu <[email protected]>
1 parent b44b940 commit 2863a49

3 files changed

Lines changed: 85 additions & 14 deletions

File tree

‎logback-core/src/main/java/ch/qos/logback/core/joran/event/SaxEventRecorder.java‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import static ch.qos.logback.core.CoreConstants.XML_PARSING;
1717

18+
import java.io.ByteArrayInputStream;
1819
import java.io.IOException;
1920
import java.io.InputStream;
2021
import java.util.ArrayList;
@@ -40,28 +41,56 @@
4041

4142
public class SaxEventRecorder extends DefaultHandler implements ContextAware {
4243

44+
// org.xml.sax.ext.LexicalHandler is an optional interface
4345
final ContextAwareImpl contextAwareImpl;
4446
final ElementPath elementPath;
4547
List<SaxEvent> saxEventList = new ArrayList<SaxEvent>();
4648
Locator locator;
4749

50+
4851
public SaxEventRecorder(Context context) {
4952
this(context, new ElementPath());
5053
}
5154

55+
5256
public SaxEventRecorder(Context context, ElementPath elementPath) {
5357
contextAwareImpl = new ContextAwareImpl(context, this);
5458
this.elementPath = elementPath;
5559
}
5660

61+
/**
62+
* An implementation which disallows external DTDs
63+
*
64+
* @param publicId The public identifier, or null if none is
65+
* available.
66+
* @param systemId The system identifier provided in the XML
67+
* document.
68+
* @return
69+
* @throws SAXException
70+
* @throws IOException
71+
* @since 1.5.13
72+
*/
73+
@Override
74+
public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException {
75+
addWarn("Document Type Declaration (DOCTYPE) with external file reference is");
76+
addWarn("disallowed to prevent Server-Side Request Forgery (SSRF) attacks.");
77+
addWarn("returning contents of SYSTEM " +systemId+ " as a white space");
78+
return new InputSource(new ByteArrayInputStream(" ".getBytes()));
79+
}
80+
5781
final public void recordEvents(InputStream inputStream) throws JoranException {
5882
recordEvents(new InputSource(inputStream));
5983
}
6084

6185
public void recordEvents(InputSource inputSource) throws JoranException {
6286
SAXParser saxParser = buildSaxParser();
6387
try {
88+
// the following sax property can be set in order to add 'this' as LexicalHandler to the saxParser
89+
// However, this is not needed as long as resolveEntity() method is implemented as above
90+
// saxParser.setProperty("http://xml.org/sax/properties/lexical-handler", this);
91+
6492
saxParser.parse(inputSource, this);
93+
6594
return;
6695
} catch (IOException ie) {
6796
handleError("I/O error occurred while parsing xml file", ie);
@@ -83,7 +112,7 @@ private SAXParser buildSaxParser() throws JoranException {
83112
try {
84113
SAXParserFactory spf = SAXParserFactory.newInstance();
85114
spf.setValidating(false);
86-
// spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
115+
//spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
87116
// See LOGBACK-1465
88117
spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
89118
spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
@@ -93,11 +122,11 @@ private SAXParser buildSaxParser() throws JoranException {
93122
String errMsg = "Error during SAX paser configuration. See https://logback.qos.ch/codes.html#saxParserConfiguration";
94123
addError(errMsg, pce);
95124
throw new JoranException(errMsg, pce);
96-
} catch (SAXException pce) {
125+
} catch (SAXException pce) {
97126
String errMsg = "Error during parser creation or parser configuration";
98127
addError(errMsg, pce);
99128
throw new JoranException(errMsg, pce);
100-
}
129+
}
101130
}
102131

103132
public void startDocument() {
@@ -116,7 +145,6 @@ protected boolean shouldIgnoreForElementPath(String tagName) {
116145
}
117146

118147
public void startElement(String namespaceURI, String localName, String qName, Attributes atts) {
119-
120148
String tagName = getTagName(localName, qName);
121149
if (!shouldIgnoreForElementPath(tagName)) {
122150
elementPath.push(tagName);
@@ -169,20 +197,17 @@ String getTagName(String localName, String qName) {
169197
}
170198

171199
public void error(SAXParseException spe) throws SAXException {
172-
addError(XML_PARSING + " - Parsing error on line " + spe.getLineNumber() + " and column "
173-
+ spe.getColumnNumber());
200+
addError(XML_PARSING + " - Parsing error on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber());
174201
addError(spe.toString());
175202
}
176203

177204
public void fatalError(SAXParseException spe) throws SAXException {
178-
addError(XML_PARSING + " - Parsing fatal error on line " + spe.getLineNumber() + " and column "
179-
+ spe.getColumnNumber());
205+
addError(XML_PARSING + " - Parsing fatal error on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber());
180206
addError(spe.toString());
181207
}
182208

183209
public void warning(SAXParseException spe) throws SAXException {
184-
addWarn(XML_PARSING + " - Parsing warning on line " + spe.getLineNumber() + " and column "
185-
+ spe.getColumnNumber(), spe);
210+
addWarn(XML_PARSING + " - Parsing warning on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber(), spe);
186211
}
187212

188213
public void addError(String msg) {

‎logback-core/src/test/input/joran/event-ssrf.xml‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?xml version="1.0" encoding="UTF-8" ?>
2+
<!--
3+
~ Logback: the reliable, generic, fast and flexible logging framework.
4+
~ Copyright (C) 1999-2024, QOS.ch. All rights reserved.
5+
~
6+
~ This program and the accompanying materials are dual-licensed under
7+
~ either the terms of the Eclipse Public License v1.0 as published by
8+
~ the Eclipse Foundation
9+
~
10+
~ or (per the licensee's choosing)
11+
~
12+
~ under the terms of the GNU Lesser General Public License version 2.1
13+
~ as published by the Free Software Foundation.
14+
-->
15+
16+
<!DOCTYPE test SYSTEM "http://192.168.1.100/">
17+
<test>
18+
19+
<!-- this action throws an exception in the Action.begin method -->
20+
<badBegin>
21+
<touch/>
22+
<touch/>
23+
</badBegin>
24+
25+
<hello name="John Doe">XXX&amp;</hello>
26+
27+
</test>

‎logback-core/src/test/java/ch/qos/logback/core/joran/event/SaxEventRecorderTest.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,16 @@
1515

1616
import java.io.FileInputStream;
1717
import java.util.List;
18+
import java.util.concurrent.TimeUnit;
1819

1920
import javax.xml.parsers.SAXParser;
2021
import javax.xml.parsers.SAXParserFactory;
2122

23+
import ch.qos.logback.core.util.StatusPrinter;
24+
import ch.qos.logback.core.util.StatusPrinter2;
2225
import org.junit.jupiter.api.Assertions;
2326
import org.junit.jupiter.api.Test;
27+
import org.junit.jupiter.api.Timeout;
2428
import org.xml.sax.Attributes;
2529

2630
import ch.qos.logback.core.Context;
@@ -31,7 +35,7 @@
3135

3236
/**
3337
* Test whether SaxEventRecorder does a good job.
34-
*
38+
*
3539
* @author Ceki Gulcu
3640
*/
3741
public class SaxEventRecorderTest {
@@ -59,15 +63,30 @@ public void dump(List<SaxEvent> seList) {
5963
}
6064

6165
@Test
62-
public void test1() throws Exception {
66+
public void testEvent1() throws Exception {
67+
System.out.println("test1");
6368
List<SaxEvent> seList = doTest("event1.xml");
69+
StatusPrinter.print(context);
6470
Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO);
6571
// dump(seList);
6672
Assertions.assertEquals(11, seList.size());
6773
}
6874

75+
@Test()
76+
@Timeout(value = 500, unit = TimeUnit.MILLISECONDS) // timeout in case attack is not prevented
77+
public void testEventSSRF() throws Exception {
78+
try {
79+
List<SaxEvent> seList = doTest("event-ssrf.xml");
80+
Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.WARN);
81+
statusChecker.assertContainsMatch(Status.WARN, "Document Type Declaration");
82+
Assertions.assertEquals(11, seList.size());
83+
} finally {
84+
StatusPrinter.print(context);
85+
}
86+
}
87+
6988
@Test
70-
public void test2() throws Exception {
89+
public void testEventAmp() throws Exception {
7190
List<SaxEvent> seList = doTest("ampEvent.xml");
7291
Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO);
7392
// dump(seList);
@@ -78,7 +97,7 @@ public void test2() throws Exception {
7897
}
7998

8099
@Test
81-
public void test3() throws Exception {
100+
public void testInc() throws Exception {
82101
List<SaxEvent> seList = doTest("inc.xml");
83102
Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO);
84103
// dump(seList);

0 commit comments

Comments
 (0)