Refactor tests to use InlineExpectationsTest

Author: atorralba

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

@@ -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
 		// both perform the security-critical action
-		if(adminCookie.getValue()=="false") {
+		if (adminCookie.getValue() == "false") { // $ SPURIOUS: $ hasConditionalBypassTest
 			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,68 @@ public static void main(String[] args) throws UnknownHostException {
 			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
 		}
 		login(user, password);
 	}
-	
+
 	public static void test3(String user, String password) {
 		Cookie adminCookie = getCookies()[0];
-		if(adminCookie.getValue()=="false")
+		if (adminCookie.getValue() == "false") // $ hasConditionalBypassTest
 			login(user, password);
 		else {
 			// do something else
 			// BAD: login may not happen
 			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 login(String user, String password) {
 		// login
 	}
-	
+
 	public static void reCheckAuth(String user, String password) {
 		// login
 	}
-	
+
 	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/ConditionalBypass.qlref

@@ -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/ConditionalBypassTest.expected

@@ -1,8 +1,7 @@
 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 | ... + ... |
-subpaths
+| TaintedPermissionsCheckTest.java:12:19:12:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| TaintedPermissionsCheckTest.java:15:27:15:53 | ... + ... | semmle.label | ... + ... |
 #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/Test.java renamed to java/ql/test/query-tests/security/CWE-807/semmle/tests/ConditionalBypassTest.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() {}
+
+}