From f175f1f1f663d29fc151c297b56d154255eb7016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louise=20S=C3=B6derstr=C3=B6m?= Date: Fri, 3 Feb 2023 09:19:16 +0100 Subject: [PATCH] [AJmycukR] vuln-fix: Securing XML parser against XXE (CVE-2023-23926) Fixing a XML External Entity (XXE) vulnerability, that was impacting the apoc.import.graphml procedure. --- .../apoc/export/graphml/XmlGraphMLReader.java | 11 +++++++ .../export/graphml/ExportGraphMLTest.java | 29 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java b/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java index 485127f6d..775c6d2e0 100644 --- a/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java +++ b/core/src/main/java/apoc/export/graphml/XmlGraphMLReader.java @@ -200,6 +200,8 @@ public long parseXML(Reader input) throws XMLStreamException { XMLInputFactory inputFactory = XMLInputFactory.newInstance(); inputFactory.setProperty("javax.xml.stream.isCoalescing", true); inputFactory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true); + inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + inputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); XMLEventReader reader = inputFactory.createXMLEventReader(input); Entity last = null; Map nodeKeys = new HashMap<>(); @@ -212,10 +214,15 @@ public long parseXML(Reader input) throws XMLStreamException { XMLEvent event; try { event = (XMLEvent) reader.next(); + if ( event.getEventType() == XMLStreamConstants.DTD) { + generateXmlDoctypeException(); + } } catch (Exception e) { // in case of unicode invalid chars we skip the event, or we exit in case of EOF if (e.getMessage().contains("Unexpected EOF")) { break; + } else if (e.getMessage().contains("DOCTYPE")) { + throw e; } continue; } @@ -410,4 +417,8 @@ private String getAttribute(StartElement element, QName qname) { Attribute attribute = element.getAttributeByName(qname); return attribute != null ? attribute.getValue() : null; } + + private RuntimeException generateXmlDoctypeException() { + throw new RuntimeException("XML documents with a DOCTYPE are not allowed."); + } } diff --git a/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java b/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java index c4449c18f..904a9137f 100644 --- a/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java +++ b/core/src/test/java/apoc/export/graphml/ExportGraphMLTest.java @@ -49,6 +49,7 @@ import static apoc.util.BinaryTestUtil.fileToBinary; import static apoc.util.MapUtil.map; import static apoc.util.TestUtil.isRunningInCI; +import static apoc.util.TestUtil.testResult; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -735,6 +736,34 @@ public void testExportGraphmlQueryWithStringCaptionCamelCase() { assertXMLEquals(output, EXPECTED_TYPES_PATH_CAMEL_CASE); } + @Test + public void testImportGraphmlPreventXXEVulnerabilityThrowsQueryExecutionException() { + QueryExecutionException e = assertThrows(QueryExecutionException.class, + () -> testResult(db, "CALL apoc.import.graphml('" + TestUtil.getUrlFileName("xml/xxe.xml") + "', {})", (r) -> { + r.next(); + r.close(); + }) + ); + + Throwable except = ExceptionUtils.getRootCause(e); + assertTrue(except instanceof RuntimeException); + assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed."); + } + + @Test + public void testImportGraphmlPreventBillionLaughVulnerabilityThrowsQueryExecutionException() { + QueryExecutionException e = assertThrows(QueryExecutionException.class, + () -> testResult(db, "CALL apoc.import.graphml('" + TestUtil.getUrlFileName("xml/billion_laughs.xml") + "', {})", (r) -> { + r.next(); + r.close(); + }) + ); + + Throwable except = ExceptionUtils.getRootCause(e); + assertTrue(except instanceof RuntimeException); + assertEquals(except.getMessage(), "XML documents with a DOCTYPE are not allowed."); + } + private void assertResults(File output, Map r, final String source) { assertCommons(r); assertEquals(output.getAbsolutePath(), r.get("file"));