From 1d1450028630d52b0e178a787f8597c811545e4c Mon Sep 17 00:00:00 2001 From: Thomas Bayer Date: Wed, 18 Dec 2024 20:31:57 +0100 Subject: [PATCH 1/3] Added check for external entity references. Block if there are some. --- .../xmlprotection/XMLProtectionException.java | 7 +++ .../xmlprotection/XMLProtector.java | 60 +++++++++++++------ .../XMLProtectionInterceptorTest.java | 42 +++++++++---- .../xmlprotection/XMLProtectorTest.java | 25 +++----- 4 files changed, 87 insertions(+), 47 deletions(-) create mode 100644 core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionException.java diff --git a/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionException.java b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionException.java new file mode 100644 index 0000000000..66edf2efb9 --- /dev/null +++ b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionException.java @@ -0,0 +1,7 @@ +package com.predic8.membrane.core.interceptor.xmlprotection; + +public class XMLProtectionException extends Exception { + public XMLProtectionException(String message) { + super(message); + } +} diff --git a/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java index f9a968818a..d33664733d 100644 --- a/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java +++ b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java @@ -14,20 +14,16 @@ package com.predic8.membrane.core.interceptor.xmlprotection; -import java.io.InputStreamReader; -import java.io.OutputStreamWriter; -import java.util.Iterator; +import org.jetbrains.annotations.*; +import org.slf4j.*; -import javax.xml.stream.XMLEventReader; -import javax.xml.stream.XMLEventWriter; -import javax.xml.stream.XMLInputFactory; -import javax.xml.stream.XMLOutputFactory; -import javax.xml.stream.XMLStreamException; -import javax.xml.stream.events.StartElement; -import javax.xml.stream.events.XMLEvent; +import javax.xml.stream.*; +import javax.xml.stream.events.*; +import java.io.*; +import java.util.*; +import java.util.function.*; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import static javax.xml.stream.XMLInputFactory.*; /** * Filters XML streams, removing potentially malicious elements: @@ -46,9 +42,9 @@ public class XMLProtector { private static Logger log = LoggerFactory.getLogger(XMLProtector.class.getName()); private static XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); static { - xmlInputFactory.setProperty(XMLInputFactory.IS_REPLACING_ENTITY_REFERENCES, false); - xmlInputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); - xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD,false); + xmlInputFactory.setProperty(IS_REPLACING_ENTITY_REFERENCES, false); + xmlInputFactory.setProperty(IS_SUPPORTING_EXTERNAL_ENTITIES, false); + xmlInputFactory.setProperty(SUPPORT_DTD,false); } private XMLEventWriter writer; @@ -63,10 +59,16 @@ public XMLProtector(OutputStreamWriter osw, boolean removeDTD, int maxElementNam this.maxAttibuteCount = maxAttibuteCount; if(!removeDTD) - xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD,true); + xmlInputFactory.setProperty(SUPPORT_DTD,true); } - public boolean protect(InputStreamReader isr) { + /** + * Is XML secure? + * @param isr + * @return false if there is any security problem in the XML + * @throws XMLProtectionException if there are critical issues like external entity references + */ + public boolean protect(InputStreamReader isr) throws XMLProtectionException { try { XMLEventReader parser; synchronized(xmlInputFactory) { @@ -91,7 +93,9 @@ public boolean protect(InputStreamReader isr) { return false; } } - } if (event instanceof javax.xml.stream.events.DTD) { + } + if (event instanceof javax.xml.stream.events.DTD dtd) { + checkExternalEntities(dtd); if (removeDTD) { log.debug("removed DTD."); continue; @@ -107,4 +111,24 @@ public boolean protect(InputStreamReader isr) { return true; } + private static void checkExternalEntities(DTD dtd) throws XMLProtectionException { + if (containsExternalEntityReferences(dtd)) { + String msg = "Possible attack. External entity found in DTD."; + log.warn(msg); + throw new XMLProtectionException(msg); + } + } + + private static boolean containsExternalEntityReferences(DTD dtd) { + var entities = dtd.getEntities(); + if (entities == null || entities.isEmpty()) + return false; + + return entities.stream().anyMatch(isExternalEntity()); + } + + private static @NotNull Predicate isExternalEntity() { + return ed -> ed.getPublicId() != null || ed.getSystemId() != null; + } + } diff --git a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java index 9ca1e88911..0fcb2471ea 100644 --- a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java +++ b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java @@ -14,15 +14,15 @@ package com.predic8.membrane.core.interceptor.xmlprotection; -import static org.junit.jupiter.api.Assertions.*; - -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Test; +import com.predic8.membrane.core.exchange.*; +import com.predic8.membrane.core.http.*; +import com.predic8.membrane.core.interceptor.*; +import com.predic8.membrane.core.util.*; +import org.junit.jupiter.api.*; -import com.predic8.membrane.core.exchange.Exchange; -import com.predic8.membrane.core.interceptor.Outcome; -import com.predic8.membrane.core.util.ByteUtil; -import com.predic8.membrane.core.util.MessageUtil; +import static com.predic8.membrane.core.http.MimeType.*; +import static com.predic8.membrane.core.interceptor.Outcome.*; +import static org.junit.jupiter.api.Assertions.*; public class XMLProtectionInterceptorTest { private static Exchange exc; @@ -38,21 +38,39 @@ public static void setUp() throws Exception { } private void runOn(String resource, boolean expectSuccess) throws Exception { - exc.getRequest().getHeader().setContentType("application/xml"); + exc.getRequest().getHeader().setContentType(APPLICATION_XML); exc.getRequest().setBodyContent(ByteUtil.getByteArrayData(this.getClass().getResourceAsStream(resource))); Outcome outcome = interceptor.handleRequest(exc); - assertEquals(expectSuccess ? Outcome.CONTINUE : Outcome.ABORT, outcome); + assertEquals(expectSuccess ? CONTINUE : ABORT, outcome); } @Test - public void testInvariant() throws Exception { + void testInvariant() throws Exception { runOn("/customer.xml", true); } @Test - public void testNotWellformed() throws Exception { + void testNotWellformed() throws Exception { runOn("/xml/not-wellformed.xml", false); } + @Test + void removeDTD() throws Exception { + exc.setRequest(Request.post("/").body(""" + + + ]> + + """).contentType(APPLICATION_XML).build()); + // Should pass + assertEquals(CONTINUE, interceptor.handleRequest(exc)); + + // Should still contain the XML + assertTrue(exc.getRequest().getBodyAsStringDecoded().contains(" input.length / 2); assertTrue(new String(output).contains("ENTITY")); } @Test - public void testExternalEntities() throws Exception { - assertTrue(runOn("/xml/entity-external.xml", false)); - assertTrue(output.length > input.length * 2 / 3); - assertTrue(new String(output).contains("ENTITY")); + void externalEntities() throws Exception { + assertThrows(XMLProtectionException.class, () -> runOn("/xml/entity-external.xml", false)); } @Test - public void testLongElementName() throws Exception { + void longElementName() throws Exception { assertFalse(runOn("/xml/long-element-name.xml")); } @Test - public void testManyAttributes() throws Exception { + void manyAttributes() throws Exception { assertFalse(runOn("/xml/many-attributes.xml")); } } From ae487109a9aba47865986f10a36a26a9ba5d9260 Mon Sep 17 00:00:00 2001 From: Thomas Bayer Date: Wed, 18 Dec 2024 20:33:45 +0100 Subject: [PATCH 2/3] Minor refactorings --- .../core/interceptor/xmlprotection/XMLProtector.java | 8 ++++---- .../core/interceptor/xmlprotection/XMLProtectorTest.java | 7 +++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java index d33664733d..7efd597648 100644 --- a/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java +++ b/core/src/main/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtector.java @@ -39,15 +39,15 @@ * an error response should be returned to the requestor. */ public class XMLProtector { - private static Logger log = LoggerFactory.getLogger(XMLProtector.class.getName()); - private static XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); + private static final Logger log = LoggerFactory.getLogger(XMLProtector.class.getName()); + private static final XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); static { xmlInputFactory.setProperty(IS_REPLACING_ENTITY_REFERENCES, false); xmlInputFactory.setProperty(IS_SUPPORTING_EXTERNAL_ENTITIES, false); xmlInputFactory.setProperty(SUPPORT_DTD,false); } - private XMLEventWriter writer; + private final XMLEventWriter writer; private final int maxAttibuteCount; private final int maxElementNameLength; private final boolean removeDTD; @@ -64,7 +64,7 @@ public XMLProtector(OutputStreamWriter osw, boolean removeDTD, int maxElementNam /** * Is XML secure? - * @param isr + * @param isr Stream with XML * @return false if there is any security problem in the XML * @throws XMLProtectionException if there are critical issues like external entity references */ diff --git a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java index 0da2b85eb4..f44d70442b 100644 --- a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java +++ b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectorTest.java @@ -25,8 +25,7 @@ public class XMLProtectorTest { private static final Logger LOG = LoggerFactory.getLogger(XMLProtectorTest.class); - private XMLProtector xmlProtector; - private byte[] input, output; + private byte[] input, output; private boolean runOn(String resource) throws Exception { return runOn(resource, true); @@ -34,7 +33,7 @@ private boolean runOn(String resource) throws Exception { private boolean runOn(String resource, boolean removeDTD) throws Exception { ByteArrayOutputStream baos = new ByteArrayOutputStream(); - xmlProtector = new XMLProtector(new OutputStreamWriter(baos, UTF_8), removeDTD, 1000, 1000); + XMLProtector xmlProtector = new XMLProtector(new OutputStreamWriter(baos, UTF_8), removeDTD, 1000, 1000); input = ByteUtil.getByteArrayData(this.getClass().getResourceAsStream(resource)); if (resource.endsWith("lmx")) { @@ -89,7 +88,7 @@ void expandingEntities() throws Exception { } @Test - void externalEntities() throws Exception { + void externalEntities() { assertThrows(XMLProtectionException.class, () -> runOn("/xml/entity-external.xml", false)); } From 3b9257ce03c139ae44e39fe3053930786cb746f8 Mon Sep 17 00:00:00 2001 From: Thomas Bayer Date: Thu, 19 Dec 2024 08:50:09 +0100 Subject: [PATCH 3/3] Formatted code --- .../XMLProtectionInterceptorTest.java | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java index 0fcb2471ea..2ccbc165e5 100644 --- a/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java +++ b/core/src/test/java/com/predic8/membrane/core/interceptor/xmlprotection/XMLProtectionInterceptorTest.java @@ -25,52 +25,52 @@ import static org.junit.jupiter.api.Assertions.*; public class XMLProtectionInterceptorTest { - private static Exchange exc; - private static XMLProtectionInterceptor interceptor; - - @BeforeAll - public static void setUp() throws Exception { - exc = new Exchange(null); - exc.setRequest(MessageUtil.getGetRequest("/axis2/services/BLZService")); - exc.setOriginalHostHeader("thomas-bayer.com:80"); - - interceptor = new XMLProtectionInterceptor(); - } - - private void runOn(String resource, boolean expectSuccess) throws Exception { - exc.getRequest().getHeader().setContentType(APPLICATION_XML); - exc.getRequest().setBodyContent(ByteUtil.getByteArrayData(this.getClass().getResourceAsStream(resource))); - Outcome outcome = interceptor.handleRequest(exc); - assertEquals(expectSuccess ? CONTINUE : ABORT, outcome); - } - - @Test - void testInvariant() throws Exception { - runOn("/customer.xml", true); - } - - @Test - void testNotWellformed() throws Exception { - runOn("/xml/not-wellformed.xml", false); - } - - @Test - void removeDTD() throws Exception { - exc.setRequest(Request.post("/").body(""" - - - ]> - - """).contentType(APPLICATION_XML).build()); - - // Should pass + private static Exchange exc; + private static XMLProtectionInterceptor interceptor; + + @BeforeAll + public static void setUp() throws Exception { + exc = new Exchange(null); + exc.setRequest(MessageUtil.getGetRequest("/axis2/services/BLZService")); + exc.setOriginalHostHeader("thomas-bayer.com:80"); + + interceptor = new XMLProtectionInterceptor(); + } + + private void runOn(String resource, boolean expectSuccess) throws Exception { + exc.getRequest().getHeader().setContentType(APPLICATION_XML); + exc.getRequest().setBodyContent(ByteUtil.getByteArrayData(this.getClass().getResourceAsStream(resource))); + Outcome outcome = interceptor.handleRequest(exc); + assertEquals(expectSuccess ? CONTINUE : ABORT, outcome); + } + + @Test + void testInvariant() throws Exception { + runOn("/customer.xml", true); + } + + @Test + void testNotWellformed() throws Exception { + runOn("/xml/not-wellformed.xml", false); + } + + @Test + void removeDTD() throws Exception { + exc.setRequest(Request.post("/").body(""" + + + ]> + + """).contentType(APPLICATION_XML).build()); + + // Should pass assertEquals(CONTINUE, interceptor.handleRequest(exc)); - // Should still contain the XML - assertTrue(exc.getRequest().getBodyAsStringDecoded().contains("