fix(spin): add configurable xml parsing protection

* Add XXE attack protection to the XML parser of the Spin XML DataFormat. * Add secure processing to the XML parser of the Spin XML DataFormat. Related to CAM-14011, closes #137
camunda · Dec 10, 2021 · 1551b80 · 1551b80
1 parent 7a327b1
commit 1551b80
Show file tree

Hide file tree

Showing 11 changed files with 279 additions and 8 deletions.
diff --git 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<DataFormatConfigurator> configurators) {
+    registerDataFormats(classloader, configurators, Collections.EMPTY_MAP);
+  }
+  public void registerDataFormats(ClassLoader classloader,
+                                  List<DataFormatConfigurator> configurators,
+                                  Map<String, Object> configurationProperties) {
 
-    Map<String, DataFormat<?>> dataFormats = new HashMap<String, DataFormat<?>>();
+    Map<String, DataFormat<?>> 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,24 +149,37 @@ public void registerDataFormats(ClassLoader classloader,
   }
 
   protected void registerCustomDataFormats(Map<String, DataFormat<?>> dataFormats, ClassLoader classloader) {
+    registerCustomDataFormats(dataFormats, classloader, Collections.EMPTY_MAP);
+  }
+
+  protected void registerCustomDataFormats(Map<String, DataFormat<?>> dataFormats, 
+                                           ClassLoader classloader, 
+                                           Map<String, Object> configurationProperties) {
     // use java.util.ServiceLoader to load custom DataFormatProvider instances on the classpath
     ServiceLoader<DataFormatProvider> providerLoader = ServiceLoader.load(DataFormatProvider.class, classloader);
 
     for (DataFormatProvider provider : providerLoader) {
       LOG.logDataFormatProvider(provider);
-      registerProvider(dataFormats, provider);
+      registerProvider(dataFormats, provider, configurationProperties);
     }
   }
 
-  protected void registerProvider(Map<String, DataFormat<?>> dataFormats, DataFormatProvider provider) {
+  protected void registerProvider(Map<String, DataFormat<?>> dataFormats,
+                                  DataFormatProvider provider) {
+    registerProvider(dataFormats, provider, Collections.EMPTY_MAP);
+  }
+
+  protected void registerProvider(Map<String, DataFormat<?>> dataFormats,
+                                  DataFormatProvider provider,
+                                  Map<String, Object> configurationProperties) {
 
     String dataFormatName = provider.getDataFormatName();
 
     if(dataFormats.containsKey(dataFormatName)) {
       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<DataFormatConfi
     INSTANCE.registerDataFormats(classloader, configurators);
   }
 
+  public static void loadDataFormats(ClassLoader classloader, Map configurationProperties) {
+    INSTANCE.registerDataFormats(classloader, Collections.EMPTY_LIST, configurationProperties);
+  }
+
 }
diff --git a/core/src/main/java/org/camunda/spin/spi/DataFormatProvider.java b/core/src/main/java/org/camunda/spin/spi/DataFormatProvider.java
@@ -16,6 +16,8 @@
  */
 package org.camunda.spin.spi;
 
+import java.util.Map;
+
 /**
  * @author Daniel Meyer
  *
@@ -32,4 +34,12 @@ public interface DataFormatProvider {
    */
   DataFormat<?> createInstance();
 
+  /**
+   * @return an instance of the data format provided by this implementation configured using the
+   * passed configuration properties.
+   */
+  default DataFormat<?> createInstance(Map<String, Object> 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
@@ -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<String, Object> 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
@@ -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<Iterator<DataFormatProvider>>() {
 

diff --git 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<SpinJsonNode> {
 
   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
@@ -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<SpinXmlElement> {
 
   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,19 +73,28 @@ public DomXmlDataFormat(String name) {
     this(name, defaultDocumentBuilderFactory());
   }
 
+  public DomXmlDataFormat(String name, Map<String, Object> 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);
   }
 
   public DomXmlDataFormat(String name, DocumentBuilderFactory documentBuilderFactory) {
     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<String,Object> configurationProperties) {
 
     DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
 
@@ -162,11 +192,75 @@ 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;
   }
 
   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 <a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet">OWASP Information of XXE attacks</a>
+   *
+   * @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/...-xml-dom/src/main/java/org/camunda/spin/impl/xml/dom/format/DomXmlDataFormatProvider.java b/...-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<String, Object> configurationProperties) {
+    return new DomXmlDataFormat(DataFormats.XML_DATAFORMAT_NAME, configurationProperties);
+  }
+
 }