Skip to content

Commit 85ee4e6

Browse files
smowtonsmowton
authored
Merge pull request github#11578 from retanoj/MybatisSqli
Java: Add MyBatis Sql Injection no @param case
2 parents 280bb68 + 0d2474b commit 85ee4e6

File tree

6 files changed

+68
-22
lines changed

6 files changed

+68
-22
lines changed

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ bindingset[unsafeExpression]
8686
predicate isMybatisCollectionTypeSqlInjection(
8787
DataFlow::Node node, MethodAccess ma, string unsafeExpression
8888
) {
89-
not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and
89+
not unsafeExpression.regexpMatch("\\$\\{\\s*" + getAMybatisConfigurationVariableKey() + "\\s*\\}") and
9090
// The parameter type of the MyBatis method parameter is Map or List or Array.
9191
// SQL injection vulnerability caused by improper use of this parameter.
9292
// e.g.
@@ -120,26 +120,31 @@ bindingset[unsafeExpression]
120120
predicate isMybatisXmlOrAnnotationSqlInjection(
121121
DataFlow::Node node, MethodAccess ma, string unsafeExpression
122122
) {
123-
not unsafeExpression.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and
123+
not unsafeExpression.regexpMatch("\\$\\{\\s*" + getAMybatisConfigurationVariableKey() + "\\s*\\}") and
124124
(
125125
// The method parameters use `@Param` annotation. Due to improper use of this parameter, SQL injection vulnerabilities are caused.
126126
// e.g.
127127
//
128128
// ```java
129129
// @Select(select id,name from test order by ${orderby,jdbcType=VARCHAR})
130130
// void test(@Param("orderby") String name);
131+
//
132+
// @Select(select id,name from test where name = ${ user . name })
133+
// void test(@Param("user") User u);
131134
// ```
132135
exists(Annotation annotation |
133136
unsafeExpression
134-
.matches("${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
135-
"%}") and
137+
.regexpMatch("\\$\\{\\s*" +
138+
annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
139+
"\\b[^}]*\\}") and
136140
annotation.getType() instanceof TypeParam and
137141
ma.getAnArgument() = node.asExpr() and
138142
annotation.getTarget() =
139143
ma.getMethod().getParameter(node.asExpr().(Argument).getParameterPos())
140144
)
141145
or
142146
// MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n].
147+
// When compiled with '-parameters' compiler option, the parameter can be reflected in SQL statement as named in method signature.
143148
// e.g.
144149
//
145150
// ```java
@@ -149,9 +154,12 @@ predicate isMybatisXmlOrAnnotationSqlInjection(
149154
exists(int i |
150155
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
151156
(
152-
unsafeExpression.matches("${param" + (i + 1) + "%}")
157+
unsafeExpression.regexpMatch("\\$\\{\\s*param" + (i + 1) + "\\b[^}]*\\}")
158+
or
159+
unsafeExpression.regexpMatch("\\$\\{\\s*arg" + i + "\\b[^}]*\\}")
153160
or
154-
unsafeExpression.matches("${arg" + i + "%}")
161+
unsafeExpression
162+
.regexpMatch("\\$\\{\\s*" + ma.getMethod().getParameter(i).getName() + "\\b[^}]*\\}")
155163
) and
156164
ma.getArgument(i) = node.asExpr()
157165
)
@@ -166,7 +174,7 @@ predicate isMybatisXmlOrAnnotationSqlInjection(
166174
exists(int i, RefType t |
167175
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
168176
ma.getMethod().getParameterType(i).getName() = t.getName() and
169-
unsafeExpression.matches("${" + t.getAField().getName() + "%}") and
177+
unsafeExpression.regexpMatch("\\$\\{\\s*" + t.getAField().getName() + "\\b[^}]*\\}") and
170178
ma.getArgument(i) = node.asExpr()
171179
)
172180
or

‎java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,45 @@
11
edges
22
| MybatisSqlInjection.java:62:19:62:43 | name : String | MybatisSqlInjection.java:63:35:63:38 | name : String |
33
| MybatisSqlInjection.java:63:35:63:38 | name : String | MybatisSqlInjectionService.java:48:19:48:29 | name : String |
4-
| MybatisSqlInjection.java:94:20:94:44 | name : String | MybatisSqlInjection.java:95:36:95:39 | name : String |
5-
| MybatisSqlInjection.java:95:36:95:39 | name : String | MybatisSqlInjectionService.java:76:20:76:30 | name : String |
6-
| MybatisSqlInjection.java:99:20:99:43 | age : String | MybatisSqlInjection.java:100:36:100:38 | age : String |
7-
| MybatisSqlInjection.java:100:36:100:38 | age : String | MybatisSqlInjectionService.java:80:20:80:29 | age : String |
4+
| MybatisSqlInjection.java:67:46:67:70 | name : String | MybatisSqlInjection.java:68:40:68:43 | name : String |
5+
| MybatisSqlInjection.java:68:40:68:43 | name : String | MybatisSqlInjectionService.java:54:32:54:42 | name : String |
6+
| MybatisSqlInjection.java:99:20:99:44 | name : String | MybatisSqlInjection.java:100:36:100:39 | name : String |
7+
| MybatisSqlInjection.java:100:36:100:39 | name : String | MybatisSqlInjectionService.java:80:20:80:30 | name : String |
8+
| MybatisSqlInjection.java:104:20:104:43 | age : String | MybatisSqlInjection.java:105:36:105:38 | age : String |
9+
| MybatisSqlInjection.java:105:36:105:38 | age : String | MybatisSqlInjectionService.java:84:20:84:29 | age : String |
10+
| MybatisSqlInjection.java:109:46:109:70 | name : String | MybatisSqlInjection.java:110:40:110:43 | name : String |
11+
| MybatisSqlInjection.java:110:40:110:43 | name : String | MybatisSqlInjectionService.java:88:32:88:42 | name : String |
812
| MybatisSqlInjectionService.java:48:19:48:29 | name : String | MybatisSqlInjectionService.java:50:23:50:26 | name : String |
913
| MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [<map.value>] : String | MybatisSqlInjectionService.java:51:27:51:33 | hashMap |
1014
| MybatisSqlInjectionService.java:50:23:50:26 | name : String | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [<map.value>] : String |
11-
| MybatisSqlInjectionService.java:76:20:76:30 | name : String | MybatisSqlInjectionService.java:77:28:77:31 | name |
12-
| MybatisSqlInjectionService.java:80:20:80:29 | age : String | MybatisSqlInjectionService.java:81:28:81:30 | age |
15+
| MybatisSqlInjectionService.java:54:32:54:42 | name : String | MybatisSqlInjectionService.java:55:32:55:35 | name |
16+
| MybatisSqlInjectionService.java:80:20:80:30 | name : String | MybatisSqlInjectionService.java:81:28:81:31 | name |
17+
| MybatisSqlInjectionService.java:84:20:84:29 | age : String | MybatisSqlInjectionService.java:85:28:85:30 | age |
18+
| MybatisSqlInjectionService.java:88:32:88:42 | name : String | MybatisSqlInjectionService.java:89:32:89:35 | name |
1319
nodes
1420
| MybatisSqlInjection.java:62:19:62:43 | name : String | semmle.label | name : String |
1521
| MybatisSqlInjection.java:63:35:63:38 | name : String | semmle.label | name : String |
16-
| MybatisSqlInjection.java:94:20:94:44 | name : String | semmle.label | name : String |
17-
| MybatisSqlInjection.java:95:36:95:39 | name : String | semmle.label | name : String |
18-
| MybatisSqlInjection.java:99:20:99:43 | age : String | semmle.label | age : String |
19-
| MybatisSqlInjection.java:100:36:100:38 | age : String | semmle.label | age : String |
22+
| MybatisSqlInjection.java:67:46:67:70 | name : String | semmle.label | name : String |
23+
| MybatisSqlInjection.java:68:40:68:43 | name : String | semmle.label | name : String |
24+
| MybatisSqlInjection.java:99:20:99:44 | name : String | semmle.label | name : String |
25+
| MybatisSqlInjection.java:100:36:100:39 | name : String | semmle.label | name : String |
26+
| MybatisSqlInjection.java:104:20:104:43 | age : String | semmle.label | age : String |
27+
| MybatisSqlInjection.java:105:36:105:38 | age : String | semmle.label | age : String |
28+
| MybatisSqlInjection.java:109:46:109:70 | name : String | semmle.label | name : String |
29+
| MybatisSqlInjection.java:110:40:110:43 | name : String | semmle.label | name : String |
2030
| MybatisSqlInjectionService.java:48:19:48:29 | name : String | semmle.label | name : String |
2131
| MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [<map.value>] : String | semmle.label | hashMap [post update] [<map.value>] : String |
2232
| MybatisSqlInjectionService.java:50:23:50:26 | name : String | semmle.label | name : String |
2333
| MybatisSqlInjectionService.java:51:27:51:33 | hashMap | semmle.label | hashMap |
24-
| MybatisSqlInjectionService.java:76:20:76:30 | name : String | semmle.label | name : String |
25-
| MybatisSqlInjectionService.java:77:28:77:31 | name | semmle.label | name |
26-
| MybatisSqlInjectionService.java:80:20:80:29 | age : String | semmle.label | age : String |
27-
| MybatisSqlInjectionService.java:81:28:81:30 | age | semmle.label | age |
34+
| MybatisSqlInjectionService.java:54:32:54:42 | name : String | semmle.label | name : String |
35+
| MybatisSqlInjectionService.java:55:32:55:35 | name | semmle.label | name |
36+
| MybatisSqlInjectionService.java:80:20:80:30 | name : String | semmle.label | name : String |
37+
| MybatisSqlInjectionService.java:81:28:81:31 | name | semmle.label | name |
38+
| MybatisSqlInjectionService.java:84:20:84:29 | age : String | semmle.label | age : String |
39+
| MybatisSqlInjectionService.java:85:28:85:30 | age | semmle.label | age |
40+
| MybatisSqlInjectionService.java:88:32:88:42 | name : String | semmle.label | name : String |
41+
| MybatisSqlInjectionService.java:89:32:89:35 | name | semmle.label | name |
2842
subpaths
2943
#select
3044
| MybatisSqlInjectionService.java:51:27:51:33 | hashMap | MybatisSqlInjection.java:62:19:62:43 | name : String | MybatisSqlInjectionService.java:51:27:51:33 | hashMap | MyBatis annotation SQL injection might include code from $@ to $@. | MybatisSqlInjection.java:62:19:62:43 | name | this user input | SqlInjectionMapper.java:33:2:33:54 | Select | this SQL operation |
45+
| MybatisSqlInjectionService.java:55:32:55:35 | name | MybatisSqlInjection.java:67:46:67:70 | name : String | MybatisSqlInjectionService.java:55:32:55:35 | name | MyBatis annotation SQL injection might include code from $@ to $@. | MybatisSqlInjection.java:67:46:67:70 | name | this user input | SqlInjectionMapper.java:36:2:36:72 | Select | this SQL operation |

‎java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ public void bad9(@RequestParam String name) {
6363
mybatisSqlInjectionService.bad9(name);
6464
}
6565

66+
@GetMapping(value = "msi10")
67+
public void bad10(@RequestParam Integer id, @RequestParam String name) {
68+
mybatisSqlInjectionService.bad10(id, name);
69+
}
70+
6671
@GetMapping(value = "good1")
6772
public List<Test> good1(Integer id) {
6873
List<Test> result = mybatisSqlInjectionService.good1(id);
@@ -99,4 +104,9 @@ public void good2(@RequestParam String name, @RequestParam Integer age) {
99104
public void good3(@RequestParam String age) {
100105
mybatisSqlInjectionService.good3(age);
101106
}
107+
108+
@GetMapping(value = "good4")
109+
public void good4(@RequestParam Integer id, @RequestParam String name) {
110+
mybatisSqlInjectionService.good4(id, name);
111+
}
102112
}

‎java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ public void bad9(String name) {
5151
sqlInjectionMapper.bad9(hashMap);
5252
}
5353

54+
public void bad10(Integer id, String name) {
55+
sqlInjectionMapper.bad10(id, name);
56+
}
57+
5458
public List<Test> good1(Integer id) {
5559
List<Test> result = sqlInjectionMapper.good1(id);
5660
return result;
@@ -80,4 +84,8 @@ public void good2(String name, Integer age){
8084
public void good3(String age){
8185
sqlInjectionMapper.good3(age);
8286
}
87+
88+
public void good4(Integer id, String name) {
89+
sqlInjectionMapper.good4(id, name);
90+
}
8391
}

‎java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ public interface SqlInjectionMapper {
3333
@Select({"select * from test", "where id = ${name}"})
3434
public Test bad9(HashMap<String, Object> map);
3535

36+
@Select({"select * from test where id = #{id} and name = '${ name }'"})
37+
String bad10(Integer id, String name);
38+
3639
List<Test> good1(Integer id);
3740

3841
//using providers
@@ -66,4 +69,6 @@ public interface SqlInjectionMapper {
6669
@Select("select * from user_info where age = #{age}")
6770
String good3(@Param("age") String age);
6871

72+
@Select({"select * from test where id = #{id} and name = #{name}"})
73+
String good4(Integer id, String name);
6974
}

‎java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
<sql id="Update_By_Example_Where_Clause">
1313
<where>
1414
<if test="test.name != null">
15-
and name = ${test.name,jdbcType=VARCHAR}
15+
and name = ${ test . name , jdbcType = VARCHAR }
1616
</if>
1717
<if test="test.id != null">
1818
and id = #{test.id}

0 commit comments

Comments
 (0)