Ekryd / sortpom

Maven plugin that helps the user sort pom.xml.
https://github.com/Ekryd/sortpom/wiki/
BSD 3-Clause "New" or "Revised" License
336 stars 181 forks source link

no sort xmls with doctype #281

Closed delanym closed 1 year ago

delanym commented 1 year ago

Im trying to use sortpom to sort checkstyle.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">

If I remove doctype checkstyle complains

Failed during checkstyle execution: Failed during checkstyle configuration: unable to parse configuration stream - Document root element "module", must match DOCTYPE root "null".:2:24 ->

Do you think this is something checkstyle should change?

Ekryd commented 1 year ago

Great initiative to sort other non-pom files. I have almost forgotten that we could do that. 😀

I got a sonar warning about this: apparently allowing doctype in an xml is a potential security vulnerability. That is why SortPom does not allow it. The sonar warnings history is no longer there, but I can dig a bit around the issue.

Ekryd commented 1 year ago

Here is a link to the OWASP site: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html

This is fun way to break your tool when allowing DOCTYPE: https://en.wikipedia.org/wiki/Billion_laughs_attack

So, the answer is Yes. Checkstyle tooling should not allow DOCTYPE references and Checkstyle itself should not endorse the usage of DOCTYPE.

delanym commented 1 year ago

Ok, well I'll mention it to them but no guarantee's they're going to change it. I see that

You can parse the document without enabling processing of the external DTD subset. In SAX, you can instruct a parser not to read the external DTD subset by setting the http://xml.org/sax/features/external-general-entities and http://xml.org/sax/features/external-parameter-entities features to false

https://web.archive.org/web/20101005080451/http://www.ibm.com/developerworks/xml/library/x-tipcfsx.html

Ekryd commented 1 year ago

Hi! I tried with

    parser.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
    parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
    parser.setFeature("http://xml.org/sax/features/external-general-entities", false);
    parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);

and a Billion Laughs xml file. The error message was

Caused by: org.xml.sax.SAXParseException; lineNumber: 1; columnNumber: 1; JAXP00010001: The parser has encountered more than "64000" entity expansions in this document; this is the limit imposed by the JDK.
    at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(ErrorHandlerWrapper.java:204)
    at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.fatalError(ErrorHandlerWrapper.java:178)

So it seems like the doctype is parsed anyway. I will keep disallow-doctype-decl for now