Skip to content

Commit 08be8ed

Browse files
haby0haby0
committed
Modify according to suggestions
1 parent db04a0d commit 08be8ed

16 files changed

+155
-259
lines changed

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

Lines changed: 25 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -35,124 +35,72 @@ class IbatisConfiguration extends RefType {
3535
*/
3636
class IbatisConfigurationGetVariablesMethod extends Method {
3737
IbatisConfigurationGetVariablesMethod() {
38-
getDeclaringType() instanceof IbatisConfiguration and
39-
hasName("getVariables") and
40-
getNumberOfParameters() = 0
38+
this.getDeclaringType() instanceof IbatisConfiguration and
39+
this.hasName("getVariables") and
40+
this.getNumberOfParameters() = 0
4141
}
4242
}
4343

4444
/**
4545
* An annotation type that identifies Ibatis select.
4646
*/
4747
private class IbatisSelectAnnotationType extends AnnotationType {
48-
IbatisSelectAnnotationType() {
49-
this.hasQualifiedName("org.apache.ibatis.annotations", "Select") or
50-
this.getAnAnnotation().getType() instanceof IbatisSelectAnnotationType
51-
}
48+
IbatisSelectAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Select") }
5249
}
5350

5451
/**
5552
* An annotation type that identifies Ibatis delete.
5653
*/
5754
private class IbatisDeleteAnnotationType extends AnnotationType {
58-
IbatisDeleteAnnotationType() {
59-
this.hasQualifiedName("org.apache.ibatis.annotations", "Delete") or
60-
this.getAnAnnotation().getType() instanceof IbatisDeleteAnnotationType
61-
}
55+
IbatisDeleteAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Delete") }
6256
}
6357

6458
/**
6559
* An annotation type that identifies Ibatis insert.
6660
*/
6761
private class IbatisInsertAnnotationType extends AnnotationType {
68-
IbatisInsertAnnotationType() {
69-
this.hasQualifiedName("org.apache.ibatis.annotations", "Insert") or
70-
this.getAnAnnotation().getType() instanceof IbatisInsertAnnotationType
71-
}
62+
IbatisInsertAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Insert") }
7263
}
7364

7465
/**
7566
* An annotation type that identifies Ibatis update.
7667
*/
7768
private class IbatisUpdateAnnotationType extends AnnotationType {
78-
IbatisUpdateAnnotationType() {
79-
this.hasQualifiedName("org.apache.ibatis.annotations", "Update") or
80-
this.getAnAnnotation().getType() instanceof IbatisUpdateAnnotationType
81-
}
69+
IbatisUpdateAnnotationType() { this.hasQualifiedName("org.apache.ibatis.annotations", "Update") }
8270
}
8371

8472
/**
8573
* Ibatis sql operation annotation.
8674
*/
87-
abstract class IbatisSqlOperationAnnotation extends Annotation {
88-
abstract string getSqlValue();
89-
}
90-
91-
/**
92-
* A `@org.apache.ibatis.annotations.Select` annotation.
93-
*/
94-
private class IbatisSelectAnnotation extends IbatisSqlOperationAnnotation {
95-
IbatisSelectAnnotation() { this.getType() instanceof IbatisSelectAnnotationType }
96-
97-
string getSelectValue() {
98-
result = this.getValue("value").(CompileTimeConstantExpr).getStringValue() or
99-
result =
100-
this.getValue("value").(ArrayInit).getInit(_).(CompileTimeConstantExpr).getStringValue()
101-
}
102-
103-
override string getSqlValue() { result = getSelectValue() }
104-
}
105-
106-
/**
107-
* A `@org.apache.ibatis.annotations.Delete` annotation.
108-
*/
109-
private class IbatisDeleteAnnotation extends IbatisSqlOperationAnnotation {
110-
IbatisDeleteAnnotation() { this.getType() instanceof IbatisDeleteAnnotationType }
111-
112-
string getDeleteValue() {
113-
result = this.getValue("value").(CompileTimeConstantExpr).getStringValue() or
114-
result =
115-
this.getValue("value").(ArrayInit).getInit(_).(CompileTimeConstantExpr).getStringValue()
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
11681
}
11782

118-
override string getSqlValue() { result = getDeleteValue() }
119-
}
120-
121-
/**
122-
* A `@org.apache.ibatis.annotations.Insert` annotation.
123-
*/
124-
private class IbatisInsertAnnotation extends IbatisSqlOperationAnnotation {
125-
IbatisInsertAnnotation() { this.getType() instanceof IbatisInsertAnnotationType }
126-
127-
string getInsertValue() {
83+
/**
84+
* Get the SQL statement string.
85+
*/
86+
string getSqlValue() {
12887
result = this.getValue("value").(CompileTimeConstantExpr).getStringValue() or
12988
result =
13089
this.getValue("value").(ArrayInit).getInit(_).(CompileTimeConstantExpr).getStringValue()
13190
}
132-
133-
override string getSqlValue() { result = getInsertValue() }
13491
}
13592

13693
/**
137-
* A `@org.apache.ibatis.annotations.Update` annotation.
94+
* Methods annotated with `@org.apache.ibatis.annotations.Select` or `@org.apache.ibatis.annotations.Delete`
95+
* or `@org.apache.ibatis.annotations.Update` or `@org.apache.ibatis.annotations.Insert`.
13896
*/
139-
private class IbatisUpdateAnnotation extends IbatisSqlOperationAnnotation {
140-
IbatisUpdateAnnotation() { this.getType() instanceof IbatisUpdateAnnotationType }
141-
142-
string getUpdateValue() {
143-
result = this.getValue("value").(CompileTimeConstantExpr).getStringValue() or
144-
result =
145-
this.getValue("value").(ArrayInit).getInit(_).(CompileTimeConstantExpr).getStringValue()
97+
class MyBatisSqlOperationAnnotationMethod extends Method {
98+
MyBatisSqlOperationAnnotationMethod() {
99+
this.getAnAnnotation() instanceof IbatisSqlOperationAnnotation
146100
}
147-
148-
override string getSqlValue() { result = getUpdateValue() }
149101
}
150102

151-
// Mybatis uses sql operation to annotate the method of interacting with the database.
152-
class MybatisSqlOperationAnnotationMethod extends Method {
153-
MybatisSqlOperationAnnotationMethod() {
154-
exists(IbatisSqlOperationAnnotation isoa |
155-
this.getAnAnnotation() = isoa
156-
)
157-
}
103+
/** The interface `org.apache.ibatis.annotations.Param`. */
104+
class TypeParam extends Interface {
105+
TypeParam() { this.hasQualifiedName("org.apache.ibatis.annotations", "Param") }
158106
}

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,21 @@
44
"qhelp.dtd">
55
<qhelp>
66
<overview>
7-
<p>MyBatis operates the database by using @Select, @Insert, etc. annotations in the method, and can use the $ character
8-
to construct dynamic SQL statements. Attackers can modify the meaning of statements or execute arbitrary SQL commands.</p>
7+
<p>MyBatis allows operating the database by annotating a method with the annotations <code>@Select</code>, <code>@Insert</code>, etc. to construct dynamic SQL statements.
8+
If the syntax `${param}` is used in those statements, and `param` is a parameter of the annotated method, attackers can exploit this to tamper with the SQL statements or execute arbitrary SQL commands.</p>
99
</overview>
1010

1111
<<recommendation>
1212
<p>
13-
When writing MyBatis mapping statements, try to use the format "#{xxx}". If you have to use parameters
14-
such as "${xxx}", you must manually filter to prevent SQL injection attacks.
13+
When writing MyBatis mapping statements, try to use the syntax <code>#{xxx}</code>. If the syntax <code>${xxx}</code> must be used, any parameters included in it should be sanitized to prevent SQL injection attacks.
1514
</p>
1615
</recommendation>
1716

1817
<example>
1918
<p>
20-
The following examples show the bad situation and the good situation respectively. The <code>bad1</code> method uses <code>$(name)</code>
21-
in the <code>@Select</code> annotation to dynamically splice SQL statements, and there is a SQL injection vulnerability.
22-
The good1 method uses the <code>#{name}</code> method in the <code>@Select</code> annotation to splice SQL statements,
23-
and the MyBatis framework will handle the dangerous characters entered by the user, And did not cause SQL injection vulnerabilities.
19+
The following sample shows a bad and a good example of MyBatis annotations usage. The <code>bad1</code> method uses <code>$(name)</code>
20+
in the <code>@Select</code> annotation to dynamically build a SQL statement, which causes a SQL injection vulnerability.
21+
The <code>good1</code> method uses <code>#{name}</code> in the <code>@Select</code> annotation to to dynamically include the parameter in a SQL statement, which allows the MyBatis framework to handle the sanitization, preventing the vulnerability.
2422
</p>
2523
<sample src="MyBatisAnnotationSqlInjection.java" />
2624
</example>

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

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,19 @@
11
/**
2-
* @name MyBatis annotation sql injection
2+
* @name SQL injection in MyBatis annotation
33
* @description Constructing a dynamic SQL statement with input that comes from an
44
* untrusted source could allow an attacker to modify the statement's
55
* meaning or to execute arbitrary SQL commands.
66
* @kind path-problem
77
* @problem.severity error
88
* @precision high
9-
* @id java/sql-injection
9+
* @id java/mybatis-annotation-sql-injection
1010
* @tags security
1111
* external/cwe/cwe-089
1212
*/
1313

1414
import java
1515
import DataFlow::PathGraph
1616
import MyBatisAnnotationSqlInjectionLib
17-
import semmle.code.java.security.SanitizerGuard
1817
import semmle.code.java.dataflow.FlowSources
1918

2019
private class MyBatisAnnotationSqlInjectionConfiguration extends TaintTracking::Configuration {
@@ -32,10 +31,6 @@ private class MyBatisAnnotationSqlInjectionConfiguration extends TaintTracking::
3231
node.getType() instanceof NumberType
3332
}
3433

35-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
36-
guard instanceof ContainsSanitizer or guard instanceof EqualsSanitizer
37-
}
38-
3934
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
4035
exists(MethodAccess ma |
4136
ma.getMethod().getDeclaringType() instanceof MapType and
@@ -60,5 +55,5 @@ where
6055
cfg.hasFlowPath(source, sink) and
6156
isMybatisAnnotationSqlInjection(sink.getNode(), isoa)
6257
select sink.getNode(), source, sink,
63-
"MyBatis annotation sql injection might include code from $@ to $@.", source.getNode(),
64-
"this user input", isoa, "this sql operation"
58+
"MyBatis annotation SQL injection might include code from $@ to $@.", source.getNode(),
59+
"this user input", isoa, "this SQL operation"

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

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
/**
2+
* Provides classes for SQL injection detection in MyBatis annotation.
3+
*/
4+
15
import java
26
import MyBatisCommonLib
37
import semmle.code.xml.MyBatisMapperXML
@@ -7,15 +11,15 @@ import semmle.code.java.frameworks.Properties
711
/** A sink for MyBatis annotation method call an argument. */
812
class MyBatisAnnotationMethodCallAnArgument extends DataFlow::Node {
913
MyBatisAnnotationMethodCallAnArgument() {
10-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma | ma.getMethod() = msoam |
14+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma | ma.getMethod() = msoam |
1115
ma.getAnArgument() = this.asExpr()
1216
)
1317
}
1418
}
1519

1620
/** Get the #{...} or ${...} parameters in the Mybatis annotation value. */
1721
private string getAnMybatiAnnotationSetValue(IbatisSqlOperationAnnotation isoa) {
18-
result = isoa.getSqlValue().trim().regexpFind("(#|\\$)(\\{([^\\}]*\\}))", _, _)
22+
result = isoa.getSqlValue().trim().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
1923
}
2024

2125
predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperationAnnotation isoa) {
@@ -27,15 +31,15 @@ predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperatio
2731
// @Select(select id,name from test where name like '%${value}%')
2832
// Test test(String name);
2933
// ```
30-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma, string res |
34+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma, string res |
3135
msoam = ma.getMethod()
3236
|
3337
msoam.getAnAnnotation() = isoa and
3438
res = getAnMybatiAnnotationSetValue(isoa) and
3539
msoam.getNumberOfParameters() = 1 and
36-
not ma.getMethod().getAParameter().hasAnnotation() and
40+
not ma.getMethod().getAParameter().getAnAnnotation().getType() instanceof TypeParam and
3741
res.matches("%${%}") and
38-
not res.matches("${" + getAnMybatisConfigurationVariableKey() + "}") and
42+
not res = "${" + getAnMybatisConfigurationVariableKey() + "}" and
3943
ma.getAnArgument() = node.asExpr()
4044
)
4145
or
@@ -47,19 +51,19 @@ predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperatio
4751
// @Select(select id,name from test where name like '%${value}%')
4852
// Test test(Map map);
4953
// ```
50-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, string res |
54+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, string res |
5155
msoam = ma.getMethod()
5256
|
5357
msoam.getAnAnnotation() = isoa and
54-
not ma.getMethod().getParameter(i).hasAnnotation() and
58+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
5559
(
5660
ma.getMethod().getParameterType(i) instanceof MapType or
5761
ma.getMethod().getParameterType(i) instanceof ListType or
5862
ma.getMethod().getParameterType(i) instanceof Array
5963
) and
6064
res = getAnMybatiAnnotationSetValue(isoa) and
6165
res.matches("%${%}") and
62-
not res.matches("${" + getAnMybatisConfigurationVariableKey() + "}") and
66+
not res = "${" + getAnMybatisConfigurationVariableKey() + "}" and
6367
ma.getArgument(i) = node.asExpr()
6468
)
6569
or
@@ -71,13 +75,13 @@ predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperatio
7175
// @Select(select id,name from test order by ${name,jdbcType=VARCHAR})
7276
// void test(Test test);
7377
// ```
74-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, Class c |
78+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, RefType t |
7579
msoam = ma.getMethod()
7680
|
7781
msoam.getAnAnnotation() = isoa and
78-
not ma.getMethod().getParameter(i).hasAnnotation() and
79-
ma.getMethod().getParameterType(i).getName() = c.getName() and
80-
getAnMybatiAnnotationSetValue(isoa).matches("%${" + c.getAField().getName() + "%}") and
82+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
83+
ma.getMethod().getParameterType(i).getName() = t.getName() and
84+
getAnMybatiAnnotationSetValue(isoa).matches("%${" + t.getAField().getName() + "%}") and
8185
ma.getArgument(i) = node.asExpr()
8286
)
8387
or
@@ -89,12 +93,10 @@ predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperatio
8993
// @Select(select id,name from test order by ${orderby,jdbcType=VARCHAR})
9094
// void test(@Param("orderby") String name);
9195
// ```
92-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, Annotation annotation |
93-
msoam = ma.getMethod()
96+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, Annotation annotation |
97+
msoam = ma.getMethod() and ma.getMethod().getParameter(i).getAnAnnotation() = annotation
9498
|
9599
msoam.getAnAnnotation() = isoa and
96-
ma.getMethod().getParameter(i).hasAnnotation() and
97-
ma.getMethod().getParameter(i).getAnAnnotation() = annotation and
98100
annotation.getType() instanceof TypeParam and
99101
getAnMybatiAnnotationSetValue(isoa)
100102
.matches("%${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
@@ -109,18 +111,18 @@ predicate isMybatisAnnotationSqlInjection(DataFlow::Node node, IbatisSqlOperatio
109111
// @Select(select id,name from test order by ${arg0,jdbcType=VARCHAR})
110112
// void test(String name);
111113
// ```
112-
exists(MybatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, string res |
114+
exists(MyBatisSqlOperationAnnotationMethod msoam, MethodAccess ma, int i, string res |
113115
msoam = ma.getMethod()
114116
|
115117
msoam.getAnAnnotation() = isoa and
116-
not ma.getMethod().getParameter(i).hasAnnotation() and
118+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
117119
res = getAnMybatiAnnotationSetValue(isoa) and
118120
(
119121
res.matches("%${param" + (i + 1) + "%}")
120122
or
121123
res.matches("%${arg" + i + "%}")
122124
) and
123-
not res.matches("${" + getAnMybatisConfigurationVariableKey() + "}") and
125+
not res = "${" + getAnMybatisConfigurationVariableKey() + "}" and
124126
ma.getArgument(i) = node.asExpr()
125127
)
126128
}

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
/**
2+
* Provides public classes for MyBatis SQL injection detection.
3+
*/
4+
15
import java
26
import semmle.code.java.dataflow.FlowSources
37
import semmle.code.java.frameworks.MyBatis
@@ -28,15 +32,10 @@ private class PropertiesFlowConfig extends DataFlow2::Configuration {
2832
string getAnMybatisConfigurationVariableKey() {
2933
exists(PropertiesFlowConfig conf, DataFlow::Node n |
3034
propertiesKey(n, result) and
31-
conf.hasFlow(_, n)
35+
conf.hasFlowTo(n)
3236
)
3337
}
3438

35-
/** The interface `org.apache.ibatis.annotations.Param`. */
36-
class TypeParam extends Interface {
37-
TypeParam() { this.hasQualifiedName("org.apache.ibatis.annotations", "Param") }
38-
}
39-
4039
/** A reference type that extends a parameterization of `java.util.List`. */
4140
class ListType extends RefType {
4241
ListType() {

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,15 @@
99

1010
<<recommendation>
1111
<p>
12-
When writing MyBatis mapping statements, try to use the format "#{xxx}". If you have to use parameters
13-
such as "${xxx}", you must manually filter to prevent SQL injection attacks.
12+
When writing MyBatis mapping statements, try to use the syntax <code>#{xxx}</code>. If the syntax <code>${xxx}</code> must be used, any parameters included in it should be sanitized to prevent SQL injection attacks.
1413
</p>
1514
</recommendation>
1615

1716
<example>
1817
<p>
1918
The following examples show the bad situation and the good situation respectively. In <code>bad1</code>
2019
and <code>bad2</code> and <code>bad3</code> and <code>bad4</code> and <code >bad5</code>, the program
21-
${ xxx} are dynamic SQL statements, these five examples of SQL injection vulnerabilities. In <code>good1</code>,
20+
${xxx} are dynamic SQL statements, these five examples of SQL injection vulnerabilities. In <code>good1</code>,
2221
the program uses the ${xxx} dynamic feature SQL statement, but there are subtle restrictions on the data,
2322
and there is no SQL injection vulnerability.
2423
</p>

0 commit comments

Comments
 (0)