Skip to content

Commit 13417db

Browse files
atorralbaatorralba
committed
Remove DataFlow references from XsltInjection.qll
1 parent ff21662 commit 13417db

File tree

2 files changed

+118
-59
lines changed

2 files changed

+118
-59
lines changed

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

Lines changed: 53 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import java
44
import semmle.code.java.dataflow.DataFlow
55
import semmle.code.java.dataflow.ExternalFlow
6-
import semmle.code.java.security.XmlParsers
76

87
/**
98
* A data flow sink for unvalidated user input that is used in XSLT transformation.
@@ -51,7 +50,6 @@ private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAddit
5150
staxSourceStep(node1, node2) or
5251
documentBuilderStep(node1, node2) or
5352
domSourceStep(node1, node2) or
54-
newTransformerOrTemplatesStep(node1, node2) or
5553
newTransformerFromTemplatesStep(node1, node2) or
5654
xsltCompilerStep(node1, node2) or
5755
xsltExecutableStep(node1, node2) or
@@ -65,7 +63,10 @@ private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAddit
6563
*/
6664
private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) {
6765
exists(XmlInputFactoryStreamReader xmlStreamReader |
68-
n1.asExpr() = xmlStreamReader.getSink() and
66+
if xmlStreamReader.getMethod().getParameterType(0) instanceof TypeString
67+
then n1.asExpr() = xmlStreamReader.getArgument(1)
68+
else n1.asExpr() = xmlStreamReader.getArgument(0)
69+
|
6970
n2.asExpr() = xmlStreamReader
7071
)
7172
}
@@ -76,7 +77,10 @@ private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) {
7677
*/
7778
private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) {
7879
exists(XmlInputFactoryEventReader xmlEventReader |
79-
n1.asExpr() = xmlEventReader.getSink() and
80+
if xmlEventReader.getMethod().getParameterType(0) instanceof TypeString
81+
then n1.asExpr() = xmlEventReader.getArgument(1)
82+
else n1.asExpr() = xmlEventReader.getArgument(0)
83+
|
8084
n2.asExpr() = xmlEventReader
8185
)
8286
}
@@ -98,7 +102,7 @@ private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) {
98102
*/
99103
private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) {
100104
exists(DocumentBuilderParse documentBuilder |
101-
n1.asExpr() = documentBuilder.getSink() and
105+
n1.asExpr() = documentBuilder.getArgument(0) and
102106
n2.asExpr() = documentBuilder
103107
)
104108
}
@@ -114,60 +118,6 @@ private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) {
114118
)
115119
}
116120

117-
/**
118-
* Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or
119-
* `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or
120-
* `TransformerFactory.newTemplates(tainted)`.
121-
*/
122-
private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) {
123-
exists(MethodAccess ma, Method m | ma.getMethod() = m |
124-
n1.asExpr() = ma.getAnArgument() and
125-
n2.asExpr() = ma and
126-
m.getDeclaringType() instanceof TransformerFactory and
127-
m.hasName(["newTransformer", "newTemplates"]) and
128-
not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf |
129-
conf.hasFlowToExpr(ma.getQualifier())
130-
)
131-
)
132-
}
133-
134-
/**
135-
* A data flow configuration for secure processing feature that is enabled on `TransformerFactory`.
136-
*/
137-
private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration {
138-
TransformerFactoryWithSecureProcessingFeatureFlowConfig() {
139-
this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig"
140-
}
141-
142-
override predicate isSource(DataFlow::Node src) {
143-
exists(Variable v | v = src.asExpr().(VarAccess).getVariable() |
144-
exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() |
145-
config.enables(configSecureProcessing())
146-
)
147-
)
148-
}
149-
150-
override predicate isSink(DataFlow::Node sink) {
151-
exists(MethodAccess ma |
152-
sink.asExpr() = ma.getQualifier() and
153-
ma.getMethod().getDeclaringType() instanceof TransformerFactory
154-
)
155-
}
156-
157-
override int fieldFlowBranchLimit() { result = 0 }
158-
}
159-
160-
/** A `ParserConfig` specific to `TransformerFactory`. */
161-
private class TransformerFactoryFeatureConfig extends ParserConfig {
162-
TransformerFactoryFeatureConfig() {
163-
exists(Method m |
164-
m = this.getMethod() and
165-
m.getDeclaringType() instanceof TransformerFactory and
166-
m.hasName("setFeature")
167-
)
168-
}
169-
}
170-
171121
/**
172122
* Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`,
173123
* i.e. `tainted.newTransformer()`.
@@ -252,3 +202,47 @@ private class TypeXsltExecutable extends Class {
252202
private class TypeXsltPackage extends Class {
253203
TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") }
254204
}
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/src/semmle/code/java/security/XsltInjectionQuery.qll

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java
44
import semmle.code.java.dataflow.FlowSources
55
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.security.XmlParsers
67
import semmle.code.java.security.XsltInjection
78

89
/**
@@ -23,3 +24,67 @@ class XsltInjectionFlowConfig extends TaintTracking::Configuration {
2324
any(XsltInjectionAdditionalTaintStep c).step(node1, node2)
2425
}
2526
}
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+
}

0 commit comments

Comments
 (0)