Java False positive on XXE detection

Rules and Languages Report False-positive / False-negative...
1

Hello!

As I was refactoring code to circumvent the problems in java.lang.IllegalArgumentException: Error: missing bug code for keySECXXEVAL · Issue #983 · spotbugs/sonar-findbugs · GitHub, I came across this false positive.

I’ve found that some java rules, like java:2755, findsecbugs:XXE_SCHEMA_FACTORY or findsecbugs:XXE_DOCUMENT (among others, there’s a bunch in this category) may falsely trigger when the safe configuration of a relevant object like SchemaFactory is done in a separate method.

Consider the following code that I extracted from my project:

import org.w3c.dom.Document;
import org.xml.sax.SAXException;
import org.xml.sax.SAXNotRecognizedException;
import org.xml.sax.SAXNotSupportedException;

import javax.xml.XMLConstants;
import javax.xml.catalog.CatalogFeatures;
import javax.xml.transform.dom.DOMSource;
import javax.xml.validation.Schema;
import javax.xml.validation.SchemaFactory;
import java.io.IOException;

public enum XmlSchemaValidationSQ {

    ; // Trick to make a purely static class without explicitly hiding the constructor

    public static final String CATALOG_FILE = XmlSchemaValidationSQ.class.getClassLoader()
                                                                         .getResource("catalog.xml").toString();

    private static final String SCHEMA_FILE = "custom-schema.xsd";

    public static void validateAgainstCustomSchema(Document xmlDocument) {
        try {
            var schemaFactory = getSafeSchemaFactory();

            var schemaFile = XmlSchemaValidationSQ.class.getClassLoader()
                                                        .getResource(XmlSchemaValidationSQ.SCHEMA_FILE);
            var schema = schemaFactory.newSchema(schemaFile);

            doValidation(xmlDocument, schema);
        } catch (IOException | SAXException e) {
            throw new RuntimeException(e);
        }
    }

    private static SchemaFactory getSafeSchemaFactory() throws SAXNotSupportedException, SAXNotRecognizedException {
        var schemaFactory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);
        // to be compliant, completely disable DOCTYPE declaration:
        schemaFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
        // or prohibit the use of all protocols by external entities:
        schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
        schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
        schemaFactory.setProperty(CatalogFeatures.Feature.FILES.getPropertyName(), CATALOG_FILE);
        return schemaFactory;
    }

    private static void doValidation(Document xmlDocument, Schema schema) throws SAXException, IOException {
        var validator = schema.newValidator();
        validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
        validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
        validator.setProperty(CatalogFeatures.Feature.FILES.getPropertyName(), CATALOG_FILE);
        validator.validate(new DOMSource(xmlDocument));
    }

}

In SonarQube 10.5.1, the line var schema = schemaFactory.newSchema(schemaFile); triggers rule findbugs:XXE_SCHEMA_FACTORY. However, the only path possible to this line leads past getSafeSchemaFactory() which sets the proper properties as listed by the rule.

Somewhat surprisingly, SonarLint (10.6.2.78685) does not find this problem, but perhaps it does not apply the findsecbugs rules even though it’s bound to our SonarQube instance?

In any case, this is a false positive. The schemaFactory used in the respective line has been configured with the right setProperty calls and that can be proven.

Edit: As a clarification; inlining getSafeSchemaFactory() leads to the code not triggering the findbugs:XXE_SCHEMA_FACTORY rule. This strengthens my feeling that this is in fact a false positive.

2

Hey there.

To report issues raised on Findbugs rules, you’ll need to raise an issue with Findbugs. GitHub - findbugsproject/findbugs: The new home of the FindBugs project

If you have a specific false-positive to look at with rules like java:S2755 (rules that Sonar develops), please share!

3

I’m going to have to disagree here. As a user, I use sonar, not findbugs. Sonar just so happens to use findbugs internally. If a bug (I consider false positives a bug) in findbugs is making Sonar ‘misbehave’ (for the lack of a better term) it is the responsibility of Sonar or its maintainers to raise a bug with findbugs, not for me as a Sonar user.

4

Sonar doesn’t use findbugs internally. If you’ve installed the GitHub - spotbugs/sonar-findbugs: SpotBugs plugin for SonarQube plugin on your SonarQube instance, then Findbugs will run in your pipeline, but only then.

5

Thanks for your reply. Our SQ instance is provided for our team and after inquiring it turns out that indeed we’re using said plugin. I’ll make sure it gets reported to findbugs.

Edit: I’ve created an issue with SpotBugs here False positive with XXE_SCHEMA_FACTORY (and possibly similar rules) when setting properties in separate method · Issue #3008 · spotbugs/spotbugs · GitHub
Edit 2: Turns out the rule itself is in FindSecBugs, so I created the issue there as well False Positive: XXE_SCHEMA_FACTORY detector does not account for setting properties in separate method · Issue #738 · find-sec-bugs/find-sec-bugs · GitHub