Skip to content

Commit df77d49

Browse files
Jami CogswellJami Cogswell
Jami Cogswell
authored and
Jami Cogswell
committed
Java: initial tests
1 parent 178b032 commit df77d49

File tree

10 files changed

+310
-8
lines changed

10 files changed

+310
-8
lines changed

‎java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,5 +114,15 @@ module CallGraph {
114114
}
115115
}
116116

117-
query predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ }
117+
predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ }
118+
}
119+
120+
import CallGraph
121+
122+
/** Holds if `src` is an unprotected request handler that reaches a state-changing `sink`. */
123+
predicate unprotectedStateChange(PathNode src, PathNode sink, PathNode sinkPred) {
124+
src.asMethod() instanceof CsrfUnprotectedMethod and
125+
sink.asMethod() instanceof DatabaseUpdateMethod and
126+
sinkPred.getASuccessor() = sink and
127+
src.getASuccessor+() = sinkPred
118128
}

‎java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,11 @@
1414

1515
import java
1616
import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery
17-
import CallGraph
17+
18+
query predicate edges(PathNode pred, PathNode succ) { CallGraph::edges(pred, succ) }
1819

1920
from PathNode source, PathNode reachable, PathNode callsReachable
20-
where
21-
source.asMethod() instanceof CsrfUnprotectedMethod and
22-
reachable.asMethod() instanceof DatabaseUpdateMethod and
23-
callsReachable.getASuccessor() = reachable and
24-
source.getASuccessor+() = callsReachable
21+
where unprotectedStateChange(source, reachable, callsReachable)
2522
select source.asMethod(), source, callsReachable,
2623
"Potential CSRF vulnerability due to using an HTTP request type which is not default-protected from CSRF for an apparent $@.",
2724
callsReachable, "state-changing action"

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
testFailures
2+
failures

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

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
import org.springframework.stereotype.Controller;
2+
import org.springframework.web.bind.annotation.RequestMapping;
3+
import org.springframework.web.bind.annotation.GetMapping;
4+
import org.springframework.web.bind.annotation.PostMapping;
5+
import org.springframework.web.bind.annotation.RequestMethod;
6+
import static org.springframework.web.bind.annotation.RequestMethod.*;
7+
import org.springframework.web.bind.annotation.RequestParam;
8+
import org.springframework.beans.factory.annotation.Autowired;
9+
import java.sql.DriverManager;
10+
import java.sql.Connection;
11+
import java.sql.PreparedStatement;
12+
import java.sql.SQLException;
13+
14+
@Controller
15+
public class CsrfUnprotectedRequestTypeTest {
16+
17+
// Test Spring sources with `PreparedStatement.executeUpdate()` as a default database update method call
18+
19+
// BAD: allows request type not default-protected from CSRF when updating a database
20+
@RequestMapping("/")
21+
public void bad1() { // $ hasCsrfUnprotectedRequestType
22+
try {
23+
String sql = "DELETE";
24+
Connection conn = DriverManager.getConnection("url");
25+
PreparedStatement ps = conn.prepareStatement(sql);
26+
ps.executeUpdate(); // database update method call
27+
} catch (SQLException e) { }
28+
}
29+
30+
// BAD: uses GET request when updating a database
31+
@RequestMapping(value = "", method = RequestMethod.GET)
32+
public void bad2() { // $ hasCsrfUnprotectedRequestType
33+
try {
34+
String sql = "DELETE";
35+
Connection conn = DriverManager.getConnection("url");
36+
PreparedStatement ps = conn.prepareStatement(sql);
37+
ps.executeUpdate(); // database update method call
38+
} catch (SQLException e) { }
39+
}
40+
41+
// BAD: uses GET request when updating a database
42+
@GetMapping(value = "")
43+
public void bad3() { // $ hasCsrfUnprotectedRequestType
44+
try {
45+
String sql = "DELETE";
46+
Connection conn = DriverManager.getConnection("url");
47+
PreparedStatement ps = conn.prepareStatement(sql);
48+
ps.executeUpdate(); // database update method call
49+
} catch (SQLException e) { }
50+
}
51+
52+
// BAD: allows GET request when updating a database
53+
@RequestMapping(value = "", method = { RequestMethod.GET, RequestMethod.POST })
54+
public void bad4() { // $ hasCsrfUnprotectedRequestType
55+
try {
56+
String sql = "DELETE";
57+
Connection conn = DriverManager.getConnection("url");
58+
PreparedStatement ps = conn.prepareStatement(sql);
59+
ps.executeUpdate(); // database update method call
60+
} catch (SQLException e) { }
61+
}
62+
63+
// BAD: uses request type not default-protected from CSRF when updating a database
64+
@RequestMapping(value = "", method = { GET, HEAD, OPTIONS, TRACE })
65+
public void bad5() { // $ hasCsrfUnprotectedRequestType
66+
try {
67+
String sql = "DELETE";
68+
Connection conn = DriverManager.getConnection("url");
69+
PreparedStatement ps = conn.prepareStatement(sql);
70+
ps.executeUpdate(); // database update method call
71+
} catch (SQLException e) { }
72+
}
73+
74+
// GOOD: uses POST request when updating a database
75+
@RequestMapping(value = "", method = RequestMethod.POST)
76+
public void good1() {
77+
try {
78+
String sql = "DELETE";
79+
Connection conn = DriverManager.getConnection("url");
80+
PreparedStatement ps = conn.prepareStatement(sql);
81+
ps.executeUpdate(); // database update method call
82+
} catch (SQLException e) { }
83+
}
84+
85+
// GOOD: uses POST request when updating a database
86+
@RequestMapping(value = "", method = POST)
87+
public void good2() {
88+
try {
89+
String sql = "DELETE";
90+
Connection conn = DriverManager.getConnection("url");
91+
PreparedStatement ps = conn.prepareStatement(sql);
92+
ps.executeUpdate(); // database update method call
93+
} catch (SQLException e) { }
94+
}
95+
96+
// GOOD: uses POST request when updating a database
97+
@PostMapping(value = "")
98+
public void good3() {
99+
try {
100+
String sql = "DELETE";
101+
Connection conn = DriverManager.getConnection("url");
102+
PreparedStatement ps = conn.prepareStatement(sql);
103+
ps.executeUpdate(); // database update method call
104+
} catch (SQLException e) { }
105+
}
106+
107+
// GOOD: uses a request type that is default-protected from CSRF when updating a database
108+
@RequestMapping(value = "", method = { POST, PUT, PATCH, DELETE })
109+
public void good4() {
110+
try {
111+
String sql = "DELETE";
112+
Connection conn = DriverManager.getConnection("url");
113+
PreparedStatement ps = conn.prepareStatement(sql);
114+
ps.executeUpdate(); // database update method call
115+
} catch (SQLException e) { }
116+
}
117+
118+
// Test database update method calls other than `PreparedStatement.executeUpdate()`
119+
120+
// BAD: allows request type not default-protected from CSRF when
121+
// updating a database using `PreparedStatement.executeLargeUpdate()`
122+
@RequestMapping("/")
123+
public void bad6() { // $ hasCsrfUnprotectedRequestType
124+
try {
125+
String sql = "DELETE";
126+
Connection conn = DriverManager.getConnection("url");
127+
PreparedStatement ps = conn.prepareStatement(sql);
128+
ps.executeLargeUpdate(); // database update method call
129+
} catch (SQLException e) { }
130+
}
131+
132+
@Autowired
133+
private MyBatisService myBatisService;
134+
135+
// BAD: uses GET request when updating a database with MyBatis XML mapper method
136+
@GetMapping(value = "")
137+
public void bad7(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
138+
myBatisService.bad7(name);
139+
}
140+
141+
// BAD: uses GET request when updating a database with `@DeleteProvider`
142+
@GetMapping(value = "badDelete")
143+
public void badDelete(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
144+
myBatisService.badDelete(name);
145+
}
146+
147+
// BAD: uses GET request when updating a database with `@UpdateProvider`
148+
@GetMapping(value = "badUpdate")
149+
public void badUpdate(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
150+
myBatisService.badUpdate(name);
151+
}
152+
153+
// BAD: uses GET request when updating a database with `@InsertProvider`
154+
@GetMapping(value = "badInsert")
155+
public void badInsert(@RequestParam String name) { // $ hasCsrfUnprotectedRequestType
156+
myBatisService.badInsert(name);
157+
}
158+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import java
2+
import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery
3+
import TestUtilities.InlineExpectationsTest
4+
5+
module CsrfUnprotectedRequestTypeTest implements TestSig {
6+
string getARelevantTag() { result = "hasCsrfUnprotectedRequestType" }
7+
8+
predicate hasActualResult(Location location, string element, string tag, string value) {
9+
tag = "hasCsrfUnprotectedRequestType" and
10+
exists(PathNode src, PathNode sink, PathNode sinkPred |
11+
unprotectedStateChange(src, sink, sinkPred)
12+
|
13+
src.getLocation() = location and
14+
element = src.toString() and
15+
value = ""
16+
)
17+
}
18+
}
19+
20+
import MakeTest<CsrfUnprotectedRequestTypeTest>

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import org.apache.ibatis.annotations.Mapper;
2+
import org.springframework.stereotype.Repository;
3+
import org.apache.ibatis.annotations.DeleteProvider;
4+
import org.apache.ibatis.annotations.UpdateProvider;
5+
import org.apache.ibatis.annotations.InsertProvider;
6+
7+
@Mapper
8+
@Repository
9+
public interface MyBatisMapper {
10+
11+
void bad7(String name);
12+
13+
//using providers
14+
@DeleteProvider(
15+
type = MyBatisProvider.class,
16+
method = "badDelete"
17+
)
18+
void badDelete(String input);
19+
20+
@UpdateProvider(
21+
type = MyBatisProvider.class,
22+
method = "badUpdate"
23+
)
24+
void badUpdate(String input);
25+
26+
@InsertProvider(
27+
type = MyBatisProvider.class,
28+
method = "badInsert"
29+
)
30+
void badInsert(String input);
31+
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?xml version="1.0" encoding="UTF-8" ?>
2+
<!DOCTYPE mapper PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN"
3+
"http://mybatis.org/dtd/mybatis-3-mapper.dtd" >
4+
<mapper namespace="MyBatisMapper">
5+
6+
<resultMap id="BaseResultMap" type="Test">
7+
<id column="id" jdbcType="INTEGER" property="id"/>
8+
<result column="name" jdbcType="VARCHAR" property="name"/>
9+
<result column="pass" jdbcType="VARCHAR" property="pass"/>
10+
</resultMap>
11+
12+
<sql id="Update_By_Example_Where_Clause">
13+
<where>
14+
<if test="test.name != null">
15+
and name = ${ test . name , jdbcType = VARCHAR }
16+
</if>
17+
<if test="test.id != null">
18+
and id = #{test.id}
19+
</if>
20+
</where>
21+
</sql>
22+
23+
<insert id="bad7" parameterType="Test">
24+
insert into test (name, pass)
25+
<trim prefix="values (" suffix=")" suffixOverrides=",">
26+
<if test="name != null">
27+
name = ${name,jdbcType=VARCHAR},
28+
</if>
29+
<if test="pass != null">
30+
pass = ${pass},
31+
</if>
32+
</trim>
33+
</insert>
34+
35+
</mapper>

‎java/ql/test/query-tests/security/CWE-352/MyBatisProvider.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import org.apache.ibatis.annotations.Param;
2+
import org.apache.ibatis.jdbc.SQL;
3+
4+
public class MyBatisProvider {
5+
6+
public String badDelete(@Param("input") final String input) {
7+
return "DELETE FROM users WHERE username = '" + input + "';";
8+
}
9+
10+
public String badUpdate(@Param("input") final String input) {
11+
String s = (new SQL() {
12+
{
13+
this.UPDATE("users");
14+
this.SET("balance = 0");
15+
this.WHERE("username = '" + input + "'");
16+
}
17+
}).toString();
18+
return s;
19+
}
20+
21+
public String badInsert(@Param("input") final String input) {
22+
return "INSERT INTO users VALUES (1, '" + input + "', 'hunter2');";
23+
}
24+
}

‎java/ql/test/query-tests/security/CWE-352/MyBatisService.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import org.springframework.beans.factory.annotation.Autowired;
2+
import org.springframework.stereotype.Service;
3+
4+
@Service
5+
public class MyBatisService {
6+
7+
@Autowired
8+
private MyBatisMapper myBatisMapper;
9+
10+
public void bad7(String name) {
11+
myBatisMapper.bad7(name);
12+
}
13+
14+
public void badDelete(String input) {
15+
myBatisMapper.badDelete(input);
16+
}
17+
18+
public void badUpdate(String input) {
19+
myBatisMapper.badUpdate(input);
20+
}
21+
22+
public void badInsert(String input) {
23+
myBatisMapper.badInsert(input);
24+
}
25+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8
1+
semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8/:${testdir}/../../../stubs/org.mybatis-3.5.4/

0 commit comments

Comments
 (0)