Skip to content

Commit fb9feab

Browse files
aschackmullaschackmull
authored
Merge pull request github#6062 from atorralba/atorralba/promote-groovy-injection
Java: Promote Groovy Code Injection from experimental
2 parents 43044cd + c44de87 commit fb9feab

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

45 files changed

+1715
-521
lines changed

‎java/change-notes/2021-06-14-groovy-code-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 "Groovy Language injection" (`java/groovy-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 @p0wn4j](https://github.com/github/codeql/pull/5467).

‎java/ql/src/experimental/Security/CWE/CWE-094/GroovyInjection.qhelp renamed to ‎java/ql/src/Security/CWE/CWE-094/GroovyInjection.qhelp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@ This is typically done when using Groovy for its scripting or domain specific la
2828
The fundamental problem is that Groovy is a dynamic language, yet <code>SecureASTCustomizer</code> works by looking at Groovy AST statically.
2929

3030
This makes it very easy for an attacker to bypass many of the intended checks
31-
(see https://kohsuke.org/2012/04/27/groovy-secureastcustomizer-is-harmful/).
31+
(see [Groovy SecureASTCustomizer is harmful](https://kohsuke.org/2012/04/27/groovy-secureastcustomizer-is-harmful/)).
3232
Therefore, besides <code>SecureASTCustomizer</code>, runtime checks are also necessary before calling Groovy methods
33-
(see https://melix.github.io/blog/2015/03/sandboxing.html).
33+
(see [Improved sandboxing of Groovy scripts](https://melix.github.io/blog/2015/03/sandboxing.html)).
3434

3535
It is also possible to use a block-list method, excluding unwanted classes from being loaded by the JVM.
3636
This method is not always recommended, because block-lists can be bypassed by unexpected values.

‎java/ql/src/experimental/Security/CWE/CWE-094/GroovyInjection.ql renamed to ‎java/ql/src/Security/CWE/CWE-094/GroovyInjection.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
*/
1212

1313
import java
14+
import semmle.code.java.security.GroovyInjectionQuery
1415
import DataFlow::PathGraph
15-
import GroovyInjectionLib
1616

1717
from DataFlow::PathNode source, DataFlow::PathNode sink, GroovyInjectionConfig conf
1818
where conf.hasFlowPath(source, sink)

‎java/ql/src/experimental/Security/CWE/CWE-094/GroovyInjectionBad.java renamed to ‎java/ql/src/Security/CWE/CWE-094/GroovyInjectionBad.java

File renamed without changes.

‎java/ql/src/experimental/Security/CWE/CWE-094/GroovyInjectionBlocklist.java renamed to ‎java/ql/src/Security/CWE/CWE-094/GroovyInjectionBlocklist.java

File renamed without changes.

‎java/ql/src/experimental/Security/CWE/CWE-094/GroovyInjectionLib.qll

Lines changed: 0 additions & 160 deletions
This file was deleted.

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ private module Frameworks {
9797
private import semmle.code.java.frameworks.spring.SpringWebMultipart
9898
private import semmle.code.java.security.ResponseSplitting
9999
private import semmle.code.java.security.InformationLeak
100+
private import semmle.code.java.security.GroovyInjection
100101
private import semmle.code.java.security.JexlInjectionSinkModels
101102
private import semmle.code.java.security.LdapInjection
102103
private import semmle.code.java.security.MvelInjection
@@ -329,6 +330,7 @@ private predicate summaryModelCsv(string row) {
329330
"java.io;File;false;File;;;Argument[0];Argument[-1];taint",
330331
"java.io;File;false;File;;;Argument[1];Argument[-1];taint",
331332
"java.net;URI;false;URI;(String);;Argument[0];Argument[-1];taint",
333+
"java.net;URL;false;URL;(String);;Argument[0];Argument[-1];taint",
332334
"javax.xml.transform.stream;StreamSource;false;StreamSource;;;Argument[0];Argument[-1];taint",
333335
"javax.xml.transform.sax;SAXSource;false;SAXSource;(InputSource);;Argument[0];Argument[-1];taint",
334336
"javax.xml.transform.sax;SAXSource;false;SAXSource;(XMLReader,InputSource);;Argument[1];Argument[-1];taint",

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

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/** Provides classes to reason about Groovy code injection attacks. */
2+
3+
private import semmle.code.java.dataflow.DataFlow
4+
private import semmle.code.java.dataflow.ExternalFlow
5+
private import semmle.code.java.frameworks.Networking
6+
7+
/** A data flow sink for Groovy expression injection vulnerabilities. */
8+
abstract class GroovyInjectionSink extends DataFlow::ExprNode { }
9+
10+
/**
11+
* A unit class for adding additional taint steps.
12+
*
13+
* Extend this class to add additional taint steps that should apply to the `GroovyInjectionConfig`.
14+
*/
15+
class GroovyInjectionAdditionalTaintStep extends Unit {
16+
/**
17+
* Holds if the step from `node1` to `node2` should be considered a taint
18+
* step for the `GroovyInjectionConfig` configuration.
19+
*/
20+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
21+
}
22+
23+
private class DefaultGroovyInjectionSink extends GroovyInjectionSink {
24+
DefaultGroovyInjectionSink() { sinkNode(this, "groovy") }
25+
}
26+
27+
private class DefaultGroovyInjectionSinkModel extends SinkModelCsv {
28+
override predicate row(string row) {
29+
row =
30+
[
31+
// Signatures are specified to exclude sinks of the type `File`
32+
"groovy.lang;GroovyShell;false;evaluate;(GroovyCodeSource);;Argument[0];groovy",
33+
"groovy.lang;GroovyShell;false;evaluate;(Reader);;Argument[0];groovy",
34+
"groovy.lang;GroovyShell;false;evaluate;(Reader,String);;Argument[0];groovy",
35+
"groovy.lang;GroovyShell;false;evaluate;(String);;Argument[0];groovy",
36+
"groovy.lang;GroovyShell;false;evaluate;(String,String);;Argument[0];groovy",
37+
"groovy.lang;GroovyShell;false;evaluate;(String,String,String);;Argument[0];groovy",
38+
"groovy.lang;GroovyShell;false;evaluate;(URI);;Argument[0];groovy",
39+
"groovy.lang;GroovyShell;false;parse;(Reader);;Argument[0];groovy",
40+
"groovy.lang;GroovyShell;false;parse;(Reader,String);;Argument[0];groovy",
41+
"groovy.lang;GroovyShell;false;parse;(String);;Argument[0];groovy",
42+
"groovy.lang;GroovyShell;false;parse;(String,String);;Argument[0];groovy",
43+
"groovy.lang;GroovyShell;false;parse;(URI);;Argument[0];groovy",
44+
"groovy.lang;GroovyShell;false;run;(GroovyCodeSource,String[]);;Argument[0];groovy",
45+
"groovy.lang;GroovyShell;false;run;(GroovyCodeSource,List);;Argument[0];groovy",
46+
"groovy.lang;GroovyShell;false;run;(Reader,String,String[]);;Argument[0];groovy",
47+
"groovy.lang;GroovyShell;false;run;(Reader,String,List);;Argument[0];groovy",
48+
"groovy.lang;GroovyShell;false;run;(String,String,String[]);;Argument[0];groovy",
49+
"groovy.lang;GroovyShell;false;run;(String,String,List);;Argument[0];groovy",
50+
"groovy.lang;GroovyShell;false;run;(URI,String[]);;Argument[0];groovy",
51+
"groovy.lang;GroovyShell;false;run;(URI,List);;Argument[0];groovy",
52+
"groovy.util;Eval;false;me;(String);;Argument[0];groovy",
53+
"groovy.util;Eval;false;me;(String,Object,String);;Argument[2];groovy",
54+
"groovy.util;Eval;false;x;(Object,String);;Argument[1];groovy",
55+
"groovy.util;Eval;false;xy;(Object,Object,String);;Argument[2];groovy",
56+
"groovy.util;Eval;false;xyz;(Object,Object,Object,String);;Argument[3];groovy",
57+
"groovy.lang;GroovyClassLoader;false;parseClass;(GroovyCodeSource);;Argument[0];groovy",
58+
"groovy.lang;GroovyClassLoader;false;parseClass;(GroovyCodeSource,boolean);;Argument[0];groovy",
59+
"groovy.lang;GroovyClassLoader;false;parseClass;(InputStream,String);;Argument[0];groovy",
60+
"groovy.lang;GroovyClassLoader;false;parseClass;(Reader,String);;Argument[0];groovy",
61+
"groovy.lang;GroovyClassLoader;false;parseClass;(String);;Argument[0];groovy",
62+
"groovy.lang;GroovyClassLoader;false;parseClass;(String,String);;Argument[0];groovy",
63+
"org.codehaus.groovy.control;CompilationUnit;false;compile;;;Argument[-1];groovy"
64+
]
65+
}
66+
}
67+
68+
/** A set of additional taint steps to consider when taint tracking Groovy related data flows. */
69+
private class DefaultGroovyInjectionAdditionalTaintStep extends GroovyInjectionAdditionalTaintStep {
70+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
71+
groovyCodeSourceTaintStep(node1, node2) or
72+
groovyCompilationUnitTaintStep(node1, node2) or
73+
groovySourceUnitTaintStep(node1, node2) or
74+
groovyReaderSourceTaintStep(node1, node2)
75+
}
76+
}
77+
78+
/**
79+
* Holds if `fromNode` to `toNode` is a dataflow step from a tainted string to
80+
* a `GroovyCodeSource` instance by calling `new GroovyCodeSource(tainted, ...)`.
81+
*/
82+
private predicate groovyCodeSourceTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
83+
exists(ConstructorCall gcscc |
84+
gcscc.getConstructedType() instanceof TypeGroovyCodeSource and
85+
gcscc = toNode.asExpr() and
86+
gcscc.getArgument(0) = fromNode.asExpr()
87+
)
88+
}
89+
90+
/**
91+
* Holds if `fromNode` to `toNode` is a dataflow step from a tainted object to
92+
* a `CompilationUnit` instance by calling `compilationUnit.addSource(..., tainted)`.
93+
*/
94+
private predicate groovyCompilationUnitTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
95+
exists(MethodAccess ma, Method m |
96+
ma.getMethod() = m and
97+
m.hasName("addSource") and
98+
m.getDeclaringType() instanceof TypeGroovyCompilationUnit
99+
|
100+
fromNode.asExpr() = ma.getArgument(ma.getNumArgument() - 1) and
101+
toNode.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getQualifier()
102+
)
103+
}
104+
105+
/**
106+
* Holds if `fromNode` to `toNode` is a dataflow step from a tainted object to
107+
* a `SourceUnit` instance by calling `new SourceUnit(..., tainted, ...)`
108+
* or `SourceUnit.create(..., tainted)`
109+
*/
110+
private predicate groovySourceUnitTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
111+
exists(ClassInstanceExpr cie, Argument arg, int index |
112+
cie.getConstructedType() instanceof TypeGroovySourceUnit and
113+
arg = cie.getArgument(index) and
114+
(
115+
index = 0 and arg.getType() instanceof TypeUrl
116+
or
117+
index = 1 and
118+
(
119+
arg.getType() instanceof TypeString or
120+
arg.getType() instanceof TypeReaderSource
121+
)
122+
)
123+
|
124+
fromNode.asExpr() = arg and
125+
toNode.asExpr() = cie
126+
)
127+
or
128+
exists(MethodAccess ma, Method m |
129+
ma.getMethod() = m and
130+
m.hasName("create") and
131+
m.getDeclaringType() instanceof TypeGroovySourceUnit
132+
|
133+
fromNode.asExpr() = ma.getArgument(1) and toNode.asExpr() = ma
134+
)
135+
}
136+
137+
/**
138+
* Holds if `fromNode` to `toNode` is a dataflow step from a tainted object to
139+
* a `ReaderSource` instance by calling `new ReaderSource(tainted, ...)`.
140+
*/
141+
private predicate groovyReaderSourceTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
142+
exists(ClassInstanceExpr cie | cie.getConstructedType() instanceof TypeReaderSource |
143+
fromNode.asExpr() = cie.getArgument(0) and toNode.asExpr() = cie
144+
)
145+
}
146+
147+
/** The class `groovy.lang.GroovyCodeSource`. */
148+
private class TypeGroovyCodeSource extends RefType {
149+
TypeGroovyCodeSource() { this.hasQualifiedName("groovy.lang", "GroovyCodeSource") }
150+
}
151+
152+
/** The class `org.codehaus.groovy.control.CompilationUnit`. */
153+
private class TypeGroovyCompilationUnit extends RefType {
154+
TypeGroovyCompilationUnit() {
155+
this.hasQualifiedName("org.codehaus.groovy.control", "CompilationUnit")
156+
}
157+
}
158+
159+
/** The class `org.codehaus.groovy.control.CompilationUnit`. */
160+
private class TypeGroovySourceUnit extends RefType {
161+
TypeGroovySourceUnit() { this.hasQualifiedName("org.codehaus.groovy.control", "SourceUnit") }
162+
}
163+
164+
/** The class `org.codehaus.groovy.control.io.ReaderSource`. */
165+
private class TypeReaderSource extends RefType {
166+
TypeReaderSource() {
167+
this.getASupertype*().hasQualifiedName("org.codehaus.groovy.control.io", "ReaderSource")
168+
}
169+
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/** Provides taint tracking configurations relating to Groovy injection vulnerabilities. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.security.GroovyInjection
7+
8+
/**
9+
* A taint-tracking configuration for unsafe user input
10+
* that is used to evaluate a Groovy expression.
11+
*/
12+
class GroovyInjectionConfig extends TaintTracking::Configuration {
13+
GroovyInjectionConfig() { this = "GroovyInjectionConfig" }
14+
15+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
16+
17+
override predicate isSink(DataFlow::Node sink) { sink instanceof GroovyInjectionSink }
18+
19+
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
20+
any(GroovyInjectionAdditionalTaintStep c).step(fromNode, toNode)
21+
}
22+
}

0 commit comments

Comments
 (0)