diff --git a/core/src/main/java/org/camunda/spin/DataFormats.java b/core/src/main/java/org/camunda/spin/DataFormats.java index 28d391da..52241d61 100644 --- a/core/src/main/java/org/camunda/spin/DataFormats.java +++ b/core/src/main/java/org/camunda/spin/DataFormats.java @@ -120,20 +120,25 @@ protected void ensureDataformatsInitialized() { } public void registerDataFormats(ClassLoader classloader) { - registerDataFormats(classloader, Collections.EMPTY_LIST); + registerDataFormats(classloader, Collections.EMPTY_LIST, Collections.EMPTY_MAP); } public void registerDataFormats(ClassLoader classloader, List configurators) { + registerDataFormats(classloader, configurators, Collections.EMPTY_MAP); + } + public void registerDataFormats(ClassLoader classloader, + List configurators, + Map configurationProperties) { - Map> dataFormats = new HashMap>(); + Map> dataFormats = new HashMap<>(); if(classloader == null) { classloader = DataFormats.class.getClassLoader(); } // discover available custom dataformat providers on the classpath - registerCustomDataFormats(dataFormats, classloader); + registerCustomDataFormats(dataFormats, classloader, configurationProperties); // discover and apply data format configurators on the classpath applyConfigurators(dataFormats, classloader, configurators); @@ -144,16 +149,29 @@ public void registerDataFormats(ClassLoader classloader, } protected void registerCustomDataFormats(Map> dataFormats, ClassLoader classloader) { + registerCustomDataFormats(dataFormats, classloader, Collections.EMPTY_MAP); + } + + protected void registerCustomDataFormats(Map> dataFormats, + ClassLoader classloader, + Map configurationProperties) { // use java.util.ServiceLoader to load custom DataFormatProvider instances on the classpath ServiceLoader providerLoader = ServiceLoader.load(DataFormatProvider.class, classloader); for (DataFormatProvider provider : providerLoader) { LOG.logDataFormatProvider(provider); - registerProvider(dataFormats, provider); + registerProvider(dataFormats, provider, configurationProperties); } } - protected void registerProvider(Map> dataFormats, DataFormatProvider provider) { + protected void registerProvider(Map> dataFormats, + DataFormatProvider provider) { + registerProvider(dataFormats, provider, Collections.EMPTY_MAP); + } + + protected void registerProvider(Map> dataFormats, + DataFormatProvider provider, + Map configurationProperties) { String dataFormatName = provider.getDataFormatName(); @@ -161,7 +179,7 @@ protected void registerProvider(Map> dataFormats, DataForm throw LOG.multipleProvidersForDataformat(dataFormatName); } else { - DataFormat dataFormatInstance = provider.createInstance(); + DataFormat dataFormatInstance = provider.createInstance(configurationProperties); dataFormats.put(dataFormatName, dataFormatInstance); } } @@ -211,4 +229,8 @@ public static void loadDataFormats(ClassLoader classloader, List createInstance(); + /** + * @return an instance of the data format provided by this implementation configured using the + * passed configuration properties. + */ + default DataFormat createInstance(Map configurationProperties) { + return createInstance(); + }; + } diff --git a/core/src/test/java/org/camunda/spin/spi/CustomDataFormatProvider.java b/core/src/test/java/org/camunda/spin/spi/CustomDataFormatProvider.java index 83c921e1..9c7ca135 100644 --- a/core/src/test/java/org/camunda/spin/spi/CustomDataFormatProvider.java +++ b/core/src/test/java/org/camunda/spin/spi/CustomDataFormatProvider.java @@ -17,6 +17,8 @@ package org.camunda.spin.spi; +import java.util.Map; + /** * @author Thorben Lindhauer * @@ -36,5 +38,12 @@ public DataFormat createInstance() { return DATA_FORMAT; } + @Override + public DataFormat createInstance(Map configurationProperties) { + boolean conditionalProperty = (boolean) configurationProperties.getOrDefault("conditional-prop", false); + DATA_FORMAT.setConditionalProperty(conditionalProperty); + return DATA_FORMAT; + } + } diff --git a/core/src/test/java/org/camunda/spin/spi/DataFormatLoadingTest.java b/core/src/test/java/org/camunda/spin/spi/DataFormatLoadingTest.java index 357fa326..edb5d9b6 100644 --- a/core/src/test/java/org/camunda/spin/spi/DataFormatLoadingTest.java +++ b/core/src/test/java/org/camunda/spin/spi/DataFormatLoadingTest.java @@ -135,6 +135,25 @@ public void testRegisterDataFormatWithConfiguratorList() { .isEqualTo(ExampleCustomDataFormatConfigurator.UPDATED_PROPERTY); } + @Test + @PrepareForTest(DataFormats.class) + public void shouldPassConfiguratorPropertiesToProvider() { + // given a custom data format provider that is returned by the service loader API + mockProviders(new CustomDataFormatProvider()); + mockConfigurators(); + + // when a map of configuration properties is passed to the "load" method + DataFormats.getInstance().registerDataFormats(DataFormats.class.getClassLoader(), + Collections.EMPTY_LIST, + Collections.singletonMap("conditional-prop", true)); + + // then the configuration property is applied + ExampleCustomDataFormat customFormat = (ExampleCustomDataFormat) DataFormats + .getDataFormat(CustomDataFormatProvider.NAME); + assertThat(customFormat.isConditionalProperty()) + .isTrue(); + } + protected void mockProviders(final DataFormatProvider... providers) { when(mockServiceLoader.iterator()).thenAnswer(new Answer>() { diff --git a/core/src/test/java/org/camunda/spin/spi/ExampleCustomDataFormat.java b/core/src/test/java/org/camunda/spin/spi/ExampleCustomDataFormat.java index 79a60fc8..9fd34385 100644 --- a/core/src/test/java/org/camunda/spin/spi/ExampleCustomDataFormat.java +++ b/core/src/test/java/org/camunda/spin/spi/ExampleCustomDataFormat.java @@ -26,6 +26,7 @@ public class ExampleCustomDataFormat implements DataFormat { protected String name; protected String property = "defaultValue"; + protected boolean conditionalProperty = false; public ExampleCustomDataFormat(String name) { this.name = name; @@ -69,4 +70,11 @@ public void setProperty(String property) { this.property = property; } + public boolean isConditionalProperty() { + return conditionalProperty; + } + + public void setConditionalProperty(boolean conditionalProperty) { + this.conditionalProperty = conditionalProperty; + } } diff --git a/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormat.java b/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormat.java index 0ea7886c..6655da92 100644 --- a/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormat.java +++ b/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormat.java @@ -16,9 +16,14 @@ */ package org.camunda.spin.impl.xml.dom.format; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.TransformerFactory; +import java.util.Collections; +import java.util.Map; + import org.camunda.spin.impl.xml.dom.DomXmlAttribute; import org.camunda.spin.impl.xml.dom.DomXmlElement; import org.camunda.spin.impl.xml.dom.DomXmlLogger; @@ -38,6 +43,17 @@ public class DomXmlDataFormat implements DataFormat { protected static final DomXmlLogger LOG = DomXmlLogger.XML_DOM_LOGGER; + protected static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities"; + protected static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl"; + protected static final String LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + protected static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities"; + protected static final String JAXP_ACCESS_EXTERNAL_SCHEMA = "http://javax.xml.XMLConstants/property/accessExternalSchema"; + protected static final String JAXP_ACCESS_EXTERNAL_SCHEMA_SYSTEM_PROPERTY = "javax.xml.accessExternalSchema"; + protected static final String JAXP_ACCESS_EXTERNAL_SCHEMA_ALL = "all"; + + public static final String XXE_PROPERTY = "xxe-processing"; + public static final String SP_PROPERTY = "secure-processing"; + /** the DocumentBuilderFactory used by the reader */ protected DocumentBuilderFactory documentBuilderFactory; @@ -57,11 +73,17 @@ public DomXmlDataFormat(String name) { this(name, defaultDocumentBuilderFactory()); } + public DomXmlDataFormat(String name, Map configurationProperties) { + this(name, configurableDocumentBuilderFactory(configurationProperties)); + } + public DomXmlDataFormat(String name, JaxBContextProvider contextProvider) { this(name, defaultDocumentBuilderFactory(), contextProvider); } - public DomXmlDataFormat(String name, DocumentBuilderFactory documentBuilderFactory, JaxBContextProvider contextProvider) { + public DomXmlDataFormat(String name, + DocumentBuilderFactory documentBuilderFactory, + JaxBContextProvider contextProvider) { this(name, documentBuilderFactory, defaultTransformerFactory(), contextProvider); } @@ -69,7 +91,10 @@ public DomXmlDataFormat(String name, DocumentBuilderFactory documentBuilderFacto this(name, documentBuilderFactory, defaultTransformerFactory(), defaultJaxBContextProvider()); } - public DomXmlDataFormat(String name, DocumentBuilderFactory documentBuilderFactory, TransformerFactory transformerFactory, JaxBContextProvider contextProvider) { + public DomXmlDataFormat(String name, + DocumentBuilderFactory documentBuilderFactory, + TransformerFactory transformerFactory, + JaxBContextProvider contextProvider) { this.name = name; this.documentBuilderFactory = documentBuilderFactory; LOG.usingDocumentBuilderFactory(documentBuilderFactory.getClass().getName()); @@ -147,6 +172,11 @@ public static TransformerFactory defaultTransformerFactory() { } public static DocumentBuilderFactory defaultDocumentBuilderFactory() { + return configurableDocumentBuilderFactory(Collections.emptyMap()); + } + + public static DocumentBuilderFactory configurableDocumentBuilderFactory( + Map configurationProperties) { DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); @@ -162,6 +192,14 @@ public static DocumentBuilderFactory defaultDocumentBuilderFactory() { documentBuilderFactory.setIgnoringElementContentWhitespace(false); LOG.documentBuilderFactoryConfiguration("ignoringElementContentWhitespace", "false"); + if ((boolean) configurationProperties.getOrDefault(XXE_PROPERTY, false) == false) { + disableXxeProcessing(documentBuilderFactory); + } + + if ((boolean) configurationProperties.getOrDefault(SP_PROPERTY, true) == true) { + enableSecureProcessing(documentBuilderFactory); + } + return documentBuilderFactory; } @@ -169,4 +207,60 @@ public static JaxBContextProvider defaultJaxBContextProvider() { return new DefaultJaxBContextProvider(); } + /* + * Configures the DocumentBuilderFactory in a way, that it is protected against + * XML External Entity Attacks. If the implementing parser does not support one or + * multiple features, the failed feature is ignored. The parser might not be protected, + * if the feature assignment fails. + * + * @see OWASP Information of XXE attacks + * + * @param dbf The factory to configure. + */ + protected static void disableXxeProcessing(DocumentBuilderFactory dbf) { + try { + dbf.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); + dbf.setFeature(LOAD_EXTERNAL_DTD, false); + dbf.setFeature(EXTERNAL_PARAMETER_ENTITIES, false); + } catch (ParserConfigurationException ignored) { + // ignore + } + dbf.setXIncludeAware(false); + dbf.setExpandEntityReferences(false); + } + + /* + * Configures the DocumentBuilderFactory to process XML securely. + * If the implementing parser does not support one or multiple features, + * the failed feature is ignored. The parser might not be protected, + * if the feature assignment fails. + * + * @param dbf The factory to configure. + */ + protected static void enableSecureProcessing(DocumentBuilderFactory dbf) { + try { + dbf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + dbf.setAttribute(JAXP_ACCESS_EXTERNAL_SCHEMA, resolveAccessExternalSchemaProperty()); + } catch (ParserConfigurationException | IllegalArgumentException ignored) { + // ignored + } + } + + /* + * JAXP allows users to override the default value via system properties and + * a central properties file (see https://docs.oracle.com/javase/tutorial/jaxp/properties/scope.html). + * However, both are overridden by an explicit configuration in code, as we apply it. + * Since we want users to customize the value, we take the system property into account. + * The properties file is not supported at the moment. + */ + protected static String resolveAccessExternalSchemaProperty() { + String systemProperty = System.getProperty(JAXP_ACCESS_EXTERNAL_SCHEMA_SYSTEM_PROPERTY); + + if (systemProperty != null) { + return systemProperty; + } else { + return JAXP_ACCESS_EXTERNAL_SCHEMA_ALL; + } + } } diff --git a/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormatProvider.java b/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormatProvider.java index a19bb760..6ce33264 100644 --- a/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormatProvider.java +++ b/dataformat-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormatProvider.java @@ -16,6 +16,8 @@ */ package org.camunda.spin.impl.xml.dom.format; +import java.util.Map; + import org.camunda.spin.DataFormats; import org.camunda.spin.spi.DataFormat; import org.camunda.spin.spi.DataFormatProvider; @@ -36,4 +38,9 @@ public DataFormat createInstance() { return new DomXmlDataFormat(DataFormats.XML_DATAFORMAT_NAME); } + @Override + public DataFormat createInstance(Map configurationProperties) { + return new DomXmlDataFormat(DataFormats.XML_DATAFORMAT_NAME, configurationProperties); + } + } diff --git a/dataformat-xml-dom/src/test/java/org/camunda/spin/xml/dom/format/spi/DomXmlDataFormatProtectionTest.java b/dataformat-xml-dom/src/test/java/org/camunda/spin/xml/dom/format/spi/DomXmlDataFormatProtectionTest.java new file mode 100644 index 00000000..f3198ff6 --- /dev/null +++ b/dataformat-xml-dom/src/test/java/org/camunda/spin/xml/dom/format/spi/DomXmlDataFormatProtectionTest.java @@ -0,0 +1,70 @@ +/* + * Copyright Camunda Services GmbH and/or licensed to Camunda Services GmbH + * under one or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information regarding copyright + * ownership. Camunda licenses this file to you under the Apache License, + * Version 2.0; you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.camunda.spin.xml.dom.format.spi; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.io.InputStream; +import java.io.InputStreamReader; + +import org.camunda.spin.DataFormats; +import org.camunda.spin.impl.xml.dom.format.DomXmlDataFormat; +import org.camunda.spin.xml.SpinXmlDataFormatException; +import org.junit.BeforeClass; +import org.junit.Test; + +public class DomXmlDataFormatProtectionTest { + + protected static DomXmlDataFormat format; + + @BeforeClass + public static void setUpMocks() { + format = (DomXmlDataFormat) DataFormats.xml(); + } + + @Test + public void shouldThrowExceptionForTooManyAttributes() { + // given + String testXml = "org/camunda/spin/xml/dom/format/spi/FeatureSecureProcessing.xml"; + InputStream testXmlAsStream = this.getClass().getClassLoader().getResourceAsStream(testXml); + + // when + assertThatThrownBy(() -> { + format.getReader().readInput(new InputStreamReader(testXmlAsStream)); + }) + // then + .isInstanceOf(SpinXmlDataFormatException.class); + } + + @Test + public void shouldThrowExceptionForDoctype() { + // given + String testXml = "org/camunda/spin/xml/dom/format/spi/XxeProcessing.xml"; + InputStream testXmlAsStream = this.getClass().getClassLoader().getResourceAsStream(testXml); + + // when + assertThatThrownBy(() -> { + format.getReader().readInput(new InputStreamReader(testXmlAsStream)); + }) + // then + .isInstanceOf(SpinXmlDataFormatException.class) + .hasMessageContaining("SPIN/DOM-XML-01009 Unable to parse input into DOM document") + .hasStackTraceContaining("DOCTYPE") + .hasStackTraceContaining("http://apache.org/xml/features/disallow-doctype-decl"); + } + +} diff --git a/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/ExternalSchemaAccess.xml b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/ExternalSchemaAccess.xml new file mode 100644 index 00000000..f6fd7546 --- /dev/null +++ b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/ExternalSchemaAccess.xml @@ -0,0 +1,9 @@ + + + + + diff --git a/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/FeatureSecureProcessing.xml b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/FeatureSecureProcessing.xml new file mode 100644 index 00000000..c97ef317 --- /dev/null +++ b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/FeatureSecureProcessing.xml @@ -0,0 +1,9 @@ + + + + + diff --git a/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/XxeProcessing.xml b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/XxeProcessing.xml new file mode 100644 index 00000000..32d03dd5 --- /dev/null +++ b/dataformat-xml-dom/src/test/resources/org/camunda/spin/xml/dom/format/spi/XxeProcessing.xml @@ -0,0 +1,14 @@ + + + ]> + + + + + egg1 egg2 &xxe; + egg3 + +