CodeQL false positive: java/xxe

[ericjkaplan]

I understand the main exploit here involves parsing untrusted XML files. The recommendation seems to be to disable DDL. The specific example I am following is:

The following example calls parse on a DocumentBuilder that is not safely configured on untrusted data, and is therefore inherently unsafe.

public void parse(Socket sock) throws Exception {
  DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
  DocumentBuilder builder = factory.newDocumentBuilder();
  builder.parse(sock.getInputStream()); //unsafe
}

In this example, the DocumentBuilder is created with DTD disabled, securing it against XXE attack.

public void disableDTDParse(Socket sock) throws Exception {
  DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
  factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
  DocumentBuilder builder = factory.newDocumentBuilder();
  builder.parse(sock.getInputStream()); //safe
}

In my case, the code was slightly different as I am using an event driven parser:

XMLInputFactory factory = XMLInputFactory.newInstance();
XMLStreamReader reader = factory.createXMLStreamReader( fis );

So my fix was as follows:

XMLInputFactory factory = XMLInputFactory.newInstance();
factory.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE);
factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");

XMLStreamReader reader = factory.createXMLStreamReader( fis );

However CodeQL still complains, so I'm wondering if my remediation is wrong or CodeQL is falsely claiming this is still an issue.

Any help would be appreciated, thanks!

[atorralba]

Hey @ericjkaplan, thanks for your report.

I reviewed what the query is currently requiring to consider an XMLInputFactory to be safe:

/**
 * A safely configured `XmlInputFactory`.
 */
class SafeXmlInputFactory extends VarAccess {
  SafeXmlInputFactory() {
    exists(Variable v |
      v = this.getVariable() and
      exists(XmlInputFactoryConfig config | config.getQualifier() = v.getAnAccess() |
        config.disables(configOptionIsSupportingExternalEntities())
      ) and
      exists(XmlInputFactoryConfig config | config.getQualifier() = v.getAnAccess() |
        config.disables(configOptionSupportDtd())
      )
    )
  }
}

Which means it wants something like:

public static void disableDTDParse(InputStream is) throws Exception {
  XMLInputFactory factory = XMLInputFactory.newInstance();
  factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
  factory.setProperty("javax.xml.stream.isSupportingExternalEntities", false);
  // ...
}

If you add that, the alert should go away.

Nonetheless, I quickly tested this code and could confirm that factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); is enough to prevent the vulnerability — and it's consistent with what OWASP says:

To protect a Java XMLInputFactory from XXE, disable DTDs (doctypes) altogether:

// This disables DTDs entirely for that factory
xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);

So this is something that we need to fix in the query. Thanks for bringing it to our attention!

[ericjkaplan]

Cool beans and thanks for the very clear reply! Indeed following your guidance the alert has gone away, glad to help!