From df77d4914f3be81ce173d7dc7d1e1ec678cd2139 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 3 Dec 2024 16:31:52 -0500 Subject: [PATCH] Java: initial tests --- .../CsrfUnprotectedRequestTypeQuery.qll | 12 +- .../CWE/CWE-352/CsrfUnprotectedRequestType.ql | 9 +- .../CsrfUnprotectedRequestTypeTest.expected | 2 + .../CsrfUnprotectedRequestTypeTest.java | 158 ++++++++++++++++++ .../CWE-352/CsrfUnprotectedRequestTypeTest.ql | 20 +++ .../security/CWE-352/MyBatisMapper.java | 31 ++++ .../security/CWE-352/MyBatisMapper.xml | 35 ++++ .../security/CWE-352/MyBatisProvider.java | 24 +++ .../security/CWE-352/MyBatisService.java | 25 +++ .../test/query-tests/security/CWE-352/options | 2 +- 10 files changed, 310 insertions(+), 8 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java create mode 100644 java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-352/MyBatisMapper.java create mode 100644 java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml create mode 100644 java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java create mode 100644 java/ql/test/query-tests/security/CWE-352/MyBatisService.java diff --git a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll index 8c4cb3beb5d..b96a3c6f1a4 100644 --- a/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll @@ -114,5 +114,15 @@ module CallGraph { } } - query predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ } + predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ } +} + +import CallGraph + +/** Holds if `src` is an unprotected request handler that reaches a state-changing `sink`. */ +predicate unprotectedStateChange(PathNode src, PathNode sink, PathNode sinkPred) { + src.asMethod() instanceof CsrfUnprotectedMethod and + sink.asMethod() instanceof DatabaseUpdateMethod and + sinkPred.getASuccessor() = sink and + src.getASuccessor+() = sinkPred } diff --git a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql index dfe01d64efc..61612198c9e 100644 --- a/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql +++ b/java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql @@ -14,14 +14,11 @@ import java import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery -import CallGraph + +query predicate edges(PathNode pred, PathNode succ) { CallGraph::edges(pred, succ) } from PathNode source, PathNode reachable, PathNode callsReachable -where - source.asMethod() instanceof CsrfUnprotectedMethod and - reachable.asMethod() instanceof DatabaseUpdateMethod and - callsReachable.getASuccessor() = reachable and - source.getASuccessor+() = callsReachable +where unprotectedStateChange(source, reachable, callsReachable) select source.asMethod(), source, callsReachable, "Potential CSRF vulnerability due to using an HTTP request type which is not default-protected from CSRF for an apparent $@.", callsReachable, "state-changing action" diff --git a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.expected b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.expected new file mode 100644 index 00000000000..8ec8033d086 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java new file mode 100644 index 00000000000..c7d83bca441 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java @@ -0,0 +1,158 @@ +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import static org.springframework.web.bind.annotation.RequestMethod.*; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.beans.factory.annotation.Autowired; +import java.sql.DriverManager; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.SQLException; + +@Controller +public class CsrfUnprotectedRequestTypeTest { + + // Test Spring sources with `PreparedStatement.executeUpdate()` as a default database update method call + + // BAD: allows request type not default-protected from CSRF when updating a database + @RequestMapping("/") + public void bad1() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // BAD: uses GET request when updating a database + @RequestMapping(value = "", method = RequestMethod.GET) + public void bad2() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // BAD: uses GET request when updating a database + @GetMapping(value = "") + public void bad3() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // BAD: allows GET request when updating a database + @RequestMapping(value = "", method = { RequestMethod.GET, RequestMethod.POST }) + public void bad4() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // BAD: uses request type not default-protected from CSRF when updating a database + @RequestMapping(value = "", method = { GET, HEAD, OPTIONS, TRACE }) + public void bad5() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // GOOD: uses POST request when updating a database + @RequestMapping(value = "", method = RequestMethod.POST) + public void good1() { + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // GOOD: uses POST request when updating a database + @RequestMapping(value = "", method = POST) + public void good2() { + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // GOOD: uses POST request when updating a database + @PostMapping(value = "") + public void good3() { + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // GOOD: uses a request type that is default-protected from CSRF when updating a database + @RequestMapping(value = "", method = { POST, PUT, PATCH, DELETE }) + public void good4() { + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeUpdate(); // database update method call + } catch (SQLException e) { } + } + + // Test database update method calls other than `PreparedStatement.executeUpdate()` + + // BAD: allows request type not default-protected from CSRF when + // updating a database using `PreparedStatement.executeLargeUpdate()` + @RequestMapping("/") + public void bad6() { // $ hasCsrfUnprotectedRequestType + try { + String sql = "DELETE"; + Connection conn = DriverManager.getConnection("url"); + PreparedStatement ps = conn.prepareStatement(sql); + ps.executeLargeUpdate(); // database update method call + } catch (SQLException e) { } + } + + @Autowired + private MyBatisService myBatisService; + + // BAD: uses GET request when updating a database with MyBatis XML mapper method + @GetMapping(value = "") + public void bad7(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType + myBatisService.bad7(name); + } + + // BAD: uses GET request when updating a database with `@DeleteProvider` + @GetMapping(value = "badDelete") + public void badDelete(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType + myBatisService.badDelete(name); + } + + // BAD: uses GET request when updating a database with `@UpdateProvider` + @GetMapping(value = "badUpdate") + public void badUpdate(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType + myBatisService.badUpdate(name); + } + + // BAD: uses GET request when updating a database with `@InsertProvider` + @GetMapping(value = "badInsert") + public void badInsert(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType + myBatisService.badInsert(name); + } +} diff --git a/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql new file mode 100644 index 00000000000..f7e5f0c9db8 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql @@ -0,0 +1,20 @@ +import java +import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery +import TestUtilities.InlineExpectationsTest + +module CsrfUnprotectedRequestTypeTest implements TestSig { + string getARelevantTag() { result = "hasCsrfUnprotectedRequestType" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasCsrfUnprotectedRequestType" and + exists(PathNode src, PathNode sink, PathNode sinkPred | + unprotectedStateChange(src, sink, sinkPred) + | + src.getLocation() = location and + element = src.toString() and + value = "" + ) + } +} + +import MakeTest diff --git a/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.java b/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.java new file mode 100644 index 00000000000..a49e9f997ff --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.java @@ -0,0 +1,31 @@ +import org.apache.ibatis.annotations.Mapper; +import org.springframework.stereotype.Repository; +import org.apache.ibatis.annotations.DeleteProvider; +import org.apache.ibatis.annotations.UpdateProvider; +import org.apache.ibatis.annotations.InsertProvider; + +@Mapper +@Repository +public interface MyBatisMapper { + + void bad7(String name); + + //using providers + @DeleteProvider( + type = MyBatisProvider.class, + method = "badDelete" + ) + void badDelete(String input); + + @UpdateProvider( + type = MyBatisProvider.class, + method = "badUpdate" + ) + void badUpdate(String input); + + @InsertProvider( + type = MyBatisProvider.class, + method = "badInsert" + ) + void badInsert(String input); +} diff --git a/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml b/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml new file mode 100644 index 00000000000..be8efe9422f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml @@ -0,0 +1,35 @@ + + + + + + + + + + + + + + and name = ${ test . name , jdbcType = VARCHAR } + + + and id = #{test.id} + + + + + + insert into test (name, pass) + + + name = ${name,jdbcType=VARCHAR}, + + + pass = ${pass}, + + + + + diff --git a/java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java b/java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java new file mode 100644 index 00000000000..53b1ca723bd --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java @@ -0,0 +1,24 @@ +import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.jdbc.SQL; + +public class MyBatisProvider { + + public String badDelete(@Param("input") final String input) { + return "DELETE FROM users WHERE username = '" + input + "';"; + } + + public String badUpdate(@Param("input") final String input) { + String s = (new SQL() { + { + this.UPDATE("users"); + this.SET("balance = 0"); + this.WHERE("username = '" + input + "'"); + } + }).toString(); + return s; + } + + public String badInsert(@Param("input") final String input) { + return "INSERT INTO users VALUES (1, '" + input + "', 'hunter2');"; + } +} diff --git a/java/ql/test/query-tests/security/CWE-352/MyBatisService.java b/java/ql/test/query-tests/security/CWE-352/MyBatisService.java new file mode 100644 index 00000000000..78ea02399b2 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-352/MyBatisService.java @@ -0,0 +1,25 @@ +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; + +@Service +public class MyBatisService { + + @Autowired + private MyBatisMapper myBatisMapper; + + public void bad7(String name) { + myBatisMapper.bad7(name); + } + + public void badDelete(String input) { + myBatisMapper.badDelete(input); + } + + public void badUpdate(String input) { + myBatisMapper.badUpdate(input); + } + + public void badInsert(String input) { + myBatisMapper.badInsert(input); + } +} diff --git a/java/ql/test/query-tests/security/CWE-352/options b/java/ql/test/query-tests/security/CWE-352/options index 595ccc6b812..339a048c320 100644 --- a/java/ql/test/query-tests/security/CWE-352/options +++ b/java/ql/test/query-tests/security/CWE-352/options @@ -1 +1 @@ -semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8 \ No newline at end of file +semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/org.mybatis-3.5.4/