guard XML parsing against XXE and billion laugh attacks

docs/checkstyle/opencast-checkstyle.xml

@@ -189,6 +189,73 @@
       <property name="severity" value="error" />
     </module>
 
+    <!-- Check for use of unsecured xml parsing           -->
+    <module name="Regexp">
+      <property name="format" value="TransformerFactory[.\s]+(?:newInstance|newDefaultInstance|newTransformer|newTemplates)" />
+      <property name="message"
+        value="Use the methods provided by the XmlSafeParser class for creating xml parsers, which are guarded against xxe and other attacks." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="DocumentBuilderFactory[.\s]+(?:newInstance|newDefaultInstance)" />
+      <property name="message"
+        value="Use the methods provided by the XmlSafeParser class for creating xml parsers, which are guarded against xxe and other attacks." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="SAXParserFactory[.\s]+(?:newInstance|newDefaultInstance)" />
+      <property name="message"
+        value="Use the methods provided by the XmlSafeParser class for creating xml parsers, which are guarded against xxe and other attacks." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="new\s+SAXBuilder" />
+      <property name="message"
+        value="Use the methods provided by the XmlSafeParser class for creating xml parsers, which are guarded against xxe and other attacks." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="\.\s*unmarshal\s*\(\s*(?!XmlSafeParser)" />
+      <property name="message"
+        value="Use the parse method provided by the XmlSafeParser class to guard against xxe, e.g.: .unmarshal(XmlSafeParser.parse(input))." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <!-- Additional parser                               -->
+    <module name="Regexp">
+      <property name="format" value="(?:XMLInputFactory|SAXReader|XPathExpressionFactory|XMLDecoder|ParserAdapter|XMLReaderAdapter|DOMParser|SAXTransformerFactory|XMLReaderFactory)" />
+      <property name="message"
+        value="It seems like an XmlParser has been added that is not protected against xxe by default. Please consider using one of the pre-existing secured parsers provided by the XmlSafeParser class in the common module, or add a securely configured version of your parser to this class." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="new\s+SAXParser" />
+      <property name="message"
+        value="It seems like an XmlParser has been added that is not protected against xxe by default. Please consider using one of the pre-existing secured parsers provided by the XmlSafeParser class in the common module, or add a securely configured version of your parser to this class." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+    <module name="Regexp">
+      <property name="format" value="SchemaFactory[.\s]+newInstance" />
+      <property name="message"
+        value="It seems like an XmlParser has been added that is not protected against xxe by default. Please consider using one of the pre-existing secured parsers provided by the XmlSafeParser class in the common module, or add a securely configured version of your parser to this class." />
+      <property name="illegalPattern" value="true" />
+      <property name="ignoreComments" value="true" />
+      <property name="severity" value="error" />
+    </module>
+
     <!-- Miscellaneous other checks.                   -->
     <!-- See http://checkstyle.sf.net/config_misc.html -->
     <module name="ArrayTypeStyle">

modules/authorization-manager/src/test/java/org/opencastproject/authorization/xacml/manager/impl/AclScannerTest.java

@@ -45,14 +45,13 @@
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
+import org.xml.sax.SAXParseException;
 
 import java.io.File;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 
-import javax.xml.bind.UnmarshalException;
-
 public class AclScannerTest {
 
   private AclDb aclDb;
@@ -160,7 +159,7 @@ public void testCorruptedFileInstall() throws Exception {
       aclScanner.install(file);
       fail("Should not be parsed.");
     } catch (XACMLParsingException e) {
-      assertTrue("The file can not be parsed.", e.getCause() instanceof UnmarshalException);
+      assertTrue("The file can not be parsed.", e.getCause() instanceof SAXParseException);
     }
   }
 
@@ -211,7 +210,7 @@ public void testCorruptedFileUpdate() throws Exception {
       aclScanner.update(file);
       fail("Should not be parsed.");
     } catch (XACMLParsingException e) {
-      assertTrue("The file can not be parsed.", e.getCause() instanceof UnmarshalException);
+      assertTrue("The file can not be parsed.", e.getCause() instanceof SAXParseException);
     }
   }
 

modules/authorization-xacml/src/main/java/org/opencastproject/authorization/xacml/XACMLUtils.java

@@ -24,6 +24,7 @@
 import org.opencastproject.mediapackage.MediaPackage;
 import org.opencastproject.security.api.AccessControlEntry;
 import org.opencastproject.security.api.AccessControlList;
+import org.opencastproject.util.XmlSafeParser;
 
 import org.jboss.security.xacml.core.model.policy.ActionMatchType;
 import org.jboss.security.xacml.core.model.policy.ActionType;
@@ -114,7 +115,7 @@ public static AccessControlList parseXacml(InputStream xacml) throws XACMLParsin
       @SuppressWarnings("unchecked")
       final AccessControlList acl = new AccessControlList();
       final List<AccessControlEntry> entries = acl.getEntries();
-      final PolicyType policy = ((JAXBElement<PolicyType>) XACMLUtils.jBossXacmlJaxbContext.createUnmarshaller().unmarshal(xacml)).getValue();
+      final PolicyType policy = ((JAXBElement<PolicyType>) XACMLUtils.jBossXacmlJaxbContext.createUnmarshaller().unmarshal(XmlSafeParser.parse(xacml))).getValue();
       for (Object object : policy.getCombinerParametersOrRuleCombinerParametersOrVariableDefinition()) {
 
         if (!(object instanceof RuleType)) {

modules/caption-impl/src/main/java/org/opencastproject/caption/converters/DFXPCaptionConverter.java

@@ -31,6 +31,7 @@
 import org.opencastproject.caption.util.TimeUtil;
 import org.opencastproject.mediapackage.MediaPackageElement;
 import org.opencastproject.mediapackage.MediaPackageElement.Type;
+import org.opencastproject.util.XmlSafeParser;
 
 import org.apache.commons.io.IOUtils;
 import org.slf4j.Logger;
@@ -52,7 +53,6 @@
 import java.util.List;
 
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.parsers.SAXParser;
 import javax.xml.parsers.SAXParserFactory;
@@ -89,8 +89,7 @@ public List<Caption> importCaption(InputStream in, String language) throws Capti
 
     Document doc;
     try {
-      DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-      DocumentBuilder builder = factory.newDocumentBuilder();
+      DocumentBuilder builder = XmlSafeParser.newDocumentBuilderFactory().newDocumentBuilder();
       doc = builder.parse(in);
       doc.getDocumentElement().normalize();
     } catch (ParserConfigurationException e) {
@@ -206,11 +205,10 @@ private String getTextCore(Node p) {
   @Override
   public void exportCaption(OutputStream outputStream, List<Caption> captions, String language) throws IOException {
     // get document builder factory and parse template
-    DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
     Document doc = null;
     InputStream is = null;
     try {
-      DocumentBuilder builder = factory.newDocumentBuilder();
+      DocumentBuilder builder = XmlSafeParser.newDocumentBuilderFactory().newDocumentBuilder();
       // load dfxp template from file
       is = DFXPCaptionConverter.class.getResourceAsStream("/templates/template.dfxp.xml");
       doc = builder.parse(is);
@@ -254,7 +252,7 @@ public void exportCaption(OutputStream outputStream, List<Caption> captions, Str
     OutputStreamWriter osw = new OutputStreamWriter(outputStream, "UTF-8");
     StreamResult result = new StreamResult(osw);
     DOMSource source = new DOMSource(doc);
-    TransformerFactory tfactory = TransformerFactory.newInstance();
+    TransformerFactory tfactory = XmlSafeParser.newTransformerFactory();
     Transformer transformer;
     try {
       transformer = tfactory.newTransformer();
@@ -283,7 +281,7 @@ public String[] getLanguageList(InputStream input) throws CaptionConverterExcept
     final List<String> langList = new LinkedList<String>();
 
     // get SAX parser
-    SAXParserFactory factory = SAXParserFactory.newInstance();
+    SAXParserFactory factory = XmlSafeParser.newSAXParserFactory();
     try {
       SAXParser parser = factory.newSAXParser();
       // create handler

modules/caption-impl/src/main/java/org/opencastproject/caption/converters/Mpeg7CaptionConverter.java

@@ -42,6 +42,7 @@
 import org.opencastproject.metadata.mpeg7.Mpeg7CatalogImpl;
 import org.opencastproject.metadata.mpeg7.TemporalDecomposition;
 import org.opencastproject.metadata.mpeg7.TextAnnotation;
+import org.opencastproject.util.XmlSafeParser;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -61,7 +62,6 @@
 import javax.xml.transform.Transformer;
 import javax.xml.transform.TransformerConfigurationException;
 import javax.xml.transform.TransformerException;
-import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.TransformerFactoryConfigurationError;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
@@ -219,7 +219,7 @@ public void exportCaption(OutputStream outputStream, List<Caption> captions, Str
 
     Transformer tf = null;
     try {
-      tf = TransformerFactory.newInstance().newTransformer();
+      tf = XmlSafeParser.newTransformerFactory().newTransformer();
       DOMSource xmlSource = new DOMSource(mpeg7.toXml());
       tf.transform(xmlSource, new StreamResult(outputStream));
     } catch (TransformerConfigurationException e) {

modules/caption-impl/src/main/java/org/opencastproject/caption/endpoint/CaptionServiceRestEndpoint.java

@@ -31,6 +31,7 @@
 import org.opencastproject.rest.AbstractJobProducerEndpoint;
 import org.opencastproject.serviceregistry.api.ServiceRegistry;
 import org.opencastproject.util.UrlSupport;
+import org.opencastproject.util.XmlSafeParser;
 import org.opencastproject.util.doc.rest.RestParameter;
 import org.opencastproject.util.doc.rest.RestQuery;
 import org.opencastproject.util.doc.rest.RestResponse;
@@ -53,9 +54,7 @@
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.transform.Transformer;
-import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
@@ -199,7 +198,7 @@ public Response languages(@FormParam("input") String inputType, @FormParam("capt
       String[] languageArray = service.getLanguageList((Catalog) element, inputType);
 
       // build response
-      DocumentBuilder docBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+      DocumentBuilder docBuilder = XmlSafeParser.newDocumentBuilderFactory().newDocumentBuilder();
       Document doc = docBuilder.newDocument();
       Element root = doc.createElement("languages");
       root.setAttribute("type", inputType);
@@ -213,8 +212,7 @@ public Response languages(@FormParam("input") String inputType, @FormParam("capt
       DOMSource domSource = new DOMSource(root);
       StringWriter writer = new StringWriter();
       StreamResult result = new StreamResult(writer);
-      Transformer transformer;
-      transformer = TransformerFactory.newInstance().newTransformer();
+      Transformer transformer = XmlSafeParser.newTransformerFactory().newTransformer();
       transformer.transform(domSource, result);
 
       return Response.status(Response.Status.OK).entity(writer.toString()).build();

modules/caption-remote/src/main/java/org/opencastproject/caption/remote/CaptionServiceRemoteImpl.java

@@ -30,6 +30,7 @@
 import org.opencastproject.mediapackage.MediaPackageElementParser;
 import org.opencastproject.mediapackage.MediaPackageException;
 import org.opencastproject.serviceregistry.api.RemoteBase;
+import org.opencastproject.util.XmlSafeParser;
 
 import org.apache.commons.lang3.StringUtils;
 import org.apache.http.HttpResponse;
@@ -46,8 +47,6 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import javax.xml.parsers.DocumentBuilderFactory;
-
 /**
  * Proxies a set of remote composer services for use as a JVM-local service. Remote services are selected at random.
  */
@@ -126,7 +125,7 @@ public String[] getLanguageList(MediaPackageElement input, String format)
       response = getResponse(post);
       if (response != null) {
         List<String> langauges = new ArrayList<String>();
-        Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder()
+        Document doc = XmlSafeParser.newDocumentBuilderFactory().newDocumentBuilder()
                 .parse(EntityUtils.toString(response.getEntity(), "UTF-8"));
         NodeList languages = doc.getElementsByTagName("languages");
         for (int i = 0; i < languages.getLength(); i++) {

modules/common/pom.xml

@@ -79,6 +79,10 @@
       <groupId>org.mnode.ical4j</groupId>
       <artifactId>ical4j</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.jdom</groupId>
+      <artifactId>com.springsource.org.jdom</artifactId>
+    </dependency>
     <!-- OSGi -->
     <dependency>
       <groupId>org.osgi</groupId>
@@ -117,7 +121,6 @@
     <dependency>
       <groupId>org.slf4j</groupId>
       <artifactId>slf4j-api</artifactId>
-      <scope>provided</scope>
     </dependency>
     <!-- REST test environment -->
     <dependency>
@@ -154,6 +157,12 @@
       <version>1.1.1</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>net.sf.saxon</groupId>
+      <artifactId>Saxon-HE</artifactId>
+      <version>9.6.0-4</version>
+      <scope>test</scope>
+    </dependency>
     <!--
     - Parameterized tests for JUnit
     - https://github.com/Pragmatists/JUnitParams

modules/common/src/main/java/org/opencastproject/job/api/JobParser.java

@@ -21,6 +21,8 @@
 
 package org.opencastproject.job.api;
 
+import org.opencastproject.util.XmlSafeParser;
+
 import org.apache.commons.io.IOUtils;
 
 import java.io.IOException;
@@ -32,7 +34,6 @@
 import javax.xml.bind.JAXBException;
 import javax.xml.bind.Marshaller;
 import javax.xml.bind.Unmarshaller;
-import javax.xml.transform.stream.StreamSource;
 
 /**
  * Marshals and unmarshals {@link Job}s.
@@ -76,7 +77,7 @@ public static Job parseJob(InputStream in) throws IOException {
     Unmarshaller unmarshaller;
     try {
       unmarshaller = jaxbContext.createUnmarshaller();
-      return unmarshaller.unmarshal(new StreamSource(in), JaxbJob.class).getValue().toJob();
+      return unmarshaller.unmarshal(XmlSafeParser.parse(in), JaxbJob.class).getValue().toJob();
     } catch (Exception e) {
       throw new IOException(e);
     } finally {
@@ -126,7 +127,7 @@ public static JaxbJobList parseJobList(InputStream in) throws IOException {
     Unmarshaller unmarshaller;
     try {
       unmarshaller = jaxbContext.createUnmarshaller();
-      return unmarshaller.unmarshal(new StreamSource(in), JaxbJobList.class).getValue();
+      return unmarshaller.unmarshal(XmlSafeParser.parse(in), JaxbJobList.class).getValue();
     } catch (Exception e) {
       throw new IOException(e);
     } finally {

modules/common/src/main/java/org/opencastproject/mediapackage/AbstractMediaPackageElement.java

@@ -570,7 +570,10 @@ public Object clone() {
       marshaller.marshal(this, out);
       Unmarshaller unmarshaller = MediaPackageImpl.context.createUnmarshaller();
       in = new ByteArrayInputStream(out.toByteArray());
+      // CHECKSTYLE:OFF
+      // in was already parsed and is therefore save
       return unmarshaller.unmarshal(in);
+      // CHECKSTYLE:ON
     } catch (JAXBException e) {
       throw new RuntimeException(e.getLinkedException() != null ? e.getLinkedException() : e);
     } finally {

modules/common/src/main/java/org/opencastproject/mediapackage/MediaPackageBuilderImpl.java

@@ -23,6 +23,7 @@
 package org.opencastproject.mediapackage;
 
 import org.opencastproject.mediapackage.identifier.Id;
+import org.opencastproject.util.XmlSafeParser;
 
 import org.apache.commons.io.IOUtils;
 import org.slf4j.Logger;
@@ -36,7 +37,6 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
 import javax.xml.xpath.XPathExpressionException;
@@ -105,7 +105,7 @@ public MediaPackage loadFromXml(InputStream is) throws MediaPackageException {
     if (serializer != null) {
       // FIXME This code runs if *any* serializer is present, regardless of the serializer implementation
       try {
-        Document xml = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(is);
+        Document xml = XmlSafeParser.newDocumentBuilderFactory().newDocumentBuilder().parse(is);
         rewriteUrls(xml, serializer);
         return MediaPackageImpl.valueOf(xml);
       } catch (Exception e) {

modules/common/src/main/java/org/opencastproject/mediapackage/MediaPackageElementParser.java

@@ -23,10 +23,12 @@
 
 import static org.apache.commons.io.IOUtils.toInputStream;
 
+import org.opencastproject.util.XmlSafeParser;
 import org.opencastproject.util.data.Function;
 
-import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
 
+import java.io.IOException;
 import java.io.StringWriter;
 import java.util.Collection;
 import java.util.Iterator;
@@ -95,9 +97,11 @@ public static MediaPackageElement getFromXml(String xml) throws MediaPackageExce
     Unmarshaller m = null;
     try {
       m = MediaPackageImpl.context.createUnmarshaller();
-      return (MediaPackageElement) m.unmarshal(new InputSource(toInputStream(xml)));
+      return (MediaPackageElement) m.unmarshal(XmlSafeParser.parse(toInputStream(xml)));
     } catch (JAXBException e) {
       throw new MediaPackageException(e.getLinkedException() != null ? e.getLinkedException() : e);
+    } catch (IOException | SAXException e) {
+      throw new MediaPackageException(e);
     }
   }