semgrep-rules
semgrep-rules copied to clipboard
semgrep
Remove inferior OWASP XXE DocumentBuilderFactory rules / Add SAXParserFactory
Describe the bug
The old OWASP rule for DocumentBuilderFactory XXE https://github.com/returntocorp/semgrep-rules/blob/develop/contrib/owasp/java/xxe/documentbuilderfactory.yaml is inferior to the newer rules in https://github.com/returntocorp/semgrep-rules/tree/develop/java/lang/security/audit/xxe and produces many false positives, I suggest just removing it.
In addition, please consider duplicating the new DocumentBuilderFactory rules for SAXParserFactory, as currently there is no coverage unless you use the flawed https://github.com/returntocorp/semgrep-rules/blob/develop/contrib/owasp/java/xxe/saxparserfactory.java. It's pretty much exactly the same logic as DocumentBuilderFactory as per https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxp-documentbuilderfactory-saxparserfactory-and-dom4j
Hi @coheigea , thanks for your report! It's on our to do list to clean up these XXE rules, but it's not high priority right now.
We have a set of accurate Java XXE rules in the pro rules, and we have to decide how to continue with these community contributed rules in parallel.
The OWASP rules in particular are linked to from an OWASP cheat sheet: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#semgrep-rules
Removing these outright would not be the most community-friendly move. I think a potential solution would be to recreate the whole set of linked XXE rules and replace the links in the OWASP cheat sheet with the more accurate versions. Unfortunately, there is currently not much bandwith on our side to get this work started. If you feel up to the task, we'll gladly support you with reviews!
Hi @0xDC0DE ,
Thanks for your response. Just to clarify, if I submit a PR to essentially duplicate the existing DocumentBuilderFactory rules in https://github.com/returntocorp/semgrep-rules/tree/develop/java/lang/security/audit/xxe for SAXParserFactory, will it be accepted or not?
Colm.
Yes! It will/should be! And we can submit a PR to update the OWASP cheatsheet to link to your rule as well!
OK @0xDC0DE , here is the first PR adding support for SAXParserFactory - https://github.com/returntocorp/semgrep-rules/pull/2907