diff --git a/logback-core/src/main/java/ch/qos/logback/core/joran/event/SaxEventRecorder.java b/logback-core/src/main/java/ch/qos/logback/core/joran/event/SaxEventRecorder.java index 1a6df6118f..eb658bd433 100644 --- a/logback-core/src/main/java/ch/qos/logback/core/joran/event/SaxEventRecorder.java +++ b/logback-core/src/main/java/ch/qos/logback/core/joran/event/SaxEventRecorder.java @@ -15,6 +15,7 @@ import static ch.qos.logback.core.CoreConstants.XML_PARSING; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; @@ -40,20 +41,43 @@ public class SaxEventRecorder extends DefaultHandler implements ContextAware { + // org.xml.sax.ext.LexicalHandler is an optional interface final ContextAwareImpl contextAwareImpl; final ElementPath elementPath; List saxEventList = new ArrayList(); Locator locator; + public SaxEventRecorder(Context context) { this(context, new ElementPath()); } + public SaxEventRecorder(Context context, ElementPath elementPath) { contextAwareImpl = new ContextAwareImpl(context, this); this.elementPath = elementPath; } + /** + * An implementation which disallows external DTDs + * + * @param publicId The public identifier, or null if none is + * available. + * @param systemId The system identifier provided in the XML + * document. + * @return + * @throws SAXException + * @throws IOException + * @since 1.5.13 + */ + @Override + public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException { + addWarn("Document Type Declaration (DOCTYPE) with external file reference is"); + addWarn("disallowed to prevent Server-Side Request Forgery (SSRF) attacks."); + addWarn("returning contents of SYSTEM " +systemId+ " as a white space"); + return new InputSource(new ByteArrayInputStream(" ".getBytes())); + } + final public void recordEvents(InputStream inputStream) throws JoranException { recordEvents(new InputSource(inputStream)); } @@ -61,7 +85,12 @@ final public void recordEvents(InputStream inputStream) throws JoranException { public void recordEvents(InputSource inputSource) throws JoranException { SAXParser saxParser = buildSaxParser(); try { + // the following sax property can be set in order to add 'this' as LexicalHandler to the saxParser + // However, this is not needed as long as resolveEntity() method is implemented as above + // saxParser.setProperty("http://xml.org/sax/properties/lexical-handler", this); + saxParser.parse(inputSource, this); + return; } catch (IOException ie) { handleError("I/O error occurred while parsing xml file", ie); @@ -83,7 +112,7 @@ private SAXParser buildSaxParser() throws JoranException { try { SAXParserFactory spf = SAXParserFactory.newInstance(); spf.setValidating(false); - // spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + //spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); // See LOGBACK-1465 spf.setFeature("http://xml.org/sax/features/external-general-entities", false); spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); @@ -93,11 +122,11 @@ private SAXParser buildSaxParser() throws JoranException { String errMsg = "Error during SAX paser configuration. See https://logback.qos.ch/codes.html#saxParserConfiguration"; addError(errMsg, pce); throw new JoranException(errMsg, pce); - } catch (SAXException pce) { + } catch (SAXException pce) { String errMsg = "Error during parser creation or parser configuration"; addError(errMsg, pce); throw new JoranException(errMsg, pce); - } + } } public void startDocument() { @@ -116,7 +145,6 @@ protected boolean shouldIgnoreForElementPath(String tagName) { } public void startElement(String namespaceURI, String localName, String qName, Attributes atts) { - String tagName = getTagName(localName, qName); if (!shouldIgnoreForElementPath(tagName)) { elementPath.push(tagName); @@ -169,20 +197,17 @@ String getTagName(String localName, String qName) { } public void error(SAXParseException spe) throws SAXException { - addError(XML_PARSING + " - Parsing error on line " + spe.getLineNumber() + " and column " - + spe.getColumnNumber()); + addError(XML_PARSING + " - Parsing error on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber()); addError(spe.toString()); } public void fatalError(SAXParseException spe) throws SAXException { - addError(XML_PARSING + " - Parsing fatal error on line " + spe.getLineNumber() + " and column " - + spe.getColumnNumber()); + addError(XML_PARSING + " - Parsing fatal error on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber()); addError(spe.toString()); } public void warning(SAXParseException spe) throws SAXException { - addWarn(XML_PARSING + " - Parsing warning on line " + spe.getLineNumber() + " and column " - + spe.getColumnNumber(), spe); + addWarn(XML_PARSING + " - Parsing warning on line " + spe.getLineNumber() + " and column " + spe.getColumnNumber(), spe); } public void addError(String msg) { diff --git a/logback-core/src/test/input/joran/event-ssrf.xml b/logback-core/src/test/input/joran/event-ssrf.xml new file mode 100644 index 0000000000..5b5b57812f --- /dev/null +++ b/logback-core/src/test/input/joran/event-ssrf.xml @@ -0,0 +1,27 @@ + + + + + + + + + + + + + XXX& + + \ No newline at end of file diff --git a/logback-core/src/test/java/ch/qos/logback/core/joran/event/SaxEventRecorderTest.java b/logback-core/src/test/java/ch/qos/logback/core/joran/event/SaxEventRecorderTest.java index 2dcca24c7d..4dc486be31 100755 --- a/logback-core/src/test/java/ch/qos/logback/core/joran/event/SaxEventRecorderTest.java +++ b/logback-core/src/test/java/ch/qos/logback/core/joran/event/SaxEventRecorderTest.java @@ -15,12 +15,16 @@ import java.io.FileInputStream; import java.util.List; +import java.util.concurrent.TimeUnit; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; +import ch.qos.logback.core.util.StatusPrinter; +import ch.qos.logback.core.util.StatusPrinter2; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; import org.xml.sax.Attributes; import ch.qos.logback.core.Context; @@ -31,7 +35,7 @@ /** * Test whether SaxEventRecorder does a good job. - * + * * @author Ceki Gulcu */ public class SaxEventRecorderTest { @@ -59,15 +63,30 @@ public void dump(List seList) { } @Test - public void test1() throws Exception { + public void testEvent1() throws Exception { + System.out.println("test1"); List seList = doTest("event1.xml"); + StatusPrinter.print(context); Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO); // dump(seList); Assertions.assertEquals(11, seList.size()); } + @Test() + @Timeout(value = 500, unit = TimeUnit.MILLISECONDS) // timeout in case attack is not prevented + public void testEventSSRF() throws Exception { + try { + List seList = doTest("event-ssrf.xml"); + Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.WARN); + statusChecker.assertContainsMatch(Status.WARN, "Document Type Declaration"); + Assertions.assertEquals(11, seList.size()); + } finally { + StatusPrinter.print(context); + } + } + @Test - public void test2() throws Exception { + public void testEventAmp() throws Exception { List seList = doTest("ampEvent.xml"); Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO); // dump(seList); @@ -78,7 +97,7 @@ public void test2() throws Exception { } @Test - public void test3() throws Exception { + public void testInc() throws Exception { List seList = doTest("inc.xml"); Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO); // dump(seList);