Skip to content

Commit 1d321c6

Browse files
haby0haby0
committed
Refactor isMybatisXmlOrAnnotationSqlInjection
1 parent daf6a4c commit 1d321c6

4 files changed

Lines changed: 115 additions & 138 deletions

File tree

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,12 @@ private class MyBatisAnnotationSqlInjectionConfiguration extends TaintTracking::
4444

4545
from
4646
MyBatisAnnotationSqlInjectionConfiguration cfg, DataFlow::PathNode source,
47-
DataFlow::PathNode sink, IbatisSqlOperationAnnotation isoa
47+
DataFlow::PathNode sink, IbatisSqlOperationAnnotation isoa, MethodAccess ma
4848
where
4949
cfg.hasFlowPath(source, sink) and
50-
isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), _, isoa)
50+
ma.getAnArgument() = sink.getNode().asExpr() and
51+
myBatisSqlOperationAnnotationFromMethod(ma.getMethod(), isoa) and
52+
isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, getAMybatisAnnotationSqlValue(isoa), _)
5153
select sink.getNode(), source, sink,
5254
"MyBatis annotation SQL injection might include code from $@ to $@.", source.getNode(),
5355
"this user input", isoa, "this SQL operation"

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

Lines changed: 101 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ private class PropertiesFlowConfig extends DataFlow2::Configuration {
3030
}
3131

3232
/** Get the key value of Mybatis Configuration Variable. */
33-
string getAnMybatisConfigurationVariableKey() {
33+
string getAMybatisConfigurationVariableKey() {
3434
exists(PropertiesFlowConfig conf, DataFlow::Node n |
3535
propertiesKey(n, result) and
3636
conf.hasFlowTo(n)
@@ -44,134 +44,124 @@ class ListType extends RefType {
4444
}
4545
}
4646

47-
/** Gets a call to the MyBatis mapper xml method. */
48-
MethodAccess getMyBatisMapperXmlMethodAccess(XMLElement xmle) {
49-
exists(MyBatisMapperSqlOperation mbmxe |
50-
mbmxe.getMapperMethod() = result.getMethod() and
51-
(
52-
mbmxe.getAChild*() = xmle
53-
or
54-
exists(MyBatisMapperSql mbms |
55-
mbmxe.getInclude().getRefid() = mbms.getId() and
56-
mbms.getAChild*() = xmle
57-
)
47+
/** Holds if the specified method uses MyBatis Mapper XMLElement. */
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
5855
)
5956
)
6057
}
6158

62-
/** Gets a call to the MyBatis SQL operation annotation method. */
63-
MethodAccess getMyBatisSqlOperationAnnotationMethodAccess(IbatisSqlOperationAnnotation isoa) {
59+
/** Holds if the specified method uses Ibatis Sql Operation Annotation. */
60+
predicate myBatisSqlOperationAnnotationFromMethod(Method method, IbatisSqlOperationAnnotation isoa) {
6461
exists(MyBatisSqlOperationAnnotationMethod msoam |
65-
msoam = result.getMethod() and
62+
msoam = method and
6663
msoam.getAnAnnotation() = isoa
6764
)
6865
}
6966

7067
/** Get the #{...} or ${...} parameters in the Mybatis mapper xml file. */
71-
private string getAnMybatisXmlSetValue(XMLElement xmle) {
72-
result = xmle.getTextValue().trim().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
68+
string getAMybatisXmlSetValue(XMLElement xmle) {
69+
result = xmle.getTextValue().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
7370
}
7471

7572
/** Get the #{...} or ${...} parameters in the Mybatis sql operation annotation value. */
76-
private string getAMybatisAnnotationSqlValue(IbatisSqlOperationAnnotation isoa) {
77-
result = isoa.getSqlValue().trim().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
73+
string getAMybatisAnnotationSqlValue(IbatisSqlOperationAnnotation isoa) {
74+
result = isoa.getSqlValue().regexpFind("(#|\\$)\\{[^\\}]*\\}", _, _)
7875
}
7976

8077
/** Holds if it is SQL injection of MyBatis xml or MyBatis annotation. */
78+
bindingset[setValue]
8179
predicate isMybatisXmlOrAnnotationSqlInjection(
82-
DataFlow::Node node, XMLElement xmle, IbatisSqlOperationAnnotation isoa
80+
//DataFlow::Node node, MyBatisMapperXMLElement xmle, IbatisSqlOperationAnnotation isoa
81+
DataFlow::Node node, MethodAccess ma, string setValue, MyBatisMapperXMLElement mmxe
8382
) {
84-
exists(MethodAccess ma, string setValue |
85-
(
86-
ma = getMyBatisMapperXmlMethodAccess(xmle) and
87-
setValue = getAnMybatisXmlSetValue(xmle)
88-
or
89-
ma = getMyBatisSqlOperationAnnotationMethodAccess(isoa) and
90-
setValue = getAMybatisAnnotationSqlValue(isoa)
91-
) and
92-
not setValue.regexpMatch("\\$\\{" + getAnMybatisConfigurationVariableKey() + "\\}") and
93-
(
94-
// The method parameters use `@Param` annotation. Due to improper use of this parameter, SQL injection vulnerabilities are caused.
95-
// e.g.
96-
//
97-
// ```java
98-
// @Select(select id,name from test order by ${orderby,jdbcType=VARCHAR})
99-
// void test(@Param("orderby") String name);
100-
// ```
101-
exists(int i, Annotation annotation |
102-
setValue
103-
.matches("%${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue()
104-
+ "%}") and
105-
annotation.getType() instanceof TypeParam and
106-
ma.getArgument(i) = node.asExpr()
107-
)
108-
or
109-
// MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n].
110-
// e.g.
111-
//
112-
// ```java
113-
// @Select(select id,name from test order by ${arg0,jdbcType=VARCHAR})
114-
// void test(String name);
115-
// ```
116-
exists(int i |
117-
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
118-
(
119-
setValue.matches("%${param" + (i + 1) + "%}")
120-
or
121-
setValue.matches("%${arg" + i + "%}")
122-
) and
123-
not setValue = "${" + getAnMybatisConfigurationVariableKey() + "}" and
124-
ma.getArgument(i) = node.asExpr()
125-
)
126-
or
127-
// SQL injection vulnerability caused by improper use of MyBatis instance class fields.
128-
// e.g.
129-
//
130-
// ```java
131-
// @Select(select id,name from test order by ${name,jdbcType=VARCHAR})
132-
// void test(Test test);
133-
// ```
134-
exists(int i, RefType t |
135-
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
136-
ma.getMethod().getParameterType(i).getName() = t.getName() and
137-
setValue.matches("%${" + t.getAField().getName() + "%}") and
138-
ma.getArgument(i) = node.asExpr()
139-
)
140-
or
141-
// The parameter type of the MyBatis method parameter is Map or List or Array.
142-
// SQL injection vulnerability caused by improper use of this parameter.
143-
// e.g.
144-
//
145-
// ```java
146-
// @Select(select id,name from test where name like '%${value}%')
147-
// Test test(Map map);
148-
// ```
149-
exists(int i, MyBatisMapperForeach mbmf |
150-
mbmf = xmle and
151-
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
152-
(
153-
ma.getMethod().getParameterType(i) instanceof MapType or
154-
ma.getMethod().getParameterType(i) instanceof ListType or
155-
ma.getMethod().getParameterType(i) instanceof Array
156-
) and
157-
setValue.matches("%${%}") and
158-
ma.getArgument(i) = node.asExpr()
159-
)
160-
or
161-
// This method has only one parameter and the parameter is not annotated with `@Param`.
162-
// Improper use of this parameter has a SQL injection vulnerability.
163-
// e.g.
164-
//
165-
// ```java
166-
// @Select(select id,name from test where name like '%${value}%')
167-
// Test test(String name);
168-
// ```
169-
exists(int i | i = 1 |
170-
ma.getMethod().getNumberOfParameters() = i and
171-
not ma.getMethod().getAParameter().getAnAnnotation().getType() instanceof TypeParam and
172-
setValue.matches("%${%}") and
173-
ma.getAnArgument() = node.asExpr()
174-
)
83+
not setValue.regexpMatch("\\$\\{" + getAMybatisConfigurationVariableKey() + "\\}") and
84+
(
85+
// The method parameters use `@Param` annotation. Due to improper use of this parameter, SQL injection vulnerabilities are caused.
86+
// e.g.
87+
//
88+
// ```java
89+
// @Select(select id,name from test order by ${orderby,jdbcType=VARCHAR})
90+
// void test(@Param("orderby") String name);
91+
// ```
92+
exists(int i, Annotation annotation |
93+
setValue
94+
.matches("${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
95+
"%}") and
96+
annotation.getType() instanceof TypeParam and
97+
ma.getArgument(i) = node.asExpr()
98+
)
99+
or
100+
// MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n].
101+
// e.g.
102+
//
103+
// ```java
104+
// @Select(select id,name from test order by ${arg0,jdbcType=VARCHAR})
105+
// void test(String name);
106+
// ```
107+
exists(int i |
108+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
109+
(
110+
setValue.matches("${param" + (i + 1) + "%}")
111+
or
112+
setValue.matches("${arg" + i + "%}")
113+
) and
114+
ma.getArgument(i) = node.asExpr()
115+
)
116+
or
117+
// SQL injection vulnerability caused by improper use of MyBatis instance class fields.
118+
// e.g.
119+
//
120+
// ```java
121+
// @Select(select id,name from test order by ${name,jdbcType=VARCHAR})
122+
// void test(Test test);
123+
// ```
124+
exists(int i, RefType t |
125+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
126+
ma.getMethod().getParameterType(i).getName() = t.getName() and
127+
setValue.matches("${" + t.getAField().getName() + "%}") and
128+
ma.getArgument(i) = node.asExpr()
129+
)
130+
or
131+
// The parameter type of the MyBatis method parameter is Map or List or Array.
132+
// SQL injection vulnerability caused by improper use of this parameter.
133+
// e.g.
134+
//
135+
// ```java
136+
// @Select(select id,name from test where name like '%${value}%')
137+
// Test test(Map map);
138+
// ```
139+
exists(int i, MyBatisMapperForeach mbmf |
140+
mbmf = mmxe and
141+
not ma.getMethod().getParameter(i).getAnAnnotation().getType() instanceof TypeParam and
142+
(
143+
ma.getMethod().getParameterType(i) instanceof MapType or
144+
ma.getMethod().getParameterType(i) instanceof ListType or
145+
ma.getMethod().getParameterType(i) instanceof Array
146+
) and
147+
setValue.matches("${%}") and
148+
ma.getArgument(i) = node.asExpr()
149+
)
150+
or
151+
// This method has only one parameter and the parameter is not annotated with `@Param`. The parameter can be named arbitrarily in the SQL statement.
152+
// If the number of method variables is greater than one, they cannot be named arbitrarily.
153+
// Improper use of this parameter has a SQL injection vulnerability.
154+
// e.g.
155+
//
156+
// ```java
157+
// @Select(select id,name from test where name like '%${value}%')
158+
// Test test(String name);
159+
// ```
160+
exists(int i | i = 1 |
161+
ma.getMethod().getNumberOfParameters() = i and
162+
not ma.getMethod().getAParameter().getAnAnnotation().getType() instanceof TypeParam and
163+
setValue.matches("${%}") and
164+
ma.getAnArgument() = node.asExpr()
175165
)
176166
)
177167
}

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@ private class MyBatisMapperXmlSqlInjectionConfiguration extends TaintTracking::C
4545

4646
from
4747
MyBatisMapperXmlSqlInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink,
48-
XMLElement xmle
48+
MyBatisMapperXMLElement mmxe, MethodAccess ma
4949
where
5050
cfg.hasFlowPath(source, sink) and
51-
isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), xmle, _)
51+
ma.getAnArgument() = sink.getNode().asExpr() and
52+
myBatisMapperXMLElementFromMethod(ma.getMethod(), mmxe) and
53+
isMybatisXmlOrAnnotationSqlInjection(sink.getNode(), ma, getAMybatisXmlSetValue(mmxe), mmxe)
5254
select sink.getNode(), source, sink,
5355
"MyBatis Mapper XML SQL injection might include code from $@ to $@.", source.getNode(),
54-
"this user input", xmle, "this SQL operation"
56+
"this user input", mmxe, "this SQL operation"

‎java/ql/src/semmle/code/xml/MyBatisMapperXML.qll‎

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ class MyBatisMapperXMLElement extends XMLElement {
3737
* An MyBatis Mapper sql operation element.
3838
*/
3939
abstract class MyBatisMapperSqlOperation extends MyBatisMapperXMLElement {
40-
abstract string getId();
40+
/**
41+
* Gets the value of the `id` attribute of MyBatis Mapper sql operation element.
42+
*/
43+
string getId() { result = this.getAttribute("id").getValue() }
4144

4245
/**
4346
* Gets the `<include>` element in a `MyBatisMapperSqlOperation`.
@@ -58,51 +61,31 @@ abstract class MyBatisMapperSqlOperation extends MyBatisMapperXMLElement {
5861
*/
5962
class MyBatisMapperInsert extends MyBatisMapperSqlOperation {
6063
MyBatisMapperInsert() { this.getName() = "insert" }
61-
62-
/**
63-
* Gets the value of the `id` attribute of this `<insert>`.
64-
*/
65-
override string getId() { result = this.getAttribute("id").getValue() }
6664
}
6765

6866
/**
6967
* A `<update>` element in a `MyBatisMapperSqlOperation`.
7068
*/
7169
class MyBatisMapperUpdate extends MyBatisMapperSqlOperation {
7270
MyBatisMapperUpdate() { this.getName() = "update" }
73-
74-
/**
75-
* Gets the value of the `id` attribute of this `<update>`.
76-
*/
77-
override string getId() { result = this.getAttribute("id").getValue() }
7871
}
7972

8073
/**
8174
* A `<delete>` element in a `MyBatisMapperSqlOperation`.
8275
*/
8376
class MyBatisMapperDelete extends MyBatisMapperSqlOperation {
8477
MyBatisMapperDelete() { this.getName() = "delete" }
85-
86-
/**
87-
* Gets the value of the `id` attribute of this `<delete>`.
88-
*/
89-
override string getId() { result = this.getAttribute("id").getValue() }
9078
}
9179

9280
/**
9381
* A `<select>` element in a `MyBatisMapperSqlOperation`.
9482
*/
9583
class MyBatisMapperSelect extends MyBatisMapperSqlOperation {
9684
MyBatisMapperSelect() { this.getName() = "select" }
97-
98-
/**
99-
* Gets the value of the `id` attribute of this `<select>`.
100-
*/
101-
override string getId() { result = this.getAttribute("id").getValue() }
10285
}
10386

10487
/**
105-
* A `<select>` element in a `MyBatisMapperXMLElement`.
88+
* A `<sql>` element in a `MyBatisMapperXMLElement`.
10689
*/
10790
class MyBatisMapperSql extends MyBatisMapperXMLElement {
10891
MyBatisMapperSql() { this.getName() = "sql" }

0 commit comments

Comments
 (0)