Skip to content

Commit 31400df

Browse files
haby0haby0
committed
Modify sink and improve SQL injection detection
1 parent 69690a2 commit 31400df

File tree

8 files changed

+235
-140
lines changed

8 files changed

+235
-140
lines changed

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ private class MyBatisMapperXmlSqlInjectionConfiguration extends TaintTracking::C
2222
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2323

2424
override predicate isSink(DataFlow::Node sink) {
25-
sink instanceof MyBatisMapperXmlSqlInjectionSink
25+
sink instanceof MyBatisMapperMethodCallAnArgument
2626
}
2727

2828
override predicate isSanitizer(DataFlow::Node node) {
@@ -33,7 +33,11 @@ private class MyBatisMapperXmlSqlInjectionConfiguration extends TaintTracking::C
3333
}
3434

3535
from
36-
MyBatisMapperXmlSqlInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
37-
where cfg.hasFlowPath(source, sink)
38-
select sink.getNode(), source, sink, "MyBatis Mapper XML sql injection might include code from $@.",
39-
source.getNode(), "this user input"
36+
MyBatisMapperXmlSqlInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink,
37+
XMLElement xmle
38+
where
39+
cfg.hasFlowPath(source, sink) and
40+
isSqlInjection(sink.getNode(), xmle)
41+
select sink.getNode(), source, sink,
42+
"MyBatis Mapper XML sql injection might include code from $@ to $@.", source.getNode(),
43+
"this user input", xmle, "this sql operation"

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

Lines changed: 88 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -7,95 +7,100 @@ private class TypeParam extends Interface {
77
TypeParam() { this.hasQualifiedName("org.apache.ibatis.annotations", "Param") }
88
}
99

10-
/** A sink for MyBatis Mapper XML file sql injection vulnerabilities. */
11-
abstract class MyBatisMapperXmlSqlInjectionSink extends DataFlow::Node { }
12-
13-
/**
14-
* A sink for MyBatis Mapper method parameter name sql injection vulnerabilities.
15-
*
16-
* e.g. MyBatis Mapper method: `void test(String name);` and MyBatis Mapper XML file:`select id,name from test where name like '%${name}%'`
17-
*/
18-
class MyBatisMapperParameterNameSqlInjectionSink extends MyBatisMapperXmlSqlInjectionSink {
19-
MyBatisMapperParameterNameSqlInjectionSink() {
20-
exists(
21-
MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess ma, int i, Method m,
22-
Expr arg, string sql
23-
|
24-
m = ma.getMethod() and arg = ma.getArgument(i)
25-
|
26-
arg = this.asExpr() and
27-
(
28-
mbmxe.getAChild*().getTextValue().trim() = sql
29-
or
30-
mbmxe.getInclude().getRefid() = mbms.getId() and
31-
mbms.getAChild*().getTextValue().trim() = sql
32-
) and
33-
not m.getParameter(i).hasAnnotation() and
34-
sql.matches("%${" + m.getParameter(i).getName() + "%") and
35-
mbmxe.getId() = ma.getMethod().getName() and
36-
ma.getMethod().getDeclaringType() =
37-
mbmxe.getParent().(MyBatisMapperXMLElement).getNamespaceRefType()
38-
)
10+
/** A reference type that extends a parameterization of `java.util.List`. */
11+
private class ListType extends RefType {
12+
ListType() {
13+
this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List")
3914
}
4015
}
4116

42-
/**
43-
* A sink for MyBatis Mapper method Param Annotation sql injection vulnerabilities.
44-
*
45-
* e.g. MyBatis Mapper method: `void test(@Param("orderby") String name);` and MyBatis Mapper XML file:`select id,name from test order by ${orderby,jdbcType=VARCHAR}`
46-
*/
47-
class MyBatisMapperParamAnnotationSqlInjectionSink extends MyBatisMapperXmlSqlInjectionSink {
48-
MyBatisMapperParamAnnotationSqlInjectionSink() {
49-
exists(
50-
MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess ma, int i, Method m,
51-
Expr arg, Annotation a, string sql
52-
|
53-
m = ma.getMethod() and arg = ma.getArgument(i)
17+
/** A sink for MyBatis Mapper method call an argument. */
18+
class MyBatisMapperMethodCallAnArgument extends DataFlow::Node {
19+
MyBatisMapperMethodCallAnArgument() {
20+
exists(MyBatisMapperSqlOperation mbmxe, MethodAccess mc |
21+
mbmxe.getMapperMethod() = mc.getMethod()
5422
|
55-
arg = this.asExpr() and
56-
(
57-
mbmxe.getAChild*().getTextValue().trim() = sql
58-
or
59-
mbmxe.getInclude().getRefid() = mbms.getId() and
60-
mbms.getAChild*().getTextValue().trim() = sql
61-
) and
62-
m.getParameter(i).hasAnnotation() and
63-
m.getParameter(i).getAnAnnotation() = a and
64-
a.getType() instanceof TypeParam and
65-
sql.matches("%${" + a.getValue("value").(CompileTimeConstantExpr).getStringValue() + "%") and
66-
mbmxe.getId() = ma.getMethod().getName() and
67-
ma.getMethod().getDeclaringType() =
68-
mbmxe.getParent().(MyBatisMapperXMLElement).getNamespaceRefType()
23+
mc.getAnArgument() = this.asExpr()
6924
)
7025
}
7126
}
7227

73-
/**
74-
* A sink for MyBatis Mapper method Class Field sql injection vulnerabilities.
75-
*
76-
* e.g. MyBatis Mapper method: `void test(Test test);` and MyBatis Mapper XML file:`select id,name from test order by ${name,jdbcType=VARCHAR}`
77-
*/
78-
class MyBatisMapperClassFieldSqlInjectionSink extends MyBatisMapperXmlSqlInjectionSink {
79-
MyBatisMapperClassFieldSqlInjectionSink() {
80-
exists(
81-
MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess ma, int i, Method m,
82-
Expr arg, string sql, Class c
83-
|
84-
m = ma.getMethod() and arg = ma.getArgument(i)
85-
|
86-
arg = this.asExpr() and
87-
(
88-
mbmxe.getAChild*().getTextValue().trim() = sql
89-
or
90-
mbmxe.getInclude().getRefid() = mbms.getId() and
91-
mbms.getAChild*().getTextValue().trim() = sql
92-
) and
93-
not m.getParameter(i).hasAnnotation() and
94-
m.getParameterType(i).getName() = c.getName() and
95-
sql.matches("%${" + c.getAField().getName() + "%") and
96-
mbmxe.getId() = ma.getMethod().getName() and
97-
ma.getMethod().getDeclaringType() =
98-
mbmxe.getParent().(MyBatisMapperXMLElement).getNamespaceRefType()
99-
)
100-
}
28+
predicate isSqlInjection(DataFlow::Node node, XMLElement xmle) {
29+
// MyBatis Mapper method parameter name sql injection vulnerabilities.
30+
// e.g. MyBatis Mapper method: `void test(String name);` and MyBatis Mapper XML file:`select id,name from test where name like '%${name}%'`
31+
exists(MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess mc, int i |
32+
mbmxe.getMapperMethod() = mc.getMethod()
33+
|
34+
(
35+
mbmxe.getAChild*() = xmle
36+
or
37+
mbmxe.getInclude().getRefid() = mbms.getId() and
38+
mbms.getAChild*() = xmle
39+
) and
40+
not mc.getMethod().getParameter(i).hasAnnotation() and
41+
xmle.getTextValue().trim().matches("%${" + mc.getMethod().getParameter(i).getName() + "%") and
42+
mc.getArgument(i) = node.asExpr()
43+
)
44+
or
45+
// MyBatis Mapper method Param Annotation sql injection vulnerabilities.
46+
// e.g. MyBatis Mapper method: `void test(@Param("orderby") String name);` and MyBatis Mapper XML file:`select id,name from test order by ${orderby,jdbcType=VARCHAR}`
47+
exists(
48+
MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess mc, int i,
49+
Annotation annotation
50+
|
51+
mbmxe.getMapperMethod() = mc.getMethod()
52+
|
53+
(
54+
mbmxe.getAChild*() = xmle
55+
or
56+
mbmxe.getInclude().getRefid() = mbms.getId() and
57+
mbms.getAChild*() = xmle
58+
) and
59+
mc.getMethod().getParameter(i).hasAnnotation() and
60+
mc.getMethod().getParameter(i).getAnAnnotation() = annotation and
61+
annotation.getType() instanceof TypeParam and
62+
xmle.getTextValue()
63+
.trim()
64+
.matches("%${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() +
65+
"%") and
66+
mc.getArgument(i) = node.asExpr()
67+
)
68+
or
69+
// MyBatis Mapper method Class Field sql injection vulnerabilities.
70+
// e.g. MyBatis Mapper method: `void test(Test test);` and MyBatis Mapper XML file:`select id,name from test order by ${name,jdbcType=VARCHAR}`
71+
exists(MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess mc, int i, Class c |
72+
mbmxe.getMapperMethod() = mc.getMethod()
73+
|
74+
(
75+
mbmxe.getAChild*() = xmle
76+
or
77+
mbmxe.getInclude().getRefid() = mbms.getId() and
78+
mbms.getAChild*() = xmle
79+
) and
80+
not mc.getMethod().getParameter(i).hasAnnotation() and
81+
mc.getMethod().getParameterType(i).getName() = c.getName() and
82+
xmle.getTextValue().trim().matches("%${" + c.getAField().getName() + "%") and
83+
mc.getArgument(i) = node.asExpr()
84+
)
85+
or
86+
// The parameter type of MyBatis Mapper method is Map or List or Array, which may cause SQL injection vulnerability.
87+
// e.g. MyBatis Mapper method: `void test(Map<String, String> params);` and MyBatis Mapper XML file:`select id,name from test where name like '%${name}%'`
88+
exists(MyBatisMapperSqlOperation mbmxe, MyBatisMapperSql mbms, MethodAccess mc, int i |
89+
mbmxe.getMapperMethod() = mc.getMethod()
90+
|
91+
(
92+
mbmxe.getAChild*() = xmle
93+
or
94+
mbmxe.getInclude().getRefid() = mbms.getId() and
95+
mbms.getAChild*() = xmle
96+
) and
97+
not mc.getMethod().getParameter(i).hasAnnotation() and
98+
(
99+
mc.getMethod().getParameterType(i) instanceof MapType or
100+
mc.getMethod().getParameterType(i) instanceof ListType or
101+
mc.getMethod().getParameterType(i) instanceof Array
102+
) and
103+
xmle.getTextValue().trim().matches("%${%") and
104+
mc.getArgument(i) = node.asExpr()
105+
)
101106
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ abstract class MyBatisMapperSqlOperation extends MyBatisMapperXMLElement {
3737
* Gets the `<include>` element in a `MyBatisMapperSqlOperation`.
3838
*/
3939
MyBatisMapperInclude getInclude() { result = getAChild*() }
40+
41+
42+
Method getMapperMethod() {
43+
result.getName() = this.getId() and
44+
result.getDeclaringType() = this.getParent().(MyBatisMapperXMLElement).getNamespaceRefType()
45+
}
4046
}
4147

4248
/**

0 commit comments

Comments
 (0)