Insecure cookie query: accept ServletRequest.isSecure(), and allow more than one possible input to a setSecure(...) call.

Author: smowton

java/ql/src/Security/CWE/CWE-614/InsecureCookie.ql

@@ -13,14 +13,31 @@
 
 import java
 import semmle.code.java.frameworks.Servlets
+import semmle.code.java.dataflow.DataFlow
+
+predicate isSafeSecureCookieSetting(Expr e) {
+  e.(CompileTimeConstantExpr).getBooleanValue() = true
+  or
+  exists(Method isSecure |
+    isSecure.getName() = "isSecure" and
+    isSecure.getDeclaringType().getASourceSupertype*() instanceof ServletRequest
+  |
+    e.(MethodAccess).getMethod() = isSecure
+  )
+}
 
 from MethodAccess add
 where
   add.getMethod() instanceof ResponseAddCookieMethod and
   not exists(Variable cookie, MethodAccess m |
     add.getArgument(0) = cookie.getAnAccess() and
     m.getMethod().getName() = "setSecure" and
-    m.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
+    forex(DataFlow::Node argSource |
+      DataFlow::localFlow(argSource, DataFlow::exprNode(m.getArgument(0))) and
+      not DataFlow::localFlowStep(_, argSource)
+    |
+      isSafeSecureCookieSetting(argSource.asExpr())
+    ) and
     m.getQualifier() = cookie.getAnAccess()
   )
 select add, "Cookie is added to response without the 'secure' flag being set."

java/ql/test/query-tests/security/CWE-311/CWE-614/semmle/tests/InsecureCookie.expected

@@ -1 +1,4 @@
-| Test.java:16:4:16:29 | addCookie(...) | Cookie is added to response without the 'secure' flag being set. |
+| Test.java:19:4:19:29 | addCookie(...) | Cookie is added to response without the 'secure' flag being set. |
+| Test.java:28:4:28:29 | addCookie(...) | Cookie is added to response without the 'secure' flag being set. |
+| Test.java:37:4:37:29 | addCookie(...) | Cookie is added to response without the 'secure' flag being set. |
+| Test.java:51:4:51:29 | addCookie(...) | Cookie is added to response without the 'secure' flag being set. |

java/ql/test/query-tests/security/CWE-311/CWE-614/semmle/tests/Test.java

@@ -8,21 +8,81 @@
 import javax.servlet.http.*;
 
 class Test {
-	public static void test(HttpServletRequest request, HttpServletResponse response) {
+
+	public static final boolean constTrue = true;
+
+	public static void test(HttpServletRequest request, HttpServletResponse response, boolean alwaysSecure, boolean otherInput) {
 		{
 			Cookie cookie = new Cookie("secret" ,"fakesecret");
-			
+
 			// BAD: secure flag not set
 			response.addCookie(cookie);
-			
+
 		}
-		
+
 		{
 			Cookie cookie = new Cookie("secret" ,"fakesecret");
-			
+
+			// BAD: secure flag set to false
+			cookie.setSecure(false);
+			response.addCookie(cookie);
+
+		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
+			// BAD: secure flag set to something not clearly true or request.isSecure()
+			cookie.setSecure(otherInput);
+			response.addCookie(cookie);
+
+		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
+			// BAD: secure flag sometimes set to something clearly true or request.isSecure()
+			boolean secureVal;
+			if(alwaysSecure)
+				secureVal = true;
+			else
+				secureVal = otherInput;
+			cookie.setSecure(secureVal);
+			response.addCookie(cookie);
+
+		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
+			// GOOD: set secure flag
+			cookie.setSecure(true);
+			response.addCookie(cookie);
+		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
 			// GOOD: set secure flag
 			cookie.setSecure(true);
 			response.addCookie(cookie);
 		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
+			// GOOD: set secure flag
+			cookie.setSecure(constTrue);
+			response.addCookie(cookie);
+		}
+
+		{
+			Cookie cookie = new Cookie("secret" ,"fakesecret");
+
+			// GOOD: set secure flag if contacted over HTTPS
+			cookie.setSecure(alwaysSecure ? true : request.isSecure());
+			response.addCookie(cookie);
+		}
+
 	}
 }