XML parsers should not be vulnerable to XXE attacks (java:S2755)

[bpapez (Benjamin Papež)]

We get java:S2755 “Disable access to external entities in XML parsing” reported on TransformerFactory.newInstance() although we pass it to a setDefaultAttributes method, which sets the recommended properties on it to prevent XXE attacks.
Our code looks like this:

    public static TransformerFactory newInstance() {
        return setDefaultAttributes(TransformerFactory.newInstance());
    }

    private static TransformerFactory setDefaultAttributes(TransformerFactory factory) {
        setOptionalAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, "");
        setOptionalAttribute(factory, XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
        setOptionalFeature(factory, FeatureKeys.ALLOW_EXTERNAL_FUNCTIONS, false);
        return factory;
    }
    private static void setOptionalAttribute(TransformerFactory factory, String name, Object value) {
        try {
            factory.setAttribute(name, value);
        } catch (IllegalArgumentException e) {
            if (logger.isDebugEnabled()) {
                logger.debug("{} property not supported by {}", name, factory.getClass().getCanonicalName());
            }
        }
    }
    private static void setOptionalFeature(TransformerFactory factory, String name, boolean value) {
        try {
            factory.setFeature(name, value);
        } catch (IllegalArgumentException | TransformerConfigurationException e) {
            if (logger.isDebugEnabled()) {
                logger.debug("{} feature not supported by {}", name, factory.getClass().getCanonicalName());
            }
        }
    }

Is the SonarQube Java scanner able to not report this false positive?

We use SonarQube Developer Edition Version 8.6 and Java Code Quality and Security plugin: 6.9.0.23563

[Quentin (Quentin Jaquier)]

Hello @bpapez and welcome to the SonarSource community!

What bothers me in your example is that if the property is not supported, you simply log a debug message, but still return the vulnerable factory. It does not seem wise to “ignore” such exceptions without using other features to make the Factory safe against XXE.

Just to be fair: if you simply remove the try/catch, the issue will still appear. This is a limitation of our engine, due to the helper used to set the property. In my opinion, this is not too bad since if you don’t have the try/catch, there is no good reason to use a helper and you can directly use factory.setAttribute.

Does it clarify the situation?

[bpapez (Benjamin Papež)]

Hello Quentin,

thanks for your valuable feedback. So at the end the false positive and your reply made me and investigating. The reason for the optional setting is that we had multiple TransformerFactory implementations deployed, but none supported all these attribute or feature settings. I agree that the debug log message is not wise, so I headed to at least change it to a warning log. However then I found that the TransformerFactory implementation we mainly use just recently released a support for all these settings ( Bug #4729: TransformerFactory doesn't accept ACCESS_EXTERNAL_STYLESHEET property - Saxon - Saxonica Developer Community ).

Now I could finally change the code and Sonar does not complain anymore.

Regards,
Benjamin