Skip to content

Commit

Permalink
Improve XML XXE protection
Browse files Browse the repository at this point in the history
  • Loading branch information
mangstadt committed Nov 4, 2023
1 parent 2e80b18 commit 80a54d0
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
5 changes: 4 additions & 1 deletion src/main/java/biweekly/io/xml/XCalDocument.java
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,10 @@ public void write(Writer writer, Integer indent, String xmlVersion) throws Trans
public void write(Writer writer, Map<String, String> outputProperties) throws TransformerException {
Transformer transformer;
try {
transformer = TransformerFactory.newInstance().newTransformer();
TransformerFactory factory = TransformerFactory.newInstance();
XmlUtils.applyXXEProtection(factory);

transformer = factory.newTransformer();
} catch (TransformerConfigurationException e) {
//should never be thrown because we're not doing anything fancy with the configuration
throw new RuntimeException(e);
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/biweekly/util/XmlUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,18 @@ public static void applyXXEProtection(DocumentBuilderFactory factory) {
* @see <a href=
* "https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Prevention_Cheat_Sheet#Java">
* XXE Cheat Sheet</a>
* @see <a href="https://rules.sonarsource.com/java/RSPEC-2755/">SonarLint
* 2755</a>
*/
public static void applyXXEProtection(TransformerFactory factory) {
//@formatter:off
String[] attributes = {
//XMLConstants.ACCESS_EXTERNAL_DTD (Java 7 only)
"http://javax.xml.XMLConstants/property/accessExternalDTD",

//XMLConstants.ACCESS_EXTERNAL_SCHEMA (Java 7 only)
"http://javax.xml.XMLConstants/property/accessExternalSchema",

//XMLConstants.ACCESS_EXTERNAL_STYLESHEET (Java 7 only)
"http://javax.xml.XMLConstants/property/accessExternalStylesheet"
};
Expand Down Expand Up @@ -283,7 +288,10 @@ public static void toWriter(Node node, Writer writer) throws TransformerExceptio
*/
public static void toWriter(Node node, Writer writer, Map<String, String> outputProperties) throws TransformerException {
try {
Transformer transformer = TransformerFactory.newInstance().newTransformer();
TransformerFactory factory = TransformerFactory.newInstance();
applyXXEProtection(factory);

Transformer transformer = factory.newTransformer();
for (Map.Entry<String, String> property : outputProperties.entrySet()) {
try {
transformer.setOutputProperty(property.getKey(), property.getValue());
Expand Down

5 comments on commit 80a54d0

@M66B
Copy link

@M66B M66B commented on 80a54d0 Dec 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not pass the Fortify scanner, unfortunately. Please see here for the correct recipes:

https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java

@mangstadt
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why? I looked at the code samples on the website you referenced, and biweekly is doing all of those things.

@M66B
Copy link

@M66B M66B commented on 80a54d0 Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm building biweekly as part of another project (FairEmail) to have this fix and the complete project was scanned by the Fortify scanner, which says it is not good enough. Don't ask me why because it is often rather difficult to deduce/guess why.

@mangstadt
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool must not realize that everything is done inside of the XmlUtils.applyXXEProtection method. It probably expects the XXE protection code to be within the same scope as the code that creates the factory object.

@M66B
Copy link

@M66B M66B commented on 80a54d0 Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool does a static code analysis, so I think that is a good theory.

Please sign in to comment.