Skip to content

Commit 753d886

Browse files
smowtonsmowton
authored
Merge pull request #6319 from haby0/java/MyBatisSqlInjection
[Java] CWE-089 MyBatis Mapper Sql Injection
2 parents 6d247bf + 75f3ebf commit 753d886

27 files changed

+1160
-0
lines changed

‎java/ql/lib/semmle/code/java/frameworks/MyBatis.qll

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,81 @@ private class SqlSinkCsv extends SinkModelCsv {
2424
]
2525
}
2626
}
27+
28+
/** The class `org.apache.ibatis.session.Configuration`. */
29+
class IbatisConfiguration extends RefType {
30+
IbatisConfiguration() { this.hasQualifiedName("org.apache.ibatis.session", "Configuration") }
31+
}
32+
33+
/**
34+
* The method `getVariables()` declared in `org.apache.ibatis.session.Configuration`.
35+
*/
36+
class IbatisConfigurationGetVariablesMethod extends Method {
37+
IbatisConfigurationGetVariablesMethod() {
38+
this.getDeclaringType() instanceof IbatisConfiguration and
39+
this.hasName("getVariables") and
40+
this.getNumberOfParameters() = 0
41+
}
42+
}
43+
44+
/**
45+
* An annotation type that identifies Ibatis select.
46+
*/
47+
private class IbatisSelectAnnotationType extends AnnotationType {
48+
IbatisSelectAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Select") }
49+
}
50+
51+
/**
52+
* An annotation type that identifies Ibatis delete.
53+
*/
54+
private class IbatisDeleteAnnotationType extends AnnotationType {
55+
IbatisDeleteAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Delete") }
56+
}
57+
58+
/**
59+
* An annotation type that identifies Ibatis insert.
60+
*/
61+
private class IbatisInsertAnnotationType extends AnnotationType {
62+
IbatisInsertAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Insert") }
63+
}
64+
65+
/**
66+
* An annotation type that identifies Ibatis update.
67+
*/
68+
private class IbatisUpdateAnnotationType extends AnnotationType {
69+
IbatisUpdateAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Update") }
70+
}
71+
72+
/**
73+
* An Ibatis SQL operation annotation.
74+
*/
75+
class IbatisSqlOperationAnnotation extends Annotation {
76+
IbatisSqlOperationAnnotation() {
77+
this.getType() instanceof IbatisSelectAnnotationType or
78+
this.getType() instanceof IbatisDeleteAnnotationType or
79+
this.getType() instanceof IbatisInsertAnnotationType or
80+
this.getType() instanceof IbatisUpdateAnnotationType
81+
}
82+
83+
/**
84+
* Gets this annotation's SQL statement string.
85+
*/
86+
string getSqlValue() {
87+
result = this.getAValue("value").(CompileTimeConstantExpr).getStringValue()
88+
}
89+
}
90+
91+
/**
92+
* Methods annotated with `@org.apache.ibatis.annotations.Select` or `@org.apache.ibatis.annotations.Delete`
93+
* or `@org.apache.ibatis.annotations.Update` or `@org.apache.ibatis.annotations.Insert`.
94+
*/
95+
class MyBatisSqlOperationAnnotationMethod extends Method {
96+
MyBatisSqlOperationAnnotationMethod() {
97+
this.getAnAnnotation() instanceof IbatisSqlOperationAnnotation
98+
}
99+
}
100+
101+
/** The interface `org.apache.ibatis.annotations.Param`. */
102+
class TypeParam extends Interface {
103+
TypeParam() { this.hasQualifiedName("org.apache.ibatis.annotations", "Param") }
104+
}

‎java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import org.apache.ibatis.annotations.Select;
2+
3+
public interface MyBatisAnnotationSqlInjection {
4+
5+
@Select("select * from test where name = ${name}")
6+
public Test bad1(String name);
7+
8+
@Select("select * from test where name = #{name}")
9+
public Test good1(String name);
10+
}

‎java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.qhelp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>MyBatis uses methods with the annotations <code>@Select</code>, <code>@Insert</code>, etc. to construct dynamic SQL statements.
7+
If the syntax <code>${param}</code> is used in those statements, and <code>param</code> is a parameter of the annotated method, attackers can exploit this to tamper with the SQL statements or execute arbitrary SQL commands.</p>
8+
</overview>
9+
10+
<recommendation>
11+
<p>
12+
When writing MyBatis mapping statements, use the syntax <code>#{xxx}</code> whenever possible. If the syntax <code>${xxx}</code> must be used, any parameters included in it should be sanitized to prevent SQL injection attacks.
13+
</p>
14+
</recommendation>
15+
16+
<example>
17+
<p>
18+
The following sample shows a bad and a good example of MyBatis annotations usage. The <code>bad1</code> method uses <code>$(name)</code>
19+
in the <code>@Select</code> annotation to dynamically build a SQL statement, which causes a SQL injection vulnerability.
20+
The <code>good1</code> method uses <code>#{name}</code> in the <code>@Select</code> annotation to dynamically include the parameter in a SQL statement, which causes the MyBatis framework to sanitize the input provided, preventing the vulnerability.
21+
</p>
22+
<sample src="MyBatisAnnotationSqlInjection.java" />
23+
</example>
24+
25+
<references>
26+
<li>
27+
Fortify:
28+
<a href="https://vulncat.fortify.com/en/detail?id=desc.config.java.sql_injection_mybatis_mapper">SQL Injection: MyBatis Mapper</a>.
29+
</li>
30+
</references>
31+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjection.ql

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/**
2+
* @name SQL injection in MyBatis annotation
3+
* @description Constructing a dynamic SQL statement with input that comes from an
4+
* untrusted source could allow an attacker to modify the statement's
5+
* meaning or to execute arbitrary SQL commands.
6+
* @kind path-problem
7+
* @problem.severity error
8+
* @precision high
9+
* @id java/mybatis-annotation-sql-injection
10+
* @tags security
11+
* external/cwe/cwe-089
12+
*/
13+
14+
import java
15+
import DataFlow::PathGraph
16+
import MyBatisCommonLib
17+
import MyBatisAnnotationSqlInjectionLib
18+
import semmle.code.java.dataflow.FlowSources
19+
20+
private class MyBatisAnnotationSqlInjectionConfiguration extends TaintTracking::Configuration {
21+
MyBatisAnnotationSqlInjectionConfiguration() { this = "MyBatis annotation sql injection" }
22+
23+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
24+
25+
override predicate isSink(DataFlow::Node sink) {
26+
sink instanceof MyBatisAnnotatedMethodCallArgument
27+
}
28+
29+
override predicate isSanitizer(DataFlow::Node node) {
30+
node.getType() instanceof PrimitiveType or
31+
node.getType() instanceof BoxedType or
32+
node.getType() instanceof NumberType
33+
}
34+
35+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
36+
exists(MethodAccess ma |
37+
ma.getMethod().getDeclaringType() instanceof TypeObject and
38+
ma.getMethod().getName() = "toString" and
39+
ma.getQualifier() = node1.asExpr() and
40+
ma = node2.asExpr()
41+
)
42+
}
43+
}
44+
45+
from
46+
MyBatisAnnotationSqlInjectionConfiguration cfg, DataFlow::PathNode source,
47+
DataFlow::PathNode sink, IbatisSqlOperationAnnotation isoa, MethodAccess ma,
48+
string unsafeExpression
49+
where
50+
cfg.hasFlowPath(source, sink) and
51+
ma.getAnArgument() = sink.getNode().asExpr() and
52+
myBatisSqlOperationAnnotationFromMethod(ma.getMethod(), isoa) and
53+
unsafeExpression = getAMybatisAnnotationSqlValue(isoa) and
54+
(
55+
isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, unsafeExpression) or
56+
isMybatisCollectionTypeSqlInjection(sink.getNode(), ma, unsafeExpression)
57+
)
58+
select sink.getNode(), source, sink,
59+
"MyBatis annotation SQL injection might include code from $@ to $@.", source.getNode(),
60+
"this user input", isoa, "this SQL operation"

‎java/ql/src/experimental/Security/CWE/CWE-089/MyBatisAnnotationSqlInjectionLib.qll

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* Provides classes for SQL injection detection regarding MyBatis annotated methods.
3+
*/
4+
5+
import java
6+
import MyBatisCommonLib
7+
import semmle.code.java.dataflow.FlowSources
8+
import semmle.code.java.frameworks.Properties
9+
10+
/** An argument of a MyBatis annotated method. */
11+
class MyBatisAnnotatedMethodCallArgument extends DataFlow::Node {
12+
MyBatisAnnotatedMethodCallArgument() {
13+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma | ma.getMethod() = msoam |
14+
ma.getAnArgument() = this.asExpr()
15+
)
16+
}
17+
}

‎java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
/**
2+
* Provides public classes for MyBatis SQL injection detection.
3+
*/
4+
5+
import java
6+
import semmle.code.xml.MyBatisMapperXML
7+
import semmle.code.java.dataflow.FlowSources
8+
import semmle.code.java.frameworks.MyBatis
9+
import semmle.code.java.frameworks.Properties
10+
11+
private predicate propertiesKey(DataFlow::Node prop, string key) {
12+
exists(MethodAccess m |
13+
m.getMethod() instanceof PropertiesSetPropertyMethod and
14+
key = m.getArgument(0).(CompileTimeConstantExpr).getStringValue() and
15+
prop.asExpr() = m.getQualifier()
16+
)
17+
}
18+
19+
/** A data flow configuration tracing flow from ibatis `Configuration.getVariables()` to a store into a `Properties` object. */
20+
private class PropertiesFlowConfig extends DataFlow2::Configuration {
21+
PropertiesFlowConfig() { this = "PropertiesFlowConfig" }
22+
23+
override predicate isSource(DataFlow::Node src) {
24+
exists(MethodAccess ma | ma.getMethod() instanceof IbatisConfigurationGetVariablesMethod |
25+
src.asExpr() = ma
26+
)
27+
}
28+
29+
override predicate isSink(DataFlow::Node sink) { propertiesKey(sink, _) }
30+
}
31+
32+
/** Gets a `Properties` key that may map onto a Mybatis `Configuration` variable. */
33+
string getAMybatisConfigurationVariableKey() {
34+
exists(PropertiesFlowConfig conf, DataFlow::Node n |
35+
propertiesKey(n, result) and
36+
conf.hasFlowTo(n)
37+
)
38+
}
39+
40+
/** A reference type that extends a parameterization of `java.util.List`. */
41+
class ListType extends RefType {
42+
ListType() {
43+
this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List")
44+
}
45+
}
46+
47+
/** Holds if the specified `method` uses MyBatis Mapper XMLElement `mmxx`. */
48+
predicate myBatisMapperXMLElementFromMethod(Method method, MyBatisMapperXMLElement mmxx) {
49+
exists(MyBatisMapperSqlOperation mbmxe | mbmxe.getMapperMethod() = method |
50+
mbmxe.getAChild*() = mmxx
51+
or
52+
exists(MyBatisMapperSql mbms |
53+
mbmxe.getInclude().getRefid() = mbms.getId() and
54+
mbms.getAChild*() = mmxx
55+
)
56+
)
57+
}
58+
59+
/** Holds if the specified `method` has Ibatis Sql operation annotation `isoa`. */
60+
predicate myBatisSqlOperationAnnotationFromMethod(Method method, IbatisSqlOperationAnnotation isoa) {
61+
exists(MyBatisSqlOperationAnnotationMethod msoam |
62+
msoam = method and
63+
msoam.getAnAnnotation() = isoa
64+
)
65+
}
66+
67+
/** Gets a `#{...}` or `${...}` expression argument in XML element `xmle`. */
68+
string getAMybatisXmlSetValue(XMLElement xmle) {
69+
result = xmle.getTextValue().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
70+
}
71+
72+
/** Gets a `#{...}` or `${...}` expression argument in annotation `isoa`. */
73+
string getAMybatisAnnotationSqlValue(IbatisSqlOperationAnnotation isoa) {
74+
result = isoa.getSqlValue().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
75+
}
76+
77+
/**
78+
* Holds if `node` is an argument to `ma` that is vulnerable to SQL injection attacks if `unsafeExpression` occurs in a MyBatis SQL expression.
79+
*
80+
* This case currently assumes all `${...}` expressions are potentially dangerous when there is a non-`@Param` annotated, collection-typed parameter to `ma`.
81+
*/
82+
bindingset[unsafeExpression]
83+
predicate isMybatisCollectionTypeSqlInjection(
84+
DataFlow::Node node, MethodAccess ma, string unsafeExpression
85+
) {
86+
not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and
87+
// The parameter type of the MyBatis method parameter is Map or List or Array.
88+
// SQL injection vulnerability caused by improper use of this parameter.
89+
// e.g.
90+
//
91+
// ```java
92+
// @Select(select id,name from test where name like '%${value}%')
93+
// Test test(Map map);
94+
// ```
95+
exists(int i |
96+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
97+
(
98+
ma.getMethod().getParameterType(i) instanceof MapType or
99+
ma.getMethod().getParameterType(i) instanceof ListType or
100+
ma.getMethod().getParameterType(i) instanceof Array
101+
) and
102+
unsafeExpression.matches("${%}") and
103+
ma.getArgument(i) = node.asExpr()
104+
)
105+
}
106+
107+
/**
108+
* Holds if `node` is an argument to `ma` that is vulnerable to SQL injection attacks if `unsafeExpression` occurs in a MyBatis SQL expression.
109+
*
110+
* This accounts for:
111+
* - arguments referred to by a name given in a `@Param` annotation,
112+
* - arguments referred to by ordinal position, like `${param1}`
113+
* - references to class instance fields
114+
* - any `${}` expression where there is a single, non-`@Param`-annotated argument to `ma`.
115+
*/
116+
bindingset[unsafeExpression]
117+
predicate isMybatisXmlOrAnnotationSqlInjection(
118+
DataFlow::Node node, MethodAccess ma, string unsafeExpression
119+
) {
120+
not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and
121+
(
122+
// The method parameters use `@Param` annotation. Due to improper use of this parameter, SQL injection vulnerabilities are caused.
123+
// e.g.
124+
//
125+
// ```java
126+
// @Select(select id,name from test order by ${orderby,jdbcType=VARCHAR})
127+
// void test(@Param("orderby") String name);
128+
// ```
129+
exists(Annotation annotation |
130+
unsafeExpression
131+
.matches("${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
132+
"%}") and
133+
annotation.getType() instanceof TypeParam and
134+
ma.getAnArgument() = node.asExpr()
135+
)
136+
or
137+
// MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n].
138+
// e.g.
139+
//
140+
// ```java
141+
// @Select(select id,name from test order by ${arg0,jdbcType=VARCHAR})
142+
// void test(String name);
143+
// ```
144+
exists(int i |
145+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
146+
(
147+
unsafeExpression.matches("${param" + (i + 1) + "%}")
148+
or
149+
unsafeExpression.matches("${arg" + i + "%}")
150+
) and
151+
ma.getArgument(i) = node.asExpr()
152+
)
153+
or
154+
// SQL injection vulnerability caused by improper use of MyBatis instance class fields.
155+
// e.g.
156+
//
157+
// ```java
158+
// @Select(select id,name from test order by ${name,jdbcType=VARCHAR})
159+
// void test(Test test);
160+
// ```
161+
exists(int i, RefType t |
162+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
163+
ma.getMethod().getParameterType(i).getName() = t.getName() and
164+
unsafeExpression.matches("${" + t.getAField().getName() + "%}") and
165+
ma.getArgument(i) = node.asExpr()
166+
)
167+
or
168+
// This method has only one parameter and the parameter is not annotated with `@Param`. The parameter can be named arbitrarily in the SQL statement.
169+
// If the number of method variables is greater than one, they cannot be named arbitrarily.
170+
// Improper use of this parameter has a SQL injection vulnerability.
171+
// e.g.
172+
//
173+
// ```java
174+
// @Select(select id,name from test where name like '%${value}%')
175+
// Test test(String name);
176+
// ```
177+
exists(int i | i = 1 |
178+
ma.getMethod().getNumberOfParameters() = i and
179+
not ma.getMethod().getAParameter().getAnAnnotation().getType() instanceof TypeParam and
180+
unsafeExpression.matches("${%}") and
181+
ma.getAnArgument() = node.asExpr()
182+
)
183+
)
184+
}

0 commit comments

Comments
 (0)