From ce2ba21240e6690b3819178c8eccead8e12941b4 Mon Sep 17 00:00:00 2001 From: ka1n4t <574702476@qq.com> Date: Tue, 22 Nov 2022 18:32:14 +0800 Subject: [PATCH 1/5] Add binding between annotation and sink-param --- .../src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll index 3351af22a25..8399efcc229 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll @@ -134,7 +134,8 @@ predicate isMybatisXmlOrAnnotationSqlInjection( .matches("${" + annotation.getValue("value").(CompileTimeConstantExpr).getStringValue() + "%}") and annotation.getType() instanceof TypeParam and - ma.getAnArgument() = node.asExpr() + ma.getAnArgument() = node.asExpr() and + annotation.getTarget() = ma.getMethod().getParameter(node.asExpr().getIndex()) ) or // MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n]. From d113fb23c839723e96709db42b071b7f90c484e4 Mon Sep 17 00:00:00 2001 From: ka1n4t <574702476@qq.com> Date: Wed, 23 Nov 2022 11:05:58 +0800 Subject: [PATCH 2/5] Add test case for PR-11368 --- .../MyBatisAnnotationSqlInjection.expected | 14 +++++++++++ .../CWE-089/src/main/MybatisSqlInjection.java | 12 +++++++++- .../src/main/MybatisSqlInjectionService.java | 8 +++++++ .../CWE-089/src/main/SqlInjectionMapper.java | 23 ++++++++++++------- 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected index dfc923f9b58..481c8307ac3 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected @@ -1,16 +1,30 @@ edges | MybatisSqlInjection.java:62:19:62:43 | name : String | MybatisSqlInjection.java:63:35:63:38 | name : String | | MybatisSqlInjection.java:63:35:63:38 | name : String | MybatisSqlInjectionService.java:48:19:48:29 | name : String | +| MybatisSqlInjection.java:94:21:94:45 | name : String | MybatisSqlInjection.java:95:37:95:40 | name : String | +| MybatisSqlInjection.java:95:37:95:40 | name : String | MybatisSqlInjectionService.java:76:21:76:31 | name : String | +| MybatisSqlInjection.java:99:21:99:44 | age : String | MybatisSqlInjection.java:100:37:100:39 | age : String | +| MybatisSqlInjection.java:100:37:100:39 | age : String | MybatisSqlInjectionService.java:80:21:80:30 | age : String | | MybatisSqlInjectionService.java:48:19:48:29 | name : String | MybatisSqlInjectionService.java:50:23:50:26 | name : String | | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | MybatisSqlInjectionService.java:51:27:51:33 | hashMap | | MybatisSqlInjectionService.java:50:23:50:26 | name : String | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | +| MybatisSqlInjectionService.java:76:21:76:31 | name : String | MybatisSqlInjectionService.java:77:29:77:32 | name | +| MybatisSqlInjectionService.java:80:21:80:30 | age : String | MybatisSqlInjectionService.java:81:29:81:31 | age | nodes | MybatisSqlInjection.java:62:19:62:43 | name : String | semmle.label | name : String | | MybatisSqlInjection.java:63:35:63:38 | name : String | semmle.label | name : String | +| MybatisSqlInjection.java:94:21:94:45 | name : String | semmle.label | name : String | +| MybatisSqlInjection.java:95:37:95:40 | name : String | semmle.label | name : String | +| MybatisSqlInjection.java:99:21:99:44 | age : String | semmle.label | age : String | +| MybatisSqlInjection.java:100:37:100:39 | age : String | semmle.label | age : String | | MybatisSqlInjectionService.java:48:19:48:29 | name : String | semmle.label | name : String | | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | semmle.label | hashMap [post update] [] : String | | MybatisSqlInjectionService.java:50:23:50:26 | name : String | semmle.label | name : String | | MybatisSqlInjectionService.java:51:27:51:33 | hashMap | semmle.label | hashMap | +| MybatisSqlInjectionService.java:76:21:76:31 | name : String | semmle.label | name : String | +| MybatisSqlInjectionService.java:77:29:77:32 | name | semmle.label | name | +| MybatisSqlInjectionService.java:80:21:80:30 | age : String | semmle.label | age : String | +| MybatisSqlInjectionService.java:81:29:81:31 | age | semmle.label | age | subpaths #select | 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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java index 624f27ad81d..5aa6876e00c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java @@ -79,7 +79,7 @@ public class MybatisSqlInjection { public void badDelete(@RequestParam String name) { mybatisSqlInjectionService.badDelete(name); } - + @GetMapping(value = "badUpdate") public void badUpdate(@RequestParam String name) { mybatisSqlInjectionService.badUpdate(name); @@ -89,4 +89,14 @@ public class MybatisSqlInjection { public void badInsert(@RequestParam String name) { mybatisSqlInjectionService.badInsert(name); } + + @GetMapping(value = "kkbad1") + public void kkbad1(@RequestParam String name, @RequestParam Integer age) { + mybatisSqlInjectionService.kkbad1(name, age); + } + + @GetMapping(value = "kkbad2") + public void kkbad2(@RequestParam String age) { + mybatisSqlInjectionService.kkbad2(age); + } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java index 89dbd599d71..28b9bebc1f4 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java @@ -72,4 +72,12 @@ public class MybatisSqlInjectionService { public void badInsert(String input) { sqlInjectionMapper.badInsert(input); } + + public void kkbad1(String name, Integer age){ + sqlInjectionMapper.kkbad1(name, age); + } + + public void kkbad2(String age){ + sqlInjectionMapper.kkbad2(age); + } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java index 5b159817297..50801558eb8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java @@ -37,26 +37,33 @@ public interface SqlInjectionMapper { //using providers @SelectProvider( - type = MyBatisProvider.class, - method = "badSelect" + type = MyBatisProvider.class, + method = "badSelect" ) String badSelect(String input); @DeleteProvider( - type = MyBatisProvider.class, - method = "badDelete" + type = MyBatisProvider.class, + method = "badDelete" ) void badDelete(String input); @UpdateProvider( - type = MyBatisProvider.class, - method = "badUpdate" + type = MyBatisProvider.class, + method = "badUpdate" ) void badUpdate(String input); @InsertProvider( - type = MyBatisProvider.class, - method = "badInsert" + type = MyBatisProvider.class, + method = "badInsert" ) void badInsert(String input); + + @Select("select * from user_info where name = #{name} and age = ${age}") + String kkbad1(@Param("name") String name, Integer age); + + @Select("select * from user_info where age = #{age}") + String kkbad2(@Param("age") String age); + } From 443d0f50c1354951c46ef5cfb1cba70150d3e682 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 24 Nov 2022 11:10:07 +0100 Subject: [PATCH 3/5] Apply suggestions from code review --- .../Security/CWE/CWE-089/MyBatisCommonLib.qll | 2 +- .../CWE-089/src/main/MybatisSqlInjection.java | 12 ++++++------ .../CWE-089/src/main/MybatisSqlInjectionService.java | 8 ++++---- .../CWE-089/src/main/SqlInjectionMapper.java | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll index 8399efcc229..4d3d77d8b65 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll @@ -135,7 +135,7 @@ predicate isMybatisXmlOrAnnotationSqlInjection( "%}") and annotation.getType() instanceof TypeParam and ma.getAnArgument() = node.asExpr() and - annotation.getTarget() = ma.getMethod().getParameter(node.asExpr().getIndex()) + annotation.getTarget() = ma.getMethod().getParameter(node.asExpr().(Argument).getParameterPos()) ) or // MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n]. diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java index 5aa6876e00c..a751d2ebb1c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjection.java @@ -90,13 +90,13 @@ public class MybatisSqlInjection { mybatisSqlInjectionService.badInsert(name); } - @GetMapping(value = "kkbad1") - public void kkbad1(@RequestParam String name, @RequestParam Integer age) { - mybatisSqlInjectionService.kkbad1(name, age); + @GetMapping(value = "good2") + public void good2(@RequestParam String name, @RequestParam Integer age) { + mybatisSqlInjectionService.good2(name, age); } - @GetMapping(value = "kkbad2") - public void kkbad2(@RequestParam String age) { - mybatisSqlInjectionService.kkbad2(age); + @GetMapping(value = "good3") + public void good3(@RequestParam String age) { + mybatisSqlInjectionService.good3(age); } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java index 28b9bebc1f4..c8e1ce9c3cb 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MybatisSqlInjectionService.java @@ -73,11 +73,11 @@ public class MybatisSqlInjectionService { sqlInjectionMapper.badInsert(input); } - public void kkbad1(String name, Integer age){ - sqlInjectionMapper.kkbad1(name, age); + public void good2(String name, Integer age){ + sqlInjectionMapper.good2(name, age); } - public void kkbad2(String age){ - sqlInjectionMapper.kkbad2(age); + public void good3(String age){ + sqlInjectionMapper.good3(age); } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java index 50801558eb8..a39b26a3aea 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/SqlInjectionMapper.java @@ -61,9 +61,9 @@ public interface SqlInjectionMapper { void badInsert(String input); @Select("select * from user_info where name = #{name} and age = ${age}") - String kkbad1(@Param("name") String name, Integer age); + String good2(@Param("name") String name, Integer age); @Select("select * from user_info where age = #{age}") - String kkbad2(@Param("age") String age); + String good3(@Param("age") String age); } From 17218fa663373a2d327309204e765d2e10cd1f0a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 24 Nov 2022 11:14:16 +0100 Subject: [PATCH 4/5] Formatting --- .../src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll index 4d3d77d8b65..0d13340b55d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-089/MyBatisCommonLib.qll @@ -135,7 +135,8 @@ predicate isMybatisXmlOrAnnotationSqlInjection( "%}") and annotation.getType() instanceof TypeParam and ma.getAnArgument() = node.asExpr() and - annotation.getTarget() = ma.getMethod().getParameter(node.asExpr().(Argument).getParameterPos()) + annotation.getTarget() = + ma.getMethod().getParameter(node.asExpr().(Argument).getParameterPos()) ) or // MyBatis default parameter sql injection vulnerabilities.the default parameter form of the method is arg[0...n] or param[1...n]. From 4bbc1dc7346507147e77eee576f458c6e8ca29bd Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 24 Nov 2022 12:34:48 +0100 Subject: [PATCH 5/5] Update test expectations --- .../MyBatisAnnotationSqlInjection.expected | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected index 481c8307ac3..1aedc93eb9a 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-089/src/main/MyBatisAnnotationSqlInjection.expected @@ -1,30 +1,30 @@ edges | MybatisSqlInjection.java:62:19:62:43 | name : String | MybatisSqlInjection.java:63:35:63:38 | name : String | | MybatisSqlInjection.java:63:35:63:38 | name : String | MybatisSqlInjectionService.java:48:19:48:29 | name : String | -| MybatisSqlInjection.java:94:21:94:45 | name : String | MybatisSqlInjection.java:95:37:95:40 | name : String | -| MybatisSqlInjection.java:95:37:95:40 | name : String | MybatisSqlInjectionService.java:76:21:76:31 | name : String | -| MybatisSqlInjection.java:99:21:99:44 | age : String | MybatisSqlInjection.java:100:37:100:39 | age : String | -| MybatisSqlInjection.java:100:37:100:39 | age : String | MybatisSqlInjectionService.java:80:21:80:30 | age : String | +| MybatisSqlInjection.java:94:20:94:44 | name : String | MybatisSqlInjection.java:95:36:95:39 | name : String | +| MybatisSqlInjection.java:95:36:95:39 | name : String | MybatisSqlInjectionService.java:76:20:76:30 | name : String | +| MybatisSqlInjection.java:99:20:99:43 | age : String | MybatisSqlInjection.java:100:36:100:38 | age : String | +| MybatisSqlInjection.java:100:36:100:38 | age : String | MybatisSqlInjectionService.java:80:20:80:29 | age : String | | MybatisSqlInjectionService.java:48:19:48:29 | name : String | MybatisSqlInjectionService.java:50:23:50:26 | name : String | | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | MybatisSqlInjectionService.java:51:27:51:33 | hashMap | | MybatisSqlInjectionService.java:50:23:50:26 | name : String | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | -| MybatisSqlInjectionService.java:76:21:76:31 | name : String | MybatisSqlInjectionService.java:77:29:77:32 | name | -| MybatisSqlInjectionService.java:80:21:80:30 | age : String | MybatisSqlInjectionService.java:81:29:81:31 | age | +| MybatisSqlInjectionService.java:76:20:76:30 | name : String | MybatisSqlInjectionService.java:77:28:77:31 | name | +| MybatisSqlInjectionService.java:80:20:80:29 | age : String | MybatisSqlInjectionService.java:81:28:81:30 | age | nodes | MybatisSqlInjection.java:62:19:62:43 | name : String | semmle.label | name : String | | MybatisSqlInjection.java:63:35:63:38 | name : String | semmle.label | name : String | -| MybatisSqlInjection.java:94:21:94:45 | name : String | semmle.label | name : String | -| MybatisSqlInjection.java:95:37:95:40 | name : String | semmle.label | name : String | -| MybatisSqlInjection.java:99:21:99:44 | age : String | semmle.label | age : String | -| MybatisSqlInjection.java:100:37:100:39 | age : String | semmle.label | age : String | +| MybatisSqlInjection.java:94:20:94:44 | name : String | semmle.label | name : String | +| MybatisSqlInjection.java:95:36:95:39 | name : String | semmle.label | name : String | +| MybatisSqlInjection.java:99:20:99:43 | age : String | semmle.label | age : String | +| MybatisSqlInjection.java:100:36:100:38 | age : String | semmle.label | age : String | | MybatisSqlInjectionService.java:48:19:48:29 | name : String | semmle.label | name : String | | MybatisSqlInjectionService.java:50:3:50:9 | hashMap [post update] [] : String | semmle.label | hashMap [post update] [] : String | | MybatisSqlInjectionService.java:50:23:50:26 | name : String | semmle.label | name : String | | MybatisSqlInjectionService.java:51:27:51:33 | hashMap | semmle.label | hashMap | -| MybatisSqlInjectionService.java:76:21:76:31 | name : String | semmle.label | name : String | -| MybatisSqlInjectionService.java:77:29:77:32 | name | semmle.label | name | -| MybatisSqlInjectionService.java:80:21:80:30 | age : String | semmle.label | age : String | -| MybatisSqlInjectionService.java:81:29:81:31 | age | semmle.label | age | +| MybatisSqlInjectionService.java:76:20:76:30 | name : String | semmle.label | name : String | +| MybatisSqlInjectionService.java:77:28:77:31 | name | semmle.label | name | +| MybatisSqlInjectionService.java:80:20:80:29 | age : String | semmle.label | age : String | +| MybatisSqlInjectionService.java:81:28:81:30 | age | semmle.label | age | subpaths #select | 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 |