Merge pull request #6600 from atorralba/atorralba/fix-conditionalbypass

Java: Fix performance of the query User-controlled bypass of sensitive method

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

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* 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

@@ -0,0 +1,35 @@
+/**
+ * Provides classes to be used in queries related to vulnerabilities
+ * about unstrusted input being used in security decisions.
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.security.SensitiveActions
+import semmle.code.java.controlflow.Guards
+
+/**
+ * Holds if `ma` is controlled by the condition expression `e`.
+ */
+predicate conditionControlsMethod(MethodAccess ma, Expr e) {
+  exists(ConditionBlock cb, SensitiveExecutionMethod m, boolean cond |
+    ma.getMethod() = m and
+    cb.controls(ma.getBasicBlock(), cond) and
+    not cb.controls(any(SensitiveExecutionMethod sem).getAReference().getBasicBlock(),
+      cond.booleanNot()) and
+    not cb.controls(any(ThrowStmt t).getBasicBlock(), cond.booleanNot()) and
+    not cb.controls(any(ReturnStmt r).getBasicBlock(), cond.booleanNot()) and
+    e = cb.getCondition()
+  )
+}
+
+/**
+ * A taint tracking configuration for untrusted data flowing to sensitive conditions.
+ */
+class ConditionalBypassFlowConfig extends TaintTracking::Configuration {
+  ConditionalBypassFlowConfig() { this = "ConditionalBypassFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { conditionControlsMethod(_, sink.asExpr()) }
+}

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

@@ -80,17 +80,12 @@ abstract class SensitiveExecutionMethod extends Method { }
 class AuthMethod extends SensitiveExecutionMethod {
   AuthMethod() {
     exists(string s | s = this.getName().toLowerCase() |
-      (
-        s.matches("%login%") or
-        s.matches("%auth%")
-      ) and
-      not (
-        s.matches("get%") or
-        s.matches("set%") or
-        s.matches("parse%") or
-        s.matches("%loginfo%")
-      )
-    )
+      s.matches(["%login%", "%auth%"]) and
+      not s.matches(["get%", "set%", "parse%", "%loginfo%", "remove%", "clean%", "%unauth%"]) and
+      // exclude "author", but not "authorize" or "authority"
+      not s.regexpMatch(".*[aA]uthors?([A-Z0-9_].*|$)")
+    ) and
+    not this.getDeclaringType().getASupertype*() instanceof TypeException
   }
 }
 

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

@@ -13,33 +13,10 @@
  */
 
 import java
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.security.SensitiveActions
-import semmle.code.java.controlflow.Dominance
-import semmle.code.java.controlflow.Guards
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.ConditionalBypassQuery
 import DataFlow::PathGraph
 
-/**
- * Calls to a sensitive method that are controlled by a condition
- * on the given expression.
- */
-predicate conditionControlsMethod(MethodAccess m, Expr e) {
-  exists(ConditionBlock cb, SensitiveExecutionMethod def, boolean cond |
-    cb.controls(m.getBasicBlock(), cond) and
-    def = m.getMethod() and
-    not cb.controls(def.getAReference().getBasicBlock(), cond.booleanNot()) and
-    e = cb.getCondition()
-  )
-}
-
-class ConditionalBypassFlowConfig extends TaintTracking::Configuration {
-  ConditionalBypassFlowConfig() { this = "ConditionalBypassFlowConfig" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof UserInput }
-
-  override predicate isSink(DataFlow::Node sink) { conditionControlsMethod(_, sink.asExpr()) }
-}
-
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, Expr e,
   ConditionalBypassFlowConfig conf

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

@@ -1,25 +0,0 @@
-edges
-| Test.java:17:26:17:38 | args : String[] | Test.java:25:6:25:21 | ... == ... |
-| Test.java:31:6:31:27 | getValue(...) : String | Test.java:31:6:31:43 | equals(...) |
-| Test.java:36:6:36:27 | getValue(...) : String | Test.java:36:6:36:36 | ... == ... |
-| Test.java:81:6:81:27 | getValue(...) : String | Test.java:81:6:81:36 | ... == ... |
-| Test.java:91:6:91:27 | getValue(...) : String | Test.java:91:6:91:36 | ... == ... |
-nodes
-| Test.java:17:26:17:38 | args : String[] | semmle.label | args : String[] |
-| Test.java:25:6:25:21 | ... == ... | semmle.label | ... == ... |
-| Test.java:31:6:31:27 | getValue(...) : String | semmle.label | getValue(...) : String |
-| Test.java:31:6:31:43 | equals(...) | semmle.label | equals(...) |
-| Test.java:36:6:36:27 | getValue(...) : String | semmle.label | getValue(...) : String |
-| Test.java:36:6:36:36 | ... == ... | semmle.label | ... == ... |
-| Test.java:81:6:81:27 | getValue(...) : String | semmle.label | getValue(...) : String |
-| Test.java:81:6:81:36 | ... == ... | semmle.label | ... == ... |
-| Test.java:91:6:91:27 | getValue(...) : String | semmle.label | getValue(...) : String |
-| Test.java:91:6:91:36 | ... == ... | semmle.label | ... == ... |
-subpaths
-#select
-| Test.java:26:4:26:24 | login(...) | Test.java:17:26:17:38 | args : String[] | Test.java:25:6:25:21 | ... == ... | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:25:6:25:21 | ... == ... | this condition | Test.java:17:26:17:38 | args | user input |
-| Test.java:32:4:32:24 | login(...) | Test.java:31:6:31:27 | getValue(...) : String | Test.java:31:6:31:43 | equals(...) | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:31:6:31:43 | equals(...) | this condition | Test.java:31:6:31:27 | getValue(...) | user input |
-| Test.java:37:4:37:24 | login(...) | Test.java:36:6:36:27 | getValue(...) : String | Test.java:36:6:36:36 | ... == ... | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:36:6:36:36 | ... == ... | this condition | Test.java:36:6:36:27 | getValue(...) | user input |
-| Test.java:39:4:39:30 | reCheckAuth(...) | Test.java:36:6:36:27 | getValue(...) : String | Test.java:36:6:36:36 | ... == ... | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:36:6:36:36 | ... == ... | this condition | Test.java:36:6:36:27 | getValue(...) | user input |
-| Test.java:82:4:82:24 | login(...) | Test.java:81:6:81:27 | getValue(...) : String | Test.java:81:6:81:36 | ... == ... | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:81:6:81:36 | ... == ... | this condition | Test.java:81:6:81:27 | getValue(...) | user input |
-| Test.java:92:4:92:24 | login(...) | Test.java:91:6:91:27 | getValue(...) : String | Test.java:91:6:91:36 | ... == ... | Sensitive method may not be executed depending on $@, which flows from $@. | Test.java:91:6:91:36 | ... == ... | this condition | Test.java:91:6:91:27 | getValue(...) | user input |

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

@@ -1 +0,0 @@
-Security/CWE/CWE-807/ConditionalBypass.ql

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

@@ -2,58 +2,45 @@
 // http://cwe.mitre.org/data/definitions/807.html
 package test.cwe807.semmle.tests;
 
-
-
-
 import java.net.InetAddress;
 import java.net.Inet4Address;
 import java.net.UnknownHostException;
 
 import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletRequest;
 import org.apache.shiro.SecurityUtils;
 import org.apache.shiro.subject.Subject;
 
-class Test {
-	public static void main(String[] args) throws UnknownHostException {
-		String user = args[0];
-		String password = args[1];
-		
-		String isAdmin = args[3];
-		
+class ConditionalBypassTest {
+	public static void main(HttpServletRequest request) throws Exception {
+		String user = request.getParameter("user");
+		String password = request.getParameter("password");
+
+		String isAdmin = request.getParameter("isAdmin");
+
 		// BAD: login is only executed if isAdmin is false, but isAdmin
 		// is controlled by the user
-		if(isAdmin=="false")
+		if (isAdmin == "false") // $ hasConditionalBypassTest
 			login(user, password);
-		
+
 		Cookie adminCookie = getCookies()[0];
 		// BAD: login is only executed if the cookie value is false, but the cookie
 		// is controlled by the user
-		if(adminCookie.getValue().equals("false"))
+		if (adminCookie.getValue().equals("false")) // $ hasConditionalBypassTest
 			login(user, password);
-		
-		// FALSE POSITIVES: both methods are conditionally executed, but they probably
+
+		// GOOD: both methods are conditionally executed, but they probably
 		// both perform the security-critical action
-		if(adminCookie.getValue()=="false") {
+		if (adminCookie.getValue() == "false") { // Safe
 			login(user, password);
 		} else {
 			reCheckAuth(user, password);
 		}
-		
+
 		// FALSE NEGATIVE: we have no way of telling that the skipped method is sensitive
-		if(adminCookie.getValue()=="false")
+		if (adminCookie.getValue() == "false") // $ MISSING: $ hasConditionalBypassTest
 			doReallyImportantSecurityWork();
-		
-		// Apache Shiro permissions system
-		String whatDoTheyWantToDo = args[4];
-		Subject subject = SecurityUtils.getSubject();
-		// BAD: permissions decision made using tainted data
-		if(subject.isPermitted("domain:sublevel:" + whatDoTheyWantToDo))
-			doIt();
-		
-		// GOOD: use fixed checks
-		if(subject.isPermitted("domain:sublevel:whatTheMethodDoes"))
-			doIt();
-		
+
 		InetAddress local = InetAddress.getLocalHost();
 		// GOOD: reverse DNS on localhost is fine
 		if (local.getCanonicalHostName().equals("localhost")) {
@@ -63,68 +50,129 @@ class Test {
 			login(user, password);
 		}
 	}
-	
+
 	public static void test(String user, String password) {
 		Cookie adminCookie = getCookies()[0];
 		// GOOD: login always happens
-		if(adminCookie.getValue()=="false")
+		if (adminCookie.getValue() == "false")
 			login(user, password);
 		else {
-			// do something else
 			login(user, password);
 		}
 	}
-	
+
 	public static void test2(String user, String password) {
 		Cookie adminCookie = getCookies()[0];
 		// BAD: login may happen once or twice
-		if(adminCookie.getValue()=="false")
+		if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
 			login(user, password);
 		else {
 			// do something else
+			doIt();
 		}
 		login(user, password);
 	}
-	
+
 	public static void test3(String user, String password) {
 		Cookie adminCookie = getCookies()[0];
-		if(adminCookie.getValue()=="false")
+		// BAD: login may not happen
+		if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
 			login(user, password);
 		else {
 			// do something else
-			// BAD: login may not happen
-			return;
+			doIt();
 		}
+		return;
 	}
-	
+
 	public static void test4(String user, String password) {
 		Cookie adminCookie = getCookies()[0];
 		// GOOD: login always happens
-		if(adminCookie.getValue()=="false") {
+		if (adminCookie.getValue() == "false") {
 			login(user, password);
 			return;
 		}
-		
+
 		// do other things
 		login(user, password);
 		return;
 	}
-	
+
+	public static void test5(String user, String password) throws Exception {
+		Cookie adminCookie = getCookies()[0];
+		// GOOD: exit with Exception if condition is not met
+		if (adminCookie.getValue() == "false") {
+			throw new Exception();
+		}
+
+		login(user, password);
+	}
+
+	public static void test6(String user, String password) {
+		Cookie adminCookie = getCookies()[0];
+		// GOOD: exit with return if condition is not met
+		if (adminCookie.getValue() == "false") {
+			return;
+		}
+
+		login(user, password);
+	}
+
+	public static void test7(String user, String password) {
+		Cookie adminCookie = getCookies()[0];
+		// BAD: login is bypasseable
+		if (adminCookie.getValue() == "false") { // $ hasConditionalBypassTest
+			login(user, password);
+			return;
+		} else {
+			doIt();
+		}
+	}
+
+	public static void test8(String user, String password) {
+		Cookie adminCookie = getCookies()[0];
+		{
+			// BAD: login may not happen
+			if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
+				authorize(user, password);
+			else {
+				// do something else
+				doIt();
+			}
+		}
+		{
+			// obtainAuthor is not sensitive, so this is safe
+			if (adminCookie.getValue() == "false")
+				obtainAuthor();
+			else {
+				doIt();
+			}
+		}
+	}
+
 	public static void login(String user, String password) {
 		// login
 	}
-	
+
 	public static void reCheckAuth(String user, String password) {
 		// login
 	}
-	
+
+	public static void authorize(String user, String password) {
+		// login
+	}
+
+	public static String obtainAuthor() {
+		return "";
+	}
+
 	public static Cookie[] getCookies() {
 		// get cookies from a servlet
 		return new Cookie[0];
 	}
-	
+
 	public static void doIt() {}
-	
+
 	public static void doReallyImportantSecurityWork() {
 		// login, authenticate, everything
 	}

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

@@ -0,0 +1,20 @@
+import java
+import semmle.code.java.security.ConditionalBypassQuery
+import TestUtilities.InlineExpectationsTest
+
+class ConditionalBypassTest extends InlineExpectationsTest {
+  ConditionalBypassTest() { this = "ConditionalBypassTest" }
+
+  override string getARelevantTag() { result = "hasConditionalBypassTest" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasConditionalBypassTest" and
+    exists(DataFlow::Node src, DataFlow::Node sink, ConditionalBypassFlowConfig conf |
+      conf.hasFlow(src, sink)
+    |
+      sink.getLocation() = location and
+      element = sink.toString() and
+      value = ""
+    )
+  }
+}

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

@@ -1,8 +1,8 @@
 edges
-| Test.java:17:26:17:38 | args : String[] | Test.java:50:26:50:64 | ... + ... |
+| TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... |
 nodes
-| Test.java:17:26:17:38 | args : String[] | semmle.label | args : String[] |
-| Test.java:50:26:50:64 | ... + ... | semmle.label | ... + ... |
+| TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... | semmle.label | ... + ... |
 subpaths
 #select
-| 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 |
+| 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 |

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

@@ -0,0 +1,25 @@
+// Test case for CWE-807 (Reliance on Untrusted Inputs in a Security Decision)
+// http://cwe.mitre.org/data/definitions/807.html
+package test.cwe807.semmle.tests;
+
+import javax.servlet.http.HttpServletRequest;
+import org.apache.shiro.SecurityUtils;
+import org.apache.shiro.subject.Subject;
+
+class TaintedPermissionsCheckTest {
+	public static void main(HttpServletRequest request) throws Exception {
+		// Apache Shiro permissions system
+		String action = request.getParameter("action");
+		Subject subject = SecurityUtils.getSubject();
+		// BAD: permissions decision made using tainted data
+		if (subject.isPermitted("domain:sublevel:" + action))
+			doIt();
+
+		// GOOD: use fixed checks
+		if (subject.isPermitted("domain:sublevel:whatTheMethodDoes"))
+			doIt();
+	}
+
+	public static void doIt() {}
+
+}