Skip to content

Commit 2cbad4a

Browse files
aschackmullaschackmull
authored
Merge pull request #6600 from atorralba/atorralba/fix-conditionalbypass
Java: Fix performance of the query User-controlled bypass of sensitive method
2 parents 3247794 + f18c163 commit 2cbad4a

File tree

11 files changed

+189
-113
lines changed

11 files changed

+189
-113
lines changed

‎java/change-notes/2021-09-14-conditional-bypass-improvements.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query `java/user-controlled-bypass` has been improved to reduce its execution time and false positive ratio. Less but more precise results should be found now, and the query should run significatively faster.

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides classes to be used in queries related to vulnerabilities
3+
* about unstrusted input being used in security decisions.
4+
*/
5+
6+
import java
7+
import semmle.code.java.dataflow.FlowSources
8+
import semmle.code.java.security.SensitiveActions
9+
import semmle.code.java.controlflow.Guards
10+
11+
/**
12+
* Holds if `ma` is controlled by the condition expression `e`.
13+
*/
14+
predicate conditionControlsMethod(MethodAccess ma, Expr e) {
15+
exists(ConditionBlock cb, SensitiveExecutionMethod m, boolean cond |
16+
ma.getMethod() = m and
17+
cb.controls(ma.getBasicBlock(), cond) and
18+
not cb.controls(any(SensitiveExecutionMethod sem).getAReference().getBasicBlock(),
19+
cond.booleanNot()) and
20+
not cb.controls(any(ThrowStmt t).getBasicBlock(), cond.booleanNot()) and
21+
not cb.controls(any(ReturnStmt r).getBasicBlock(), cond.booleanNot()) and
22+
e = cb.getCondition()
23+
)
24+
}
25+
26+
/**
27+
* A taint tracking configuration for untrusted data flowing to sensitive conditions.
28+
*/
29+
class ConditionalBypassFlowConfig extends TaintTracking::Configuration {
30+
ConditionalBypassFlowConfig() { this = "ConditionalBypassFlowConfig" }
31+
32+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
33+
34+
override predicate isSink(DataFlow::Node sink) { conditionControlsMethod(_, sink.asExpr()) }
35+
}

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

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -80,17 +80,12 @@ abstract class SensitiveExecutionMethod extends Method { }
8080
class AuthMethod extends SensitiveExecutionMethod {
8181
AuthMethod() {
8282
exists(string s | s = this.getName().toLowerCase() |
83-
(
84-
s.matches("%login%") or
85-
s.matches("%auth%")
86-
) and
87-
not (
88-
s.matches("get%") or
89-
s.matches("set%") or
90-
s.matches("parse%") or
91-
s.matches("%loginfo%")
92-
)
93-
)
83+
s.matches(["%login%", "%auth%"]) and
84+
not s.matches(["get%", "set%", "parse%", "%loginfo%", "remove%", "clean%", "%unauth%"]) and
85+
// exclude "author", but not "authorize" or "authority"
86+
not s.regexpMatch(".*[aA]uthors?([A-Z0-9_].*|$)")
87+
) and
88+
not this.getDeclaringType().getASupertype*() instanceof TypeException
9489
}
9590
}
9691

‎java/ql/src/Security/CWE/CWE-807/ConditionalBypass.ql

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -13,33 +13,10 @@
1313
*/
1414

1515
import java
16-
import semmle.code.java.dataflow.FlowSources
17-
import semmle.code.java.security.SensitiveActions
18-
import semmle.code.java.controlflow.Dominance
19-
import semmle.code.java.controlflow.Guards
16+
import semmle.code.java.dataflow.DataFlow
17+
import semmle.code.java.security.ConditionalBypassQuery
2018
import DataFlow::PathGraph
2119

22-
/**
23-
* Calls to a sensitive method that are controlled by a condition
24-
* on the given expression.
25-
*/
26-
predicate conditionControlsMethod(MethodAccess m, Expr e) {
27-
exists(ConditionBlock cb, SensitiveExecutionMethod def, boolean cond |
28-
cb.controls(m.getBasicBlock(), cond) and
29-
def = m.getMethod() and
30-
not cb.controls(def.getAReference().getBasicBlock(), cond.booleanNot()) and
31-
e = cb.getCondition()
32-
)
33-
}
34-
35-
class ConditionalBypassFlowConfig extends TaintTracking::Configuration {
36-
ConditionalBypassFlowConfig() { this = "ConditionalBypassFlowConfig" }
37-
38-
override predicate isSource(DataFlow::Node source) { source instanceof UserInput }
39-
40-
override predicate isSink(DataFlow::Node sink) { conditionControlsMethod(_, sink.asExpr()) }
41-
}
42-
4320
from
4421
DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, Expr e,
4522
ConditionalBypassFlowConfig conf

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

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

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

Lines changed: 0 additions & 1 deletion
This file was deleted.

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

Whitespace-only changes.

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

Lines changed: 95 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -2,58 +2,45 @@
22
// http://cwe.mitre.org/data/definitions/807.html
33
package test.cwe807.semmle.tests;
44

5-
6-
7-
85
import java.net.InetAddress;
96
import java.net.Inet4Address;
107
import java.net.UnknownHostException;
118

129
import javax.servlet.http.Cookie;
10+
import javax.servlet.http.HttpServletRequest;
1311
import org.apache.shiro.SecurityUtils;
1412
import org.apache.shiro.subject.Subject;
1513

16-
class Test {
17-
public static void main(String[] args) throws UnknownHostException {
18-
String user = args[0];
19-
String password = args[1];
20-
21-
String isAdmin = args[3];
22-
14+
class ConditionalBypassTest {
15+
public static void main(HttpServletRequest request) throws Exception {
16+
String user = request.getParameter("user");
17+
String password = request.getParameter("password");
18+
19+
String isAdmin = request.getParameter("isAdmin");
20+
2321
// BAD: login is only executed if isAdmin is false, but isAdmin
2422
// is controlled by the user
25-
if(isAdmin=="false")
23+
if (isAdmin == "false") // $ hasConditionalBypassTest
2624
login(user, password);
27-
25+
2826
Cookie adminCookie = getCookies()[0];
2927
// BAD: login is only executed if the cookie value is false, but the cookie
3028
// is controlled by the user
31-
if(adminCookie.getValue().equals("false"))
29+
if (adminCookie.getValue().equals("false")) // $ hasConditionalBypassTest
3230
login(user, password);
33-
34-
// FALSE POSITIVES: both methods are conditionally executed, but they probably
31+
32+
// GOOD: both methods are conditionally executed, but they probably
3533
// both perform the security-critical action
36-
if(adminCookie.getValue()=="false") {
34+
if (adminCookie.getValue() == "false") { // Safe
3735
login(user, password);
3836
} else {
3937
reCheckAuth(user, password);
4038
}
41-
39+
4240
// FALSE NEGATIVE: we have no way of telling that the skipped method is sensitive
43-
if(adminCookie.getValue()=="false")
41+
if (adminCookie.getValue() == "false") // $ MISSING: $ hasConditionalBypassTest
4442
doReallyImportantSecurityWork();
45-
46-
// Apache Shiro permissions system
47-
String whatDoTheyWantToDo = args[4];
48-
Subject subject = SecurityUtils.getSubject();
49-
// BAD: permissions decision made using tainted data
50-
if(subject.isPermitted("domain:sublevel:" + whatDoTheyWantToDo))
51-
doIt();
52-
53-
// GOOD: use fixed checks
54-
if(subject.isPermitted("domain:sublevel:whatTheMethodDoes"))
55-
doIt();
56-
43+
5744
InetAddress local = InetAddress.getLocalHost();
5845
// GOOD: reverse DNS on localhost is fine
5946
if (local.getCanonicalHostName().equals("localhost")) {
@@ -63,68 +50,129 @@ public static void main(String[] args) throws UnknownHostException {
6350
login(user, password);
6451
}
6552
}
66-
53+
6754
public static void test(String user, String password) {
6855
Cookie adminCookie = getCookies()[0];
6956
// GOOD: login always happens
70-
if(adminCookie.getValue()=="false")
57+
if (adminCookie.getValue() == "false")
7158
login(user, password);
7259
else {
73-
// do something else
7460
login(user, password);
7561
}
7662
}
77-
63+
7864
public static void test2(String user, String password) {
7965
Cookie adminCookie = getCookies()[0];
8066
// BAD: login may happen once or twice
81-
if(adminCookie.getValue()=="false")
67+
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
8268
login(user, password);
8369
else {
8470
// do something else
71+
doIt();
8572
}
8673
login(user, password);
8774
}
88-
75+
8976
public static void test3(String user, String password) {
9077
Cookie adminCookie = getCookies()[0];
91-
if(adminCookie.getValue()=="false")
78+
// BAD: login may not happen
79+
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
9280
login(user, password);
9381
else {
9482
// do something else
95-
// BAD: login may not happen
96-
return;
83+
doIt();
9784
}
85+
return;
9886
}
99-
87+
10088
public static void test4(String user, String password) {
10189
Cookie adminCookie = getCookies()[0];
10290
// GOOD: login always happens
103-
if(adminCookie.getValue()=="false") {
91+
if (adminCookie.getValue() == "false") {
10492
login(user, password);
10593
return;
10694
}
107-
95+
10896
// do other things
10997
login(user, password);
11098
return;
11199
}
112-
100+
101+
public static void test5(String user, String password) throws Exception {
102+
Cookie adminCookie = getCookies()[0];
103+
// GOOD: exit with Exception if condition is not met
104+
if (adminCookie.getValue() == "false") {
105+
throw new Exception();
106+
}
107+
108+
login(user, password);
109+
}
110+
111+
public static void test6(String user, String password) {
112+
Cookie adminCookie = getCookies()[0];
113+
// GOOD: exit with return if condition is not met
114+
if (adminCookie.getValue() == "false") {
115+
return;
116+
}
117+
118+
login(user, password);
119+
}
120+
121+
public static void test7(String user, String password) {
122+
Cookie adminCookie = getCookies()[0];
123+
// BAD: login is bypasseable
124+
if (adminCookie.getValue() == "false") { // $ hasConditionalBypassTest
125+
login(user, password);
126+
return;
127+
} else {
128+
doIt();
129+
}
130+
}
131+
132+
public static void test8(String user, String password) {
133+
Cookie adminCookie = getCookies()[0];
134+
{
135+
// BAD: login may not happen
136+
if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
137+
authorize(user, password);
138+
else {
139+
// do something else
140+
doIt();
141+
}
142+
}
143+
{
144+
// obtainAuthor is not sensitive, so this is safe
145+
if (adminCookie.getValue() == "false")
146+
obtainAuthor();
147+
else {
148+
doIt();
149+
}
150+
}
151+
}
152+
113153
public static void login(String user, String password) {
114154
// login
115155
}
116-
156+
117157
public static void reCheckAuth(String user, String password) {
118158
// login
119159
}
120-
160+
161+
public static void authorize(String user, String password) {
162+
// login
163+
}
164+
165+
public static String obtainAuthor() {
166+
return "";
167+
}
168+
121169
public static Cookie[] getCookies() {
122170
// get cookies from a servlet
123171
return new Cookie[0];
124172
}
125-
173+
126174
public static void doIt() {}
127-
175+
128176
public static void doReallyImportantSecurityWork() {
129177
// login, authenticate, everything
130178
}

‎java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.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.ConditionalBypassQuery
3+
import TestUtilities.InlineExpectationsTest
4+
5+
class ConditionalBypassTest extends InlineExpectationsTest {
6+
ConditionalBypassTest() { this = "ConditionalBypassTest" }
7+
8+
override string getARelevantTag() { result = "hasConditionalBypassTest" }
9+
10+
override predicate hasActualResult(Location location, string element, string tag, string value) {
11+
tag = "hasConditionalBypassTest" and
12+
exists(DataFlow::Node src, DataFlow::Node sink, ConditionalBypassFlowConfig conf |
13+
conf.hasFlow(src, sink)
14+
|
15+
sink.getLocation() = location and
16+
element = sink.toString() and
17+
value = ""
18+
)
19+
}
20+
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
edges
2-
| Test.java:17:26:17:38 | args : String[] | Test.java:50:26:50:64 | ... + ... |
2+
| TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... |
33
nodes
4-
| Test.java:17:26:17:38 | args : String[] | semmle.label | args : String[] |
5-
| Test.java:50:26:50:64 | ... + ... | semmle.label | ... + ... |
4+
| TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
5+
| TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... | semmle.label | ... + ... |
66
subpaths
77
#select
8-
| Test.java:50:6:50:65 | isPermitted(...) | Test.java:17:26:17:38 | args : String[] | Test.java:50:26:50:64 | ... + ... | Permissions check uses user-controlled $@. | Test.java:17:26:17:38 | args | data |
8+
| TaintedPermissionsCheckTest.java:15:7:15:54 | isPermitted(...) | TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... | Permissions check uses user-controlled $@. | TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) | data |

0 commit comments

Comments
 (0)