Skip to content

Commit cfa0d46

Browse files
aschackmullaschackmull
authored
Merge pull request github#6097 from atorralba/atorralba/promote-xslt-injection
Java: Promote XSLT Injection from experimental
2 parents e027d51 + 78c12dc commit cfa0d46

31 files changed

+409
-455
lines changed

‎java/change-notes/2021-06-16-xslt-injection-query.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "XSLT transformation with user-controlled stylesheet" (`java/xslt-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @ggolawski](https://github.com/github/codeql/pull/3363).

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ private module Frameworks {
110110
private import semmle.code.java.security.MvelInjection
111111
private import semmle.code.java.security.OgnlInjection
112112
private import semmle.code.java.security.XPath
113+
private import semmle.code.java.security.XsltInjection
113114
private import semmle.code.java.frameworks.android.Android
114115
private import semmle.code.java.frameworks.android.SQLite
115116
private import semmle.code.java.frameworks.Jdbc

‎java/ql/lib/semmle/code/java/security/XsltInjection.qll

Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
/** Provides classes to reason about XSLT injection vulnerabilities. */
2+
3+
import java
4+
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.dataflow.ExternalFlow
6+
7+
/**
8+
* A data flow sink for unvalidated user input that is used in XSLT transformation.
9+
* Extend this class to add your own XSLT Injection sinks.
10+
*/
11+
abstract class XsltInjectionSink extends DataFlow::Node { }
12+
13+
/** A default sink representing methods susceptible to XSLT Injection attacks. */
14+
private class DefaultXsltInjectionSink extends XsltInjectionSink {
15+
DefaultXsltInjectionSink() { sinkNode(this, "xslt") }
16+
}
17+
18+
private class DefaultXsltInjectionSinkModel extends SinkModelCsv {
19+
override predicate row(string row) {
20+
row =
21+
[
22+
"javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt",
23+
"net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt",
24+
"net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt",
25+
"net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt",
26+
"net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt",
27+
"net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt"
28+
]
29+
}
30+
}
31+
32+
/**
33+
* A unit class for adding additional taint steps.
34+
*
35+
* Extend this class to add additional taint steps that should apply to the `XsltInjectionFlowConfig`.
36+
*/
37+
class XsltInjectionAdditionalTaintStep extends Unit {
38+
/**
39+
* Holds if the step from `node1` to `node2` should be considered a taint
40+
* step for the `XsltInjectionFlowConfig` configuration.
41+
*/
42+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
43+
}
44+
45+
/** A set of additional taint steps to consider when taint tracking XSLT related data flows. */
46+
private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep {
47+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
48+
xmlStreamReaderStep(node1, node2) or
49+
xmlEventReaderStep(node1, node2) or
50+
staxSourceStep(node1, node2) or
51+
documentBuilderStep(node1, node2) or
52+
domSourceStep(node1, node2) or
53+
newTransformerFromTemplatesStep(node1, node2) or
54+
xsltCompilerStep(node1, node2) or
55+
xsltExecutableStep(node1, node2) or
56+
xsltPackageStep(node1, node2)
57+
}
58+
}
59+
60+
/**
61+
* Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and
62+
* `XMLStreamReader`, i.e. `XMLInputFactory.createXMLStreamReader(tainted)`.
63+
*/
64+
private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) {
65+
exists(XmlInputFactoryStreamReader xmlStreamReader |
66+
if xmlStreamReader.getMethod().getParameterType(0) instanceof TypeString
67+
then n1.asExpr() = xmlStreamReader.getArgument(1)
68+
else n1.asExpr() = xmlStreamReader.getArgument(0)
69+
|
70+
n2.asExpr() = xmlStreamReader
71+
)
72+
}
73+
74+
/**
75+
* Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and
76+
* `XMLEventReader`, i.e. `XMLInputFactory.createXMLEventReader(tainted)`.
77+
*/
78+
private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) {
79+
exists(XmlInputFactoryEventReader xmlEventReader |
80+
if xmlEventReader.getMethod().getParameterType(0) instanceof TypeString
81+
then n1.asExpr() = xmlEventReader.getArgument(1)
82+
else n1.asExpr() = xmlEventReader.getArgument(0)
83+
|
84+
n2.asExpr() = xmlEventReader
85+
)
86+
}
87+
88+
/**
89+
* Holds if `n1` to `n2` is a dataflow step that converts between `XMLStreamReader` or
90+
* `XMLEventReader` and `StAXSource`, i.e. `new StAXSource(tainted)`.
91+
*/
92+
private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) {
93+
exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeStAXSource |
94+
n1.asExpr() = cc.getAnArgument() and
95+
n2.asExpr() = cc
96+
)
97+
}
98+
99+
/**
100+
* Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` and `Document`,
101+
* i.e. `DocumentBuilder.parse(tainted)`.
102+
*/
103+
private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) {
104+
exists(DocumentBuilderParse documentBuilder |
105+
n1.asExpr() = documentBuilder.getArgument(0) and
106+
n2.asExpr() = documentBuilder
107+
)
108+
}
109+
110+
/**
111+
* Holds if `n1` to `n2` is a dataflow step that converts between `Document` and `DOMSource`, i.e.
112+
* `new DOMSource(tainted)`.
113+
*/
114+
private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) {
115+
exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeDOMSource |
116+
n1.asExpr() = cc.getAnArgument() and
117+
n2.asExpr() = cc
118+
)
119+
}
120+
121+
/**
122+
* Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`,
123+
* i.e. `tainted.newTransformer()`.
124+
*/
125+
private predicate newTransformerFromTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) {
126+
exists(MethodAccess ma, Method m | ma.getMethod() = m |
127+
n1.asExpr() = ma.getQualifier() and
128+
n2.asExpr() = ma and
129+
m.getDeclaringType() instanceof TypeTemplates and
130+
m.hasName("newTransformer")
131+
)
132+
}
133+
134+
/**
135+
* Holds if `n1` to `n2` is a dataflow step that converts between `Source` or `URI` and
136+
* `XsltExecutable` or `XsltPackage`, i.e. `XsltCompiler.compile(tainted)` or
137+
* `XsltCompiler.loadExecutablePackage(tainted)` or `XsltCompiler.compilePackage(tainted)` or
138+
* `XsltCompiler.loadLibraryPackage(tainted)`.
139+
*/
140+
private predicate xsltCompilerStep(DataFlow::Node n1, DataFlow::Node n2) {
141+
exists(MethodAccess ma, Method m | ma.getMethod() = m |
142+
n1.asExpr() = ma.getArgument(0) and
143+
n2.asExpr() = ma and
144+
m.getDeclaringType() instanceof TypeXsltCompiler and
145+
m.hasName(["compile", "loadExecutablePackage", "compilePackage", "loadLibraryPackage"])
146+
)
147+
}
148+
149+
/**
150+
* Holds if `n1` to `n2` is a dataflow step that converts between `XsltExecutable` and
151+
* `XsltTransformer` or `Xslt30Transformer`, i.e. `XsltExecutable.load()` or
152+
* `XsltExecutable.load30()`.
153+
*/
154+
private predicate xsltExecutableStep(DataFlow::Node n1, DataFlow::Node n2) {
155+
exists(MethodAccess ma, Method m | ma.getMethod() = m |
156+
n1.asExpr() = ma.getQualifier() and
157+
n2.asExpr() = ma and
158+
m.getDeclaringType() instanceof TypeXsltExecutable and
159+
m.hasName(["load", "load30"])
160+
)
161+
}
162+
163+
/**
164+
* Holds if `n1` to `n2` is a dataflow step that converts between `XsltPackage` and
165+
* `XsltExecutable`, i.e. `XsltPackage.link()`.
166+
*/
167+
private predicate xsltPackageStep(DataFlow::Node n1, DataFlow::Node n2) {
168+
exists(MethodAccess ma, Method m | ma.getMethod() = m |
169+
n1.asExpr() = ma.getQualifier() and
170+
n2.asExpr() = ma and
171+
m.getDeclaringType() instanceof TypeXsltPackage and
172+
m.hasName("link")
173+
)
174+
}
175+
176+
/** The class `javax.xml.transform.stax.StAXSource`. */
177+
private class TypeStAXSource extends Class {
178+
TypeStAXSource() { this.hasQualifiedName("javax.xml.transform.stax", "StAXSource") }
179+
}
180+
181+
/** The class `javax.xml.transform.dom.DOMSource`. */
182+
private class TypeDOMSource extends Class {
183+
TypeDOMSource() { this.hasQualifiedName("javax.xml.transform.dom", "DOMSource") }
184+
}
185+
186+
/** The interface `javax.xml.transform.Templates`. */
187+
private class TypeTemplates extends Interface {
188+
TypeTemplates() { this.hasQualifiedName("javax.xml.transform", "Templates") }
189+
}
190+
191+
/** The class `net.sf.saxon.s9api.XsltCompiler`. */
192+
private class TypeXsltCompiler extends Class {
193+
TypeXsltCompiler() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltCompiler") }
194+
}
195+
196+
/** The class `net.sf.saxon.s9api.XsltExecutable`. */
197+
private class TypeXsltExecutable extends Class {
198+
TypeXsltExecutable() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltExecutable") }
199+
}
200+
201+
/** The class `net.sf.saxon.s9api.XsltPackage`. */
202+
private class TypeXsltPackage extends Class {
203+
TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") }
204+
}
205+
206+
// XmlParsers classes
207+
/** A call to `DocumentBuilder.parse`. */
208+
private class DocumentBuilderParse extends MethodAccess {
209+
DocumentBuilderParse() {
210+
exists(Method m |
211+
this.getMethod() = m and
212+
m.getDeclaringType() instanceof DocumentBuilder and
213+
m.hasName("parse")
214+
)
215+
}
216+
}
217+
218+
/** The class `javax.xml.parsers.DocumentBuilder`. */
219+
private class DocumentBuilder extends RefType {
220+
DocumentBuilder() { this.hasQualifiedName("javax.xml.parsers", "DocumentBuilder") }
221+
}
222+
223+
/** A call to `XMLInputFactory.createXMLStreamReader`. */
224+
private class XmlInputFactoryStreamReader extends MethodAccess {
225+
XmlInputFactoryStreamReader() {
226+
exists(Method m |
227+
this.getMethod() = m and
228+
m.getDeclaringType() instanceof XmlInputFactory and
229+
m.hasName("createXMLStreamReader")
230+
)
231+
}
232+
}
233+
234+
/** A call to `XMLInputFactory.createEventReader`. */
235+
private class XmlInputFactoryEventReader extends MethodAccess {
236+
XmlInputFactoryEventReader() {
237+
exists(Method m |
238+
this.getMethod() = m and
239+
m.getDeclaringType() instanceof XmlInputFactory and
240+
m.hasName("createXMLEventReader")
241+
)
242+
}
243+
}
244+
245+
/** The class `javax.xml.stream.XMLInputFactory`. */
246+
private class XmlInputFactory extends RefType {
247+
XmlInputFactory() { this.hasQualifiedName("javax.xml.stream", "XMLInputFactory") }
248+
}

‎java/ql/lib/semmle/code/java/security/XsltInjectionQuery.qll

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/** Provides taint tracking configurations to be used in XSLT injection queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.security.XmlParsers
7+
import semmle.code.java.security.XsltInjection
8+
9+
/**
10+
* A taint-tracking configuration for unvalidated user input that is used in XSLT transformation.
11+
*/
12+
class XsltInjectionFlowConfig extends TaintTracking::Configuration {
13+
XsltInjectionFlowConfig() { this = "XsltInjectionFlowConfig" }
14+
15+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
16+
17+
override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink }
18+
19+
override predicate isSanitizer(DataFlow::Node node) {
20+
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
21+
}
22+
23+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
24+
any(XsltInjectionAdditionalTaintStep c).step(node1, node2)
25+
}
26+
}
27+
28+
/**
29+
* A set of additional taint steps to consider when taint tracking XSLT related data flows.
30+
* These steps use data flow logic themselves.
31+
*/
32+
private class DataFlowXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep {
33+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
34+
newTransformerOrTemplatesStep(node1, node2)
35+
}
36+
}
37+
38+
/**
39+
* Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or
40+
* `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or
41+
* `TransformerFactory.newTemplates(tainted)`.
42+
*/
43+
private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) {
44+
exists(MethodAccess ma, Method m | ma.getMethod() = m |
45+
n1.asExpr() = ma.getAnArgument() and
46+
n2.asExpr() = ma and
47+
m.getDeclaringType() instanceof TransformerFactory and
48+
m.hasName(["newTransformer", "newTemplates"]) and
49+
not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf |
50+
conf.hasFlowToExpr(ma.getQualifier())
51+
)
52+
)
53+
}
54+
55+
/**
56+
* A data flow configuration for secure processing feature that is enabled on `TransformerFactory`.
57+
*/
58+
private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration {
59+
TransformerFactoryWithSecureProcessingFeatureFlowConfig() {
60+
this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig"
61+
}
62+
63+
override predicate isSource(DataFlow::Node src) {
64+
exists(Variable v | v = src.asExpr().(VarAccess).getVariable() |
65+
exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() |
66+
config.enables(configSecureProcessing())
67+
)
68+
)
69+
}
70+
71+
override predicate isSink(DataFlow::Node sink) {
72+
exists(MethodAccess ma |
73+
sink.asExpr() = ma.getQualifier() and
74+
ma.getMethod().getDeclaringType() instanceof TransformerFactory
75+
)
76+
}
77+
78+
override int fieldFlowBranchLimit() { result = 0 }
79+
}
80+
81+
/** A `ParserConfig` specific to `TransformerFactory`. */
82+
private class TransformerFactoryFeatureConfig extends ParserConfig {
83+
TransformerFactoryFeatureConfig() {
84+
exists(Method m |
85+
m = this.getMethod() and
86+
m.getDeclaringType() instanceof TransformerFactory and
87+
m.hasName("setFeature")
88+
)
89+
}
90+
}

‎java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.java renamed to ‎java/ql/src/Security/CWE/CWE-074/XsltInjection.java

File renamed without changes.

‎java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.qhelp renamed to ‎java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,20 @@
44
<qhelp>
55
<overview>
66
<p>XSLT (Extensible Stylesheet Language Transformations) is a language for transforming XML
7-
documents into other XML documents or other formats. Processing of unvalidated XSLT stylesheet can
8-
let attacker to read arbitrary files from the filesystem or to execute arbitrary code.</p>
7+
documents into other XML documents or other formats. Processing unvalidated XSLT stylesheets can
8+
allow attackers to read arbitrary files from the filesystem or to execute arbitrary code.</p>
99
</overview>
1010

1111
<recommendation>
12-
<p>The general recommendation is to not process untrusted XSLT stylesheets. If user provided
12+
<p>The general recommendation is to not process untrusted XSLT stylesheets. If user-provided
1313
stylesheets must be processed, enable the secure processing mode.</p>
1414
</recommendation>
1515

1616
<example>
1717
<p>In the following examples, the code accepts an XSLT stylesheet from the user and processes it.
1818
</p>
1919

20-
<p>In the first example, the user provided XSLT stylesheet is parsed and processed.</p>
20+
<p>In the first example, the user-provided XSLT stylesheet is parsed and processed.</p>
2121

2222
<p>In the second example, secure processing mode is enabled.</p>
2323

‎java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.ql renamed to ‎java/ql/src/Security/CWE/CWE-074/XsltInjection.ql

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @name XSLT transformation with user-controlled stylesheet
3-
* @description Doing an XSLT transformation with user-controlled stylesheet can lead to
3+
* @description Performing an XSLT transformation with user-controlled stylesheets can lead to
44
* information disclosure or execution of arbitrary code.
55
* @kind path-problem
66
* @problem.severity error
@@ -11,8 +11,7 @@
1111
*/
1212

1313
import java
14-
import semmle.code.java.dataflow.FlowSources
15-
import XsltInjectionLib
14+
import semmle.code.java.security.XsltInjectionQuery
1615
import DataFlow::PathGraph
1716

1817
from DataFlow::PathNode source, DataFlow::PathNode sink, XsltInjectionFlowConfig conf

0 commit comments

Comments
 (0)