Skip to content

Commit 3860299

Browse files
d10cd10c
committed
Java: ConditionalBypass (enable diff-informed + convert test to qlref)
1 parent 61bce89 commit 3860299

File tree

5 files changed

+56
-34
lines changed

5 files changed

+56
-34
lines changed

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,13 @@ module ConditionalBypassFlowConfig implements DataFlow::ConfigSig {
4848
endsWithStep(node1, node2)
4949
}
5050

51-
predicate observeDiffInformedIncrementalMode() {
52-
any() // TODO: Make sure that the location overrides match the query's select clause: Column 1 does not select a source or sink originating from the flow call on line 25 (/Users/d10c/src/semmle-code/ql/java/ql/src/Security/CWE/CWE-807/ConditionalBypass.ql@26:8:26:8)
51+
predicate observeDiffInformedIncrementalMode() { any() }
52+
53+
Location getASelectedSinkLocation(DataFlow::Node sink) {
54+
exists(MethodCall m, Expr e | result = [m, e].getLocation() |
55+
conditionControlsMethod(m, e) and
56+
sink.asExpr() = e
57+
)
5358
}
5459
}
5560

‎java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.expected

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
#select
2+
| ConditionalBypassTest.java:24:4:24:24 | login(...) | ConditionalBypassTest.java:19:20:19:50 | getParameter(...) : String | ConditionalBypassTest.java:23:7:23:24 | ... == ... | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:23:7:23:24 | ... == ... | this condition | ConditionalBypassTest.java:19:20:19:50 | getParameter(...) | user-controlled value |
3+
| ConditionalBypassTest.java:30:4:30:24 | login(...) | ConditionalBypassTest.java:29:7:29:28 | getValue(...) : String | ConditionalBypassTest.java:29:7:29:44 | equals(...) | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:29:7:29:44 | equals(...) | this condition | ConditionalBypassTest.java:29:7:29:28 | getValue(...) | user-controlled value |
4+
| ConditionalBypassTest.java:77:4:77:24 | login(...) | ConditionalBypassTest.java:76:7:76:28 | getValue(...) : String | ConditionalBypassTest.java:76:7:76:39 | ... == ... | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:76:7:76:39 | ... == ... | this condition | ConditionalBypassTest.java:76:7:76:28 | getValue(...) | user-controlled value |
5+
| ConditionalBypassTest.java:89:4:89:24 | login(...) | ConditionalBypassTest.java:88:7:88:28 | getValue(...) : String | ConditionalBypassTest.java:88:7:88:39 | ... == ... | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:88:7:88:39 | ... == ... | this condition | ConditionalBypassTest.java:88:7:88:28 | getValue(...) | user-controlled value |
6+
| ConditionalBypassTest.java:134:4:134:24 | login(...) | ConditionalBypassTest.java:133:7:133:28 | getValue(...) : String | ConditionalBypassTest.java:133:7:133:39 | ... == ... | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:133:7:133:39 | ... == ... | this condition | ConditionalBypassTest.java:133:7:133:28 | getValue(...) | user-controlled value |
7+
| ConditionalBypassTest.java:146:5:146:29 | authorize(...) | ConditionalBypassTest.java:145:8:145:29 | getValue(...) : String | ConditionalBypassTest.java:145:8:145:40 | ... == ... | Sensitive method may not be executed depending on a $@, which flows from $@. | ConditionalBypassTest.java:145:8:145:40 | ... == ... | this condition | ConditionalBypassTest.java:145:8:145:29 | getValue(...) | user-controlled value |
8+
edges
9+
| ConditionalBypassTest.java:19:20:19:50 | getParameter(...) : String | ConditionalBypassTest.java:23:7:23:24 | ... == ... | provenance | Src:MaD:2 |
10+
| ConditionalBypassTest.java:29:7:29:28 | getValue(...) : String | ConditionalBypassTest.java:29:7:29:44 | equals(...) | provenance | Src:MaD:1 |
11+
| ConditionalBypassTest.java:76:7:76:28 | getValue(...) : String | ConditionalBypassTest.java:76:7:76:39 | ... == ... | provenance | Src:MaD:1 |
12+
| ConditionalBypassTest.java:88:7:88:28 | getValue(...) : String | ConditionalBypassTest.java:88:7:88:39 | ... == ... | provenance | Src:MaD:1 |
13+
| ConditionalBypassTest.java:133:7:133:28 | getValue(...) : String | ConditionalBypassTest.java:133:7:133:39 | ... == ... | provenance | Src:MaD:1 |
14+
| ConditionalBypassTest.java:145:8:145:29 | getValue(...) : String | ConditionalBypassTest.java:145:8:145:40 | ... == ... | provenance | Src:MaD:1 |
15+
models
16+
| 1 | Source: javax.servlet.http; Cookie; false; getValue; (); ; ReturnValue; remote; manual |
17+
| 2 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual |
18+
nodes
19+
| ConditionalBypassTest.java:19:20:19:50 | getParameter(...) : String | semmle.label | getParameter(...) : String |
20+
| ConditionalBypassTest.java:23:7:23:24 | ... == ... | semmle.label | ... == ... |
21+
| ConditionalBypassTest.java:29:7:29:28 | getValue(...) : String | semmle.label | getValue(...) : String |
22+
| ConditionalBypassTest.java:29:7:29:44 | equals(...) | semmle.label | equals(...) |
23+
| ConditionalBypassTest.java:76:7:76:28 | getValue(...) : String | semmle.label | getValue(...) : String |
24+
| ConditionalBypassTest.java:76:7:76:39 | ... == ... | semmle.label | ... == ... |
25+
| ConditionalBypassTest.java:88:7:88:28 | getValue(...) : String | semmle.label | getValue(...) : String |
26+
| ConditionalBypassTest.java:88:7:88:39 | ... == ... | semmle.label | ... == ... |
27+
| ConditionalBypassTest.java:133:7:133:28 | getValue(...) : String | semmle.label | getValue(...) : String |
28+
| ConditionalBypassTest.java:133:7:133:39 | ... == ... | semmle.label | ... == ... |
29+
| ConditionalBypassTest.java:145:8:145:29 | getValue(...) : String | semmle.label | getValue(...) : String |
30+
| ConditionalBypassTest.java:145:8:145:40 | ... == ... | semmle.label | ... == ... |
31+
subpaths

‎java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.java

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,18 @@ public static void main(HttpServletRequest request) throws Exception {
1616
String user = request.getParameter("user");
1717
String password = request.getParameter("password");
1818

19-
String isAdmin = request.getParameter("isAdmin");
19+
String isAdmin = request.getParameter("isAdmin"); // $ Source
2020

2121
// BAD: login is only executed if isAdmin is false, but isAdmin
2222
// is controlled by the user
23-
if (isAdmin == "false") // $ hasConditionalBypassTest
24-
login(user, password);
23+
if (isAdmin == "false") // $ Sink
24+
login(user, password); // $ Alert
2525

2626
Cookie adminCookie = getCookies()[0];
2727
// BAD: login is only executed if the cookie value is false, but the cookie
2828
// is controlled by the user
29-
if (adminCookie.getValue().equals("false")) // $ hasConditionalBypassTest
30-
login(user, password);
29+
if (adminCookie.getValue().equals("false")) // $ Source Sink
30+
login(user, password); // $ Alert
3131

3232
// GOOD: both methods are conditionally executed, but they probably
3333
// both perform the security-critical action
@@ -38,7 +38,7 @@ public static void main(HttpServletRequest request) throws Exception {
3838
}
3939

4040
// FALSE NEGATIVE: we have no way of telling that the skipped method is sensitive
41-
if (adminCookie.getValue() == "false") // $ MISSING: hasConditionalBypassTest
41+
if (adminCookie.getValue() == "false") // $ MISSING: Alert
4242
doReallyImportantSecurityWork();
4343

4444
InetAddress local = InetAddress.getLocalHost();
@@ -73,8 +73,8 @@ public static void test(String user, String password) {
7373
public static void test2(String user, String password) {
7474
Cookie adminCookie = getCookies()[0];
7575
// BAD: login may happen once or twice
76-
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
77-
login(user, password);
76+
if (adminCookie.getValue() == "false") // $ Source Sink
77+
login(user, password); // $ Alert
7878
else {
7979
// do something else
8080
doIt();
@@ -85,8 +85,8 @@ public static void test2(String user, String password) {
8585
public static void test3(String user, String password) {
8686
Cookie adminCookie = getCookies()[0];
8787
// BAD: login may not happen
88-
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
89-
login(user, password);
88+
if (adminCookie.getValue() == "false") // $ Source Sink
89+
login(user, password); // $ Alert
9090
else {
9191
// do something else
9292
doIt();
@@ -130,8 +130,8 @@ public static void test6(String user, String password) {
130130
public static void test7(String user, String password) {
131131
Cookie adminCookie = getCookies()[0];
132132
// BAD: login is bypasseable
133-
if (adminCookie.getValue() == "false") { // $ hasConditionalBypassTest
134-
login(user, password);
133+
if (adminCookie.getValue() == "false") { // $ Source Sink
134+
login(user, password); // $ Alert
135135
return;
136136
} else {
137137
doIt();
@@ -142,8 +142,8 @@ public static void test8(String user, String password) {
142142
Cookie adminCookie = getCookies()[0];
143143
{
144144
// BAD: login may not happen
145-
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
146-
authorize(user, password);
145+
if (adminCookie.getValue() == "false") // $ Source Sink
146+
authorize(user, password); // $ Alert
147147
else {
148148
// do something else
149149
doIt();

‎java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.ql

Lines changed: 0 additions & 18 deletions
This file was deleted.

‎java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.qlref

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
query: Security/CWE/CWE-807/ConditionalBypass.ql
2+
postprocess:
3+
- utils/test/PrettyPrintModels.ql
4+
- utils/test/InlineExpectationsTestQuery.ql

0 commit comments

Comments
 (0)