HADOOP-18575. Make XML transformer factory more lenient (#5224)

Due diligence followup to
HADOOP-18469. Add secure XML parser factories to XMLUtils (#4940)

Contributed by P J Fanning
This commit is contained in:
PJ Fanning 2022-12-18 13:25:10 +01:00 committed by GitHub
parent 33785fc5ad
commit 6a07b5dc10
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 42 additions and 4 deletions

View File

@ -29,6 +29,8 @@
import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.classification.InterfaceStability;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.xml.sax.SAXException; import org.xml.sax.SAXException;
import java.io.*; import java.io.*;
@ -41,6 +43,9 @@
@InterfaceStability.Unstable @InterfaceStability.Unstable
public class XMLUtils { public class XMLUtils {
private static final Logger LOG =
LoggerFactory.getLogger(XMLUtils.class);
public static final String DISALLOW_DOCTYPE_DECL = public static final String DISALLOW_DOCTYPE_DECL =
"http://apache.org/xml/features/disallow-doctype-decl"; "http://apache.org/xml/features/disallow-doctype-decl";
public static final String LOAD_EXTERNAL_DECL = public static final String LOAD_EXTERNAL_DECL =
@ -138,8 +143,8 @@ public static TransformerFactory newSecureTransformerFactory()
throws TransformerConfigurationException { throws TransformerConfigurationException {
TransformerFactory trfactory = TransformerFactory.newInstance(); TransformerFactory trfactory = TransformerFactory.newInstance();
trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
return trfactory; return trfactory;
} }
@ -156,8 +161,29 @@ public static SAXTransformerFactory newSecureSAXTransformerFactory()
throws TransformerConfigurationException { throws TransformerConfigurationException {
SAXTransformerFactory trfactory = (SAXTransformerFactory) SAXTransformerFactory.newInstance(); SAXTransformerFactory trfactory = (SAXTransformerFactory) SAXTransformerFactory.newInstance();
trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); trfactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
trfactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); bestEffortSetAttribute(trfactory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
return trfactory; return trfactory;
} }
/**
* Set an attribute value on a {@link TransformerFactory}. If the TransformerFactory
* does not support the attribute, the method just returns <code>false</code> and
* logs the issue at debug level.
*
* @param transformerFactory to update
* @param name of the attribute to set
* @param value to set on the attribute
* @return whether the attribute was successfully set
*/
static boolean bestEffortSetAttribute(TransformerFactory transformerFactory,
String name, Object value) {
try {
transformerFactory.setAttribute(name, value);
return true;
} catch (Throwable t) {
LOG.debug("Issue setting TransformerFactory attribute {}: {}", name, t.toString());
}
return false;
}
} }

View File

@ -20,10 +20,12 @@
import java.io.InputStream; import java.io.InputStream;
import java.io.StringReader; import java.io.StringReader;
import java.io.StringWriter; import java.io.StringWriter;
import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParser;
import javax.xml.transform.Transformer; import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource; import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamResult;
import javax.xml.transform.stream.StreamSource; import javax.xml.transform.stream.StreamSource;
@ -31,6 +33,7 @@
import org.apache.hadoop.test.AbstractHadoopTestBase; import org.apache.hadoop.test.AbstractHadoopTestBase;
import org.assertj.core.api.Assertions; import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test; import org.junit.Test;
import org.w3c.dom.Document; import org.w3c.dom.Document;
import org.xml.sax.InputSource; import org.xml.sax.InputSource;
@ -128,6 +131,15 @@ public void testExternalDtdWithSecureSAXTransformerFactory() throws Exception {
} }
} }
@Test
public void testBestEffortSetAttribute() throws Exception {
TransformerFactory factory = TransformerFactory.newInstance();
Assert.assertFalse("unexpected attribute results in return of false",
XMLUtils.bestEffortSetAttribute(factory, "unsupportedAttribute false", "abc"));
Assert.assertTrue("expected attribute results in return of false",
XMLUtils.bestEffortSetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, ""));
}
private static InputStream getResourceStream(final String filename) { private static InputStream getResourceStream(final String filename) {
return TestXMLUtils.class.getResourceAsStream(filename); return TestXMLUtils.class.getResourceAsStream(filename);
} }