prevent Server-Side Request Forgery (SSRF) attacks by ignoring external DTD files in DOCTYPE

Signed-off-by: Ceki Gulcu <ceki@qos.ch>

Author: ceki

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,28 +41,56 @@
 
 public class SaxEventRecorder extends DefaultHandler implements ContextAware {
 
+    // org.xml.sax.ext.LexicalHandler is an optional interface
     final ContextAwareImpl contextAwareImpl;
     final ElementPath elementPath;
     List<SaxEvent> saxEventList = new ArrayList<SaxEvent>();
     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));
     }
 
     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) {

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

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+  ~ Logback: the reliable, generic, fast and flexible logging framework.
+  ~ Copyright (C) 1999-2024, QOS.ch. All rights reserved.
+  ~
+  ~ This program and the accompanying materials are dual-licensed under
+  ~ either the terms of the Eclipse Public License v1.0 as published by
+  ~ the Eclipse Foundation
+  ~
+  ~   or (per the licensee's choosing)
+  ~
+  ~ under the terms of the GNU Lesser General Public License version 2.1
+  ~ as published by the Free Software Foundation.
+  -->
+
+<!DOCTYPE test SYSTEM "http://192.168.1.100/">
+<test>
+
+    <!-- this action throws an exception in the Action.begin method -->
+    <badBegin>
+        <touch/>
+        <touch/>
+    </badBegin>
+
+    <hello name="John Doe">XXX&amp;</hello>
+
+</test>

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<SaxEvent> seList) {
     }
 
     @Test
-    public void test1() throws Exception {
+    public void testEvent1() throws Exception {
+        System.out.println("test1");
         List<SaxEvent> 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<SaxEvent> 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<SaxEvent> 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<SaxEvent> seList = doTest("inc.xml");
         Assertions.assertTrue(statusChecker.getHighestLevel(0) == Status.INFO);
         // dump(seList);