whodes / WebGoat

Other
0 stars 0 forks source link

Introduced protections against XXE attacks in XMLInputFactory #6

Closed pixee-whodes[bot] closed 2 months ago

pixee-whodes[bot] commented 3 months ago

This change updates all instances of XMLInputFactory to prevent them from resolving external entities, which can protect you from arbitrary code execution, sensitive data exfiltration, and probably a bunch more evil things attackers are still discovering.

Without this protection, attackers can cause your XMLInputFactory parser to retrieve sensitive information with attacks like this:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE foo [ <!ENTITY xxe SYSTEM "file:///etc/passwd"> ]>
<book>
    <title>&xxe;</title>
</book>

Yes, it's pretty insane that this is the default behavior. Our change hardens the factories created with the necessary security features to prevent your parser from resolving external entities.

+ import io.github.pixee.security.XMLInputFactorySecurity;
  ...
- XMLInputFactory xmlInputFactory = XMLInputFactory.newFactory();
+ XMLInputFactory xmlInputFactory = XMLInputFactorySecurity.hardenFactory(XMLInputFactory.newFactory());

You could take our protections one step further by changing our supplied code to prevent the user from supplying a DOCTYPE, which is more aggressive and more secure, but also more likely to affect existing code behavior:

+ import io.github.pixee.security.XMLInputFactorySecurity;
+ import io.github.pixee.security.XMLRestrictions;
  ...
  XMLInputFactory xmlInputFactory = XMLInputFactorySecurity.hardenFactory(XMLInputFactory.newFactory(), XMLRestrictions.DISALLOW_DOCTYPE);
More reading * [https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html) * [https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing](https://owasp.org/www-community/vulnerabilities/XML_External_Entity_(XXE)_Processing) * [https://github.com/swisskyrepo/PayloadsAllTheThings/blob/master/XXE%20Injection/README.md](https://github.com/swisskyrepo/PayloadsAllTheThings/blob/master/XXE%20Injection/README.md)

I have additional improvements ready for this repo! If you want to see them, leave the comment:

@pixeebot next

... and I will open a new PR right away!

🧚🤖 Powered by Pixeebot

Feedback | Community | Docs | Codemod ID: pixee:java/harden-xmlinputfactory

pixee-whodes[bot] commented 2 months ago

I'm confident in this change, but I'm not a maintainer of this project. Do you see any reason not to merge it?

If this change was not helpful, or you have suggestions for improvements, please let me know!

pixee-whodes[bot] commented 2 months ago

Unless you're in the microscopic percentage of developers intentionally allowing external entities in XML on purpose, this change should be merged. In that case however, it should be noted that whoever is supplying XML to your code must be completely trusted.

It should also be noted that the parser you think you're using may actually be different than the one you intend, since the Java XML system has conditions that allows libraries to insert themselves as the primary XML provider.

If there are other concerns about this change, I'd love to hear about them!

pixee-whodes[bot] commented 2 months ago

This change may not be a priority right now, so I'll close it. If there was something I could have done better, please let me know!

You can also customize me to make sure I'm working with you in the way you want.