Skip to content

Java: Add XXE sinks #6564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Java: Add XXE sinks #6564

merged 2 commits into from
Sep 10, 2021

Conversation

haby0
Copy link
Contributor

@haby0 haby0 commented Aug 27, 2021

There are many XML parsers for Java, and most of them are vulnerable to XXE because their default settings enable parsing of
external entities. This query currently identifies vulnerable XML parsing from the following parsers: javax.xml.validation.Validator,
org.dom4j.DocumentHelper, org.apache.commons.digester3.Digester, java.beans.XMLDecoder, org.rundeck.api.parser.ParserHelper, org.apache.commons.digester.Digester, org.apache.tomcat.util.digester.Digester

Open source components: vertx-web

@smowton
Copy link
Contributor

smowton commented Sep 2, 2021

This PR is currently a big rebase and merge mess -- recommend flattening unnecessary commits to make reviewing this easier.

* @kind path-problem
* @problem.severity error
* @precision high
* @id java/xxe
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @id java/xxe
* @id java/xxe-with-experimental-sinks

@@ -0,0 +1,31 @@
/**
* @name Resolving XML external entity in user-controlled data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @name Resolving XML external entity in user-controlled data
* @name Resolving XML external entity in user-controlled data (experimental sinks)

Comment on lines +3 to +4
* @description Parsing user-controlled XML documents and allowing expansion of external entity
* references may lead to disclosure of confidential data or denial of service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @description Parsing user-controlled XML documents and allowing expansion of external entity
* references may lead to disclosure of confidential data or denial of service.
* @description Parsing user-controlled XML documents and allowing expansion of external entity
* references may lead to disclosure of confidential data or denial of service.
* (note this version differs from query `java/xxe` by including support for additional possibly-vulnerable XML parsers)

}

/**
* The classes `org.apache.commons.digester3.Digester` or `org.apache.commons.digester.Digester` or `org.apache.tomcat.util.digester.Digester`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The classes `org.apache.commons.digester3.Digester` or `org.apache.commons.digester.Digester` or `org.apache.tomcat.util.digester.Digester`.
* The classes `org.apache.commons.digester3.Digester`, `org.apache.commons.digester.Digester` or `org.apache.tomcat.util.digester.Digester`.

@@ -0,0 +1,29 @@
/**
* @name Resolving XML external entity in local source
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to add this version? Did you need it to catch some CVE? If not, drop the local version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i am using it to catch some cve.

@@ -1002,7 +1002,7 @@ class SAXTransformerFactoryNewXMLFilter extends XmlParserCall {
exists(Method m |
this.getMethod() = m and
m.getDeclaringType().hasQualifiedName("javax.xml.transform.sax", "SAXTransformerFactory") and
m.hasName("newXMLFilter")
m.getName() in ["newXMLFilter", "newTransformerHandler"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this change to the experimental query

@@ -144,7 +144,7 @@ private predicate constantStringExpr(Expr e, string val) {
}

/** An expression that always has the same string value. */
private class ConstantStringExpr extends Expr {
class ConstantStringExpr extends Expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy this to your qll file rather than expose it: the heuristic is bad enough that we probably don't want to publicly expose this difficult-to-explain partial solution.

@haby0 haby0 requested review from smowton and removed request for a team September 7, 2021 10:24
@@ -991,28 +991,6 @@ class SafeTransformer extends MethodAccess {
}
}

/*
* SAXTransformer: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#saxtransformerfactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this change: this file should be unchanged except exposing SafeTransformerFactoryFlowConfig which is acceptable, then the experimental query should add the extra sink.

@haby0 haby0 requested a review from smowton September 8, 2021 01:14
…a version that uses local sources.

Originally authored by @haby0, squashed to clean up a tangled commit history.
@smowton smowton merged commit 0ebbb33 into github:main Sep 10, 2021
@haby0 haby0 deleted the java/xxe/new branch September 15, 2021 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants