From 345a8534ff39cc9550cbacabe2b3fffe20293508 Mon Sep 17 00:00:00 2001 From: Thomas Lenz Date: Thu, 22 Jun 2017 14:26:15 +0200 Subject: implement a workaround to fix XXE and SSRF problems in an old XMLStreamParser implementation of a third party library --- utils/pom.xml | 6 ++ .../java/at/gv/egiz/slbinding/SLUnmarshaller.java | 70 +++++++++++++++++++--- .../at/gv/egiz/slbinding/UnmarshallCXSRTest.java | 26 +++++++- .../CreateXMLSignatureResponse_with_Attacke.xml | 25 ++++++++ 4 files changed, 119 insertions(+), 8 deletions(-) create mode 100644 utils/src/test/resources/at/gv/egiz/slbinding/CreateXMLSignatureResponse_with_Attacke.xml (limited to 'utils') diff --git a/utils/pom.xml b/utils/pom.xml index 5a41e98e..95d7f655 100644 --- a/utils/pom.xml +++ b/utils/pom.xml @@ -37,6 +37,12 @@ com.sun.xml.stream 1.0.2 + + + commons-io + commons-io + 2.5 + diff --git a/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java b/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java index de1b2ddf..2d1808c8 100644 --- a/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java +++ b/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java @@ -25,7 +25,10 @@ package at.gv.egiz.slbinding; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStreamReader; +import java.io.Reader; import java.net.URL; import java.util.Arrays; import java.util.Collection; @@ -38,6 +41,8 @@ import javax.xml.bind.JAXBException; import javax.xml.bind.UnmarshalException; import javax.xml.bind.Unmarshaller; import javax.xml.bind.ValidationEvent; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.stream.XMLEventReader; import javax.xml.stream.XMLInputFactory; import javax.xml.stream.XMLStreamException; @@ -46,6 +51,7 @@ import javax.xml.transform.stream.StreamSource; import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; +import org.apache.commons.io.IOUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.xml.sax.SAXException; @@ -237,17 +243,67 @@ public class SLUnmarshaller { public Object unmarshal(StreamSource source) throws XMLStreamException, JAXBException { ReportingValidationEventHandler validationEventHandler = new ReportingValidationEventHandler(); -// System.setProperty("javax.xml.stream.XMLInputFactory", "com.sun.xml.stream.ZephyrParserFactory"); -// System.setProperty("com.sun.xml.stream.ZephyrParserFactory", "com.sun.xml.stream.ZephyrParserFactory"); -// XMLInputFactory inputFactory = XMLInputFactory.newInstance("com.sun.xml.stream.ZephyrParserFactory", null); - + Reader inputReader = source.getReader(); + + if (inputReader instanceof InputStreamReader) { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + try { + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + } catch (ParserConfigurationException e) { + log.error("Can NOT set Xerces parser security features. -> XML parsing is possible insecure!!!! ", e); + + } + + try { + //create copy of input stream + InputStreamReader isReader = (InputStreamReader) inputReader; + String encoding = isReader.getEncoding(); + byte[] backup = IOUtils.toByteArray(isReader, encoding); + + //validate input stream + dbf.newDocumentBuilder().parse(new ByteArrayInputStream(backup)); + + //create new inputStreamReader for reak processing + inputReader = new InputStreamReader(new ByteArrayInputStream(backup), encoding); + + + } catch (SAXException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + + } catch (ParserConfigurationException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + } catch (IOException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + } + + } else { + log.error("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + log.error("Reader is not of type InputStreamReader -> can not make a copy of the InputStream --> extended XML validation is not possible!!! "); + log.error("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); + + } + + //parse XML with original functionality XMLInputFactory inputFactory = XMLInputFactory.newInstance(); //disallow DTD and external entities inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false); inputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false); - XMLEventReader eventReader = inputFactory.createXMLEventReader(source.getReader()); + + + XMLEventReader eventReader = inputFactory.createXMLEventReader(inputReader); RedirectEventFilter redirectEventFilter = new RedirectEventFilter(); XMLEventReader filteredReader = inputFactory.createFilteredReader(eventReader, redirectEventFilter); @@ -255,8 +311,8 @@ public Object unmarshal(StreamSource source) throws XMLStreamException, JAXBExce unmarshaller.setEventHandler(validationEventHandler); unmarshaller.setListener(new RedirectUnmarshallerListener(redirectEventFilter)); - unmarshaller.setSchema(slSchema); - + unmarshaller.setSchema(slSchema); + Object object; try { log.trace("Before unmarshal()."); diff --git a/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java b/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java index 99c11cbe..62a8d622 100644 --- a/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java +++ b/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java @@ -25,6 +25,7 @@ package at.gv.egiz.slbinding; +import java.io.BufferedInputStream; import java.io.InputStream; import java.io.InputStreamReader; @@ -49,7 +50,7 @@ public class UnmarshallCXSRTest { assertNotNull(s); SLUnmarshaller unmarshaller = new SLUnmarshaller(); - Object object = unmarshaller.unmarshal(new StreamSource(new InputStreamReader(s))); + Object object = unmarshaller.unmarshal(new StreamSource(new InputStreamReader(new BufferedInputStream(s)))); assertTrue(object.getClass().getName(), object instanceof JAXBElement); @@ -59,4 +60,27 @@ public class UnmarshallCXSRTest { } + @Test + public void testUnmarshalCreateXMLSignatureResponseWithDocTypeXXEOrSSRF() throws JAXBException { + + ClassLoader cl = UnmarshallCXSRTest.class.getClassLoader(); + InputStream s = cl.getResourceAsStream("at/gv/egiz/slbinding/CreateXMLSignatureResponse_with_Attacke.xml"); + + assertNotNull(s); + + SLUnmarshaller unmarshaller = new SLUnmarshaller(); + Object object; + try { + object = unmarshaller.unmarshal(new StreamSource(new InputStreamReader(new BufferedInputStream(s)))); + + assertTrue(object.getClass().getName(), object instanceof JAXBElement); + Object value = ((JAXBElement) object).getValue(); + assertFalse(value.getClass().getName(), value instanceof CreateXMLSignatureResponseType); + + } catch (XMLStreamException e) { + assertTrue(e.getClass().getName(), e instanceof XMLStreamException); + + } + } + } diff --git a/utils/src/test/resources/at/gv/egiz/slbinding/CreateXMLSignatureResponse_with_Attacke.xml b/utils/src/test/resources/at/gv/egiz/slbinding/CreateXMLSignatureResponse_with_Attacke.xml new file mode 100644 index 00000000..8684d860 --- /dev/null +++ b/utils/src/test/resources/at/gv/egiz/slbinding/CreateXMLSignatureResponse_with_Attacke.xml @@ -0,0 +1,25 @@ + + + %sp; + %param1; + %exfil; +]>7Dp/5KcvUfCnkohkOOzvFaeAIRc=fCbFrz0xI0wiN+PPn4leURvfdIo=Zozx+mW/lHUO8q02DBK3Aud/sSpVdWGjfBScZDBjuzLyQyrRlXH2xo3lij5/xJa0MIIDdzCCAd+gAwIBAgIRMqGxalf5fUuhqgSjs+IArBMwDQYJKoZIhvcNAQEFBQAw +TTESMBAGA1UEAwwJVlNpZyBDQSAyMSowKAYDVQQKDCFIYXVwdHZlcmJhbmQgw7Zz +dGVyci4gU296aWFsdmVycy4xCzAJBgNVBAYTAkFUMB4XDTA2MTEwODA1MjYxN1oX +DTExMTEwODA1MjYxN1owYTEXMBUGA1UEAwwOTWFydGluIENlbnRuZXIxKjAoBgNV +BAoMIUhhdXB0dmVyYmFuZCDDtnN0ZXJyLiBTb3ppYWx2ZXJzLjENMAsGA1UECwwE +VlNpZzELMAkGA1UEBhMCQVQwSTATBgcqhkjOPQIBBggqhkjOPQMBAQMyAASZohyZ +R1JDH+sANEROtE5LQFFepjfo5Xk7eRtrpnfa1MFhEOfYXxElEInOVFU049+jgZgw +gZUwEwYDVR0jBAwwCoAISGl1XDyvIyowEQYDVR0OBAoECESRXVYYhLE8MA4GA1Ud +DwEB/wQEAwIGwDAWBgNVHSAEDzANMAsGCSooAAoBBAFmADBDBggrBgEFBQcBAQQ3 +MDUwMwYIKwYBBQUHMAGGJ2h0dHA6Ly9vY3NwLmVjYXJkLnNvemlhbHZlcnNpY2hl +cnVuZy5hdDANBgkqhkiG9w0BAQUFAAOCAYEAPTDL/MLfmNw0VcHqny3lNi30hL8z +OtyiwQRo7QFA98Pm+8WPyQjyK0UVIej+NZVIVSU7WdYWVuu+au8bd3B10WBikLMl +QfEWqYDHGp+bfB4GB4WVeS78tNmXaacXjzLqae/KLALRn/dVBN/acf3C+Ey3kSYw +/96J+qgbaowlT18OvUTs1ABHgut1x31hLIgTj0R5nzfOOUXXnUN+rWm5SuaNMTHW +NMNhM6Y4jfACOsudmboeIZfgrmbDtCa2lLU95Mct2dcbBsnMRFUYoZc+9eEI/xCH +JdzFZp1DAyqzb6Y84YUr+QDCxJT5BVdU0zTI73t0ls64556ifsfq/2sixHeQgMSM +z/qQfPUC9so32sDPNHHNbKVYx9m0VpPwekWXBEVJWFffQbPe55deZ+uVFLOG4y0G +c+o3eXV2Vs9te1OoA+KRow8kjL7iil06DNOddeDQVPj7zqRQtoLKMLTJflfZp5pd +UPEZNM5Pw92T501vzHO9JNv5f/Wp3PTskBNJ2010-04-20T06:08:36ZGF2imE3FjjqwM8BH0RY+VjtiAI8=C=AT,O=Hauptverband österr. Sozialvers.,CN=VSig CA 217229045246817736659347185373920056355859text/plain \ No newline at end of file -- cgit v1.2.3 From 0603c0fbdfe028113431c65590b6e7e28929f6f6 Mon Sep 17 00:00:00 2001 From: Thomas Lenz Date: Fri, 23 Jun 2017 06:50:14 +0200 Subject: some small refactoring and code documentation --- utils/src/main/java/at/gv/egiz/dom/DOMUtils.java | 78 +++++++++++++++++++++- .../java/at/gv/egiz/slbinding/SLUnmarshaller.java | 52 +++++++-------- .../at/gv/egiz/slbinding/UnmarshallCXSRTest.java | 6 ++ 3 files changed, 104 insertions(+), 32 deletions(-) (limited to 'utils') diff --git a/utils/src/main/java/at/gv/egiz/dom/DOMUtils.java b/utils/src/main/java/at/gv/egiz/dom/DOMUtils.java index eae8f05e..2054021a 100644 --- a/utils/src/main/java/at/gv/egiz/dom/DOMUtils.java +++ b/utils/src/main/java/at/gv/egiz/dom/DOMUtils.java @@ -33,7 +33,8 @@ import java.io.StringWriter; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.stream.XMLStreamException; import javax.xml.transform.OutputKeys; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; @@ -47,7 +48,8 @@ import org.w3c.dom.Document; import org.w3c.dom.Node; import org.w3c.dom.Text; import org.w3c.dom.bootstrap.DOMImplementationRegistry; -import org.w3c.dom.ls.DOMImplementationLS; +import org.w3c.dom.ls.DOMImplementationLS; +import org.xml.sax.SAXException; public final class DOMUtils { @@ -160,6 +162,76 @@ public final class DOMUtils { base64OutputStream.close(); return doc.createTextNode(outputStream.toString("ASCII")); - } + } + + /** + * Set XML parser features to {@link DocumentBuilderFactory} to prevent XXE, XEE and SSRF attacks + *
+ *
+ * These features are set by this method: + * + * + * + * @param dbf {@link DocumentBuilderFactory} on which the features should be registered + */ + public static void setXMLParserFlagsAgainstXXEAndSSRFAttacks(DocumentBuilderFactory dbf) { + try { + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + } catch (ParserConfigurationException e) { + log.error("Can NOT set Xerces parser security features. -> XML parsing is possible insecure!!!! ", e); + + } + + } + + /** + * Parse an {@link InputStream} that contains a XML document into a {@link Document} + * This method set all features to prevent XXE, XEE and SSRF attacks + * + * These features are set by this method: + * + * + * @param is {@link InputStream} that contains a serialized XML document + * @return Deserialized {@link Document} from input XML + * @throws XMLStreamException XML parser has an error + * @throws IOException + */ + public static Document validateXMLAgainstXXEAndSSRFAttacks(InputStream is) throws XMLStreamException, IOException { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + setXMLParserFlagsAgainstXXEAndSSRFAttacks(dbf); + + try { + //validate input stream + return dbf.newDocumentBuilder().parse(is); + + } catch (SAXException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + } catch (ParserConfigurationException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + } catch (IOException e) { + log.error("XML data validation FAILED with msg: " + e.getMessage(), e); + throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); + + } + + } } diff --git a/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java b/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java index 2d1808c8..489c36d8 100644 --- a/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java +++ b/utils/src/main/java/at/gv/egiz/slbinding/SLUnmarshaller.java @@ -57,6 +57,7 @@ import org.slf4j.LoggerFactory; import org.xml.sax.SAXException; import at.gv.egiz.bku.utils.ClasspathURLStreamHandler; +import at.gv.egiz.dom.DOMUtils; import at.gv.egiz.validation.ReportingValidationEventHandler; public class SLUnmarshaller { @@ -241,43 +242,28 @@ public class SLUnmarshaller { * @throws JAXBException */ public Object unmarshal(StreamSource source) throws XMLStreamException, JAXBException { + Reader inputReader = source.getReader(); - ReportingValidationEventHandler validationEventHandler = new ReportingValidationEventHandler(); - Reader inputReader = source.getReader(); - - if (inputReader instanceof InputStreamReader) { - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - - try { - dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); - dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - - } catch (ParserConfigurationException e) { - log.error("Can NOT set Xerces parser security features. -> XML parsing is possible insecure!!!! ", e); - - } - + /* Validate XML against XXE, XEE, and SSRF + * + * This pre-validation step is required because com.sun.xml.stream.sjsxp-1.0.2 XML stream parser library does not + * support all XML parser features to prevent these types of attacks + */ + if (inputReader instanceof InputStreamReader) { try { //create copy of input stream InputStreamReader isReader = (InputStreamReader) inputReader; String encoding = isReader.getEncoding(); byte[] backup = IOUtils.toByteArray(isReader, encoding); - //validate input stream - dbf.newDocumentBuilder().parse(new ByteArrayInputStream(backup)); + //validate input stream + DOMUtils.validateXMLAgainstXXEAndSSRFAttacks(new ByteArrayInputStream(backup)); //create new inputStreamReader for reak processing inputReader = new InputStreamReader(new ByteArrayInputStream(backup), encoding); - } catch (SAXException e) { - log.error("XML data validation FAILED with msg: " + e.getMessage(), e); - throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); - - - } catch (ParserConfigurationException e) { + } catch (XMLStreamException e) { log.error("XML data validation FAILED with msg: " + e.getMessage(), e); throw new XMLStreamException("XML data validation FAILED with msg: " + e.getMessage(), e); @@ -294,20 +280,28 @@ public Object unmarshal(StreamSource source) throws XMLStreamException, JAXBExce } - //parse XML with original functionality + /* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + * parse XML with original functionality + * + * This code implements the the original mocca XML processing by using + * com.sun.xml.stream.sjsxp-1.0.2 XML stream parser library. Currently, this library is required to get full + * security-layer specific XML processing. However, there this lib does not fully support XXE, XEE and SSRF + * prevention mechanisms (e.g.: XMLInputFactory.SUPPORT_DTD flag is not used) + * + * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! + */ XMLInputFactory inputFactory = XMLInputFactory.newInstance(); //disallow DTD and external entities inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false); inputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false); - - - + XMLEventReader eventReader = inputFactory.createXMLEventReader(inputReader); RedirectEventFilter redirectEventFilter = new RedirectEventFilter(); XMLEventReader filteredReader = inputFactory.createFilteredReader(eventReader, redirectEventFilter); Unmarshaller unmarshaller = jaxbContext.createUnmarshaller(); + ReportingValidationEventHandler validationEventHandler = new ReportingValidationEventHandler(); unmarshaller.setEventHandler(validationEventHandler); unmarshaller.setListener(new RedirectUnmarshallerListener(redirectEventFilter)); diff --git a/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java b/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java index 62a8d622..5f97be0f 100644 --- a/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java +++ b/utils/src/test/java/at/gv/egiz/slbinding/UnmarshallCXSRTest.java @@ -77,6 +77,12 @@ public class UnmarshallCXSRTest { Object value = ((JAXBElement) object).getValue(); assertFalse(value.getClass().getName(), value instanceof CreateXMLSignatureResponseType); + /* If the parser has no exception and no CreateXMLSignatureResponseType than the test fails, because + * the tested XML document contains a CreateXMLSignatureResponseType and an XXE, SSRF attack vector. + * Consequently, the parser result has to be an error + */ + assertFalse(true); + } catch (XMLStreamException e) { assertTrue(e.getClass().getName(), e instanceof XMLStreamException); -- cgit v1.2.3