diff --git a/subprojects/groovy-xml/src/main/java/groovy/xml/FactorySupport.java b/subprojects/groovy-xml/src/main/java/groovy/xml/FactorySupport.java index dc936c69649..7f53b865297 100644 --- a/subprojects/groovy-xml/src/main/java/groovy/xml/FactorySupport.java +++ b/subprojects/groovy-xml/src/main/java/groovy/xml/FactorySupport.java @@ -32,6 +32,7 @@ import javax.xml.xpath.XPathFactory; import javax.xml.xpath.XPathFactoryConfigurationException; import java.security.PrivilegedExceptionAction; +import java.util.logging.Logger; /** * Support class for creating hardened JAXP factories. @@ -43,8 +44,28 @@ */ public class FactorySupport { + private static final Logger LOG = Logger.getLogger(FactorySupport.class.getName()); + private static final String DISALLOW_DOCTYPE_DECL_FEATURE = "http://apache.org/xml/features/disallow-doctype-decl"; + /** + * Logs that a security-hardening setting could not be applied to a JAXP factory. These settings + * are applied on a best-effort basis (a non-standard JAXP provider may not recognise them), but a + * silent failure would leave the factory more permissive than the caller expects, so it is logged + * rather than swallowed. + * + * @param factory the factory the setting was applied to + * @param name the feature/attribute/property name + * @param value the value that could not be applied + * @param cause the failure, or {@code null} if the value was simply not retained after being set + */ + private static void warnHardeningNotApplied(Object factory, String name, Object value, Exception cause) { + LOG.warning("Unable to apply XML security setting '" + name + "'=" + value + " on " + + factory.getClass().getName() + + (cause != null ? ": " + cause : " (value was not retained after being set)") + + "; the factory may be more permissive than intended (e.g. weaker XXE/DTD protection)."); + } + /** * Runs the supplied factory creation action and normalizes checked failures. * @@ -241,59 +262,75 @@ public static XPathFactory createXPathFactory() { return factory; } - private static void setFeatureQuietly(DocumentBuilderFactory factory, String feature, boolean value) { + // package-private for testing + static void setFeatureQuietly(DocumentBuilderFactory factory, String feature, boolean value) { try { factory.setFeature(feature, value); - } catch (ParserConfigurationException ignored) { - // feature is not supported, ignore + if (factory.getFeature(feature) != value) { + warnHardeningNotApplied(factory, feature, value, null); + } + } catch (ParserConfigurationException e) { + warnHardeningNotApplied(factory, feature, value, e); } } private static void setFeatureQuietly(SAXParserFactory factory, String feature, boolean value) { try { factory.setFeature(feature, value); - } catch (ParserConfigurationException | SAXNotSupportedException | SAXNotRecognizedException ignored) { - // feature is not supported, ignore + if (factory.getFeature(feature) != value) { + warnHardeningNotApplied(factory, feature, value, null); + } + } catch (ParserConfigurationException | SAXNotSupportedException | SAXNotRecognizedException e) { + warnHardeningNotApplied(factory, feature, value, e); } } private static void setFeatureQuietly(TransformerFactory factory, String feature, boolean value) { try { factory.setFeature(feature, value); - } catch (TransformerConfigurationException ignored) { - // feature is not supported, ignore + if (factory.getFeature(feature) != value) { + warnHardeningNotApplied(factory, feature, value, null); + } + } catch (TransformerConfigurationException e) { + warnHardeningNotApplied(factory, feature, value, e); } } private static void setFeatureQuietly(SchemaFactory factory, String feature, boolean value) { try { factory.setFeature(feature, value); - } catch (SAXNotSupportedException | SAXNotRecognizedException ignored) { - // feature is not supported, ignore + if (factory.getFeature(feature) != value) { + warnHardeningNotApplied(factory, feature, value, null); + } + } catch (SAXNotSupportedException | SAXNotRecognizedException e) { + warnHardeningNotApplied(factory, feature, value, e); } } private static void setFeatureQuietly(XPathFactory factory, String feature, boolean value) { try { factory.setFeature(feature, value); - } catch (XPathFactoryConfigurationException ignored) { - // feature is not supported, ignore + if (factory.getFeature(feature) != value) { + warnHardeningNotApplied(factory, feature, value, null); + } + } catch (XPathFactoryConfigurationException e) { + warnHardeningNotApplied(factory, feature, value, e); } } private static void setAttributeQuietly(TransformerFactory factory, String attribute, Object value) { try { factory.setAttribute(attribute, value); - } catch (IllegalArgumentException ignored) { - // attribute is not supported, ignore + } catch (IllegalArgumentException e) { + warnHardeningNotApplied(factory, attribute, value, e); } } private static void setPropertyQuietly(XMLInputFactory factory, String property, Object value) { try { factory.setProperty(property, value); - } catch (IllegalArgumentException ignored) { - // property is not supported, ignore + } catch (IllegalArgumentException e) { + warnHardeningNotApplied(factory, property, value, e); } } } diff --git a/subprojects/groovy-xml/src/test/groovy/groovy/xml/FactorySupportTest.java b/subprojects/groovy-xml/src/test/groovy/groovy/xml/FactorySupportTest.java index 2b8f6fd8952..22216f8c8e9 100644 --- a/subprojects/groovy-xml/src/test/groovy/groovy/xml/FactorySupportTest.java +++ b/subprojects/groovy-xml/src/test/groovy/groovy/xml/FactorySupportTest.java @@ -21,6 +21,7 @@ import org.junit.jupiter.api.Test; import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParserFactory; @@ -30,6 +31,14 @@ import javax.xml.xpath.XPathFactory; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -161,4 +170,81 @@ public void hardenedXPathFactoryEnablesSecureProcessing() throws Exception { XPathFactory factory = FactorySupport.createXPathFactory(); assertTrue(factory.getFeature(XMLConstants.FEATURE_SECURE_PROCESSING)); } + + // GROOVY-12120: a failure to apply a security feature must be logged, not silently swallowed + + @Test + public void warnsWhenSecurityFeatureSetThrows() { + MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory(); + factory.throwOnSetFeature = true; + List logs = captureFactorySupportLogs(() -> + FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true)); + assertTrue(logs.stream().anyMatch(r -> r.getLevel() == Level.WARNING + && r.getMessage().contains(XMLConstants.FEATURE_SECURE_PROCESSING)), + "expected a warning when the security feature could not be set"); + } + + @Test + public void warnsWhenSecurityFeatureNotRetained() { + MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory(); + factory.forcedGetFeature = Boolean.FALSE; // pretend the value never took effect + List logs = captureFactorySupportLogs(() -> + FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true)); + assertTrue(logs.stream().anyMatch(r -> r.getLevel() == Level.WARNING + && r.getMessage().contains(XMLConstants.FEATURE_SECURE_PROCESSING)), + "expected a warning when the security feature was not retained after being set"); + } + + @Test + public void doesNotWarnWhenSecurityFeatureApplied() { + MockDocumentBuilderFactory factory = new MockDocumentBuilderFactory(); + List logs = captureFactorySupportLogs(() -> + FactorySupport.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true)); + assertTrue(logs.isEmpty(), "no warning expected when the feature applies cleanly"); + } + + private static List captureFactorySupportLogs(Runnable action) { + Logger logger = Logger.getLogger(FactorySupport.class.getName()); + List records = new ArrayList<>(); + Handler handler = new Handler() { + public void publish(LogRecord record) { records.add(record); } + public void flush() { } + public void close() { } + }; + boolean useParent = logger.getUseParentHandlers(); + Level oldLevel = logger.getLevel(); + logger.addHandler(handler); + logger.setUseParentHandlers(false); + logger.setLevel(Level.ALL); + try { + action.run(); + } finally { + logger.removeHandler(handler); + logger.setUseParentHandlers(useParent); + logger.setLevel(oldLevel); + } + return records; + } + + /** A non-standard JAXP factory whose feature handling can be made to fail or no-op. */ + private static final class MockDocumentBuilderFactory extends DocumentBuilderFactory { + boolean throwOnSetFeature; + Boolean forcedGetFeature; // when non-null, getFeature returns this regardless of what was set + final Map features = new HashMap<>(); + + public DocumentBuilder newDocumentBuilder() { return null; } + public void setAttribute(String name, Object value) { } + public Object getAttribute(String name) { return null; } + + public void setFeature(String name, boolean value) throws ParserConfigurationException { + if (throwOnSetFeature) { + throw new ParserConfigurationException("unsupported feature: " + name); + } + features.put(name, value); + } + + public boolean getFeature(String name) { + return forcedGetFeature != null ? forcedGetFeature : Boolean.TRUE.equals(features.get(name)); + } + } }