Java: initial tests

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
 }

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"

java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.expected

@@ -0,0 +1,2 @@
+testFailures
+failures

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);
+	}
+}

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<CsrfUnprotectedRequestTypeTest>

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);
+}

java/ql/test/query-tests/security/CWE-352/MyBatisMapper.xml

@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!DOCTYPE mapper PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN"
+  "http://mybatis.org/dtd/mybatis-3-mapper.dtd" >
+<mapper namespace="MyBatisMapper">
+
+  <resultMap id="BaseResultMap" type="Test">
+    <id column="id" jdbcType="INTEGER" property="id"/>
+    <result column="name" jdbcType="VARCHAR" property="name"/>
+    <result column="pass" jdbcType="VARCHAR" property="pass"/>
+  </resultMap>
+
+  <sql id="Update_By_Example_Where_Clause">
+    <where>
+      <if test="test.name != null">
+        and name = ${ test . name , jdbcType = VARCHAR }
+      </if>
+      <if test="test.id != null">
+        and id = #{test.id}
+      </if>
+    </where>
+  </sql>
+
+  <insert id="bad7" parameterType="Test">
+    insert into test (name, pass)
+    <trim prefix="values (" suffix=")" suffixOverrides=",">
+      <if test="name != null">
+        name = ${name,jdbcType=VARCHAR},
+      </if>
+      <if test="pass != null">
+        pass = ${pass},
+      </if>
+    </trim>
+  </insert>
+
+</mapper>

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');";
+    }
+}

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);
+	}
+}

java/ql/test/query-tests/security/CWE-352/options

@@ -1 +1 @@
-semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8
+semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/org.mybatis-3.5.4/