Merge pull request #9116 from smowton/smowton/feature/accept-conditional-cookie-security

Java: tolerate `cookie.setSecure(request.isSecure())`

Author: smowton

java/ql/lib/semmle/code/java/frameworks/Servlets.qll

@@ -8,7 +8,7 @@ import semmle.code.java.Type
  * The interface `javax.servlet.ServletRequest` or
  * `javax.servlet.http.HttpServletRequest`.
  */
-library class ServletRequest extends RefType {
+class ServletRequest extends RefType {
   ServletRequest() {
     this.hasQualifiedName("javax.servlet", "ServletRequest") or
     this instanceof HttpServletRequest
@@ -18,15 +18,15 @@ library class ServletRequest extends RefType {
 /**
  * The interface `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequest extends RefType {
+class HttpServletRequest extends RefType {
   HttpServletRequest() { this.hasQualifiedName("javax.servlet.http", "HttpServletRequest") }
 }
 
 /**
  * The method `getParameter(String)` or `getParameterValues(String)`
  * declared in `javax.servlet.ServletRequest`.
  */
-library class ServletRequestGetParameterMethod extends Method {
+class ServletRequestGetParameterMethod extends Method {
   ServletRequestGetParameterMethod() {
     this.getDeclaringType() instanceof ServletRequest and
     (
@@ -41,7 +41,7 @@ library class ServletRequestGetParameterMethod extends Method {
 /**
  * The method `getParameterNames()` declared in `javax.servlet.ServletRequest`.
  */
-library class ServletRequestGetParameterNamesMethod extends Method {
+class ServletRequestGetParameterNamesMethod extends Method {
   ServletRequestGetParameterNamesMethod() {
     this.getDeclaringType() instanceof ServletRequest and
     this.hasName("getParameterNames") and
@@ -52,7 +52,7 @@ library class ServletRequestGetParameterNamesMethod extends Method {
 /**
  * The method `getParameterMap()` declared in `javax.servlet.ServletRequest`.
  */
-library class ServletRequestGetParameterMapMethod extends Method {
+class ServletRequestGetParameterMapMethod extends Method {
   ServletRequestGetParameterMapMethod() {
     this.getDeclaringType() instanceof ServletRequest and
     this.hasName("getParameterMap") and
@@ -63,7 +63,7 @@ library class ServletRequestGetParameterMapMethod extends Method {
 /**
  * The method `getQueryString()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetQueryStringMethod extends Method {
+class HttpServletRequestGetQueryStringMethod extends Method {
   HttpServletRequestGetQueryStringMethod() {
     this.getDeclaringType() instanceof HttpServletRequest and
     this.hasName("getQueryString") and
@@ -85,7 +85,7 @@ class HttpServletRequestGetPathMethod extends Method {
 /**
  * The method `getHeader(String)` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetHeaderMethod extends Method {
+class HttpServletRequestGetHeaderMethod extends Method {
   HttpServletRequestGetHeaderMethod() {
     this.getDeclaringType() instanceof HttpServletRequest and
     this.hasName("getHeader") and
@@ -97,7 +97,7 @@ library class HttpServletRequestGetHeaderMethod extends Method {
 /**
  * The method `getHeaders(String)` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetHeadersMethod extends Method {
+class HttpServletRequestGetHeadersMethod extends Method {
   HttpServletRequestGetHeadersMethod() {
     this.getDeclaringType() instanceof HttpServletRequest and
     this.hasName("getHeaders") and
@@ -109,7 +109,7 @@ library class HttpServletRequestGetHeadersMethod extends Method {
 /**
  * The method `getHeaderNames()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetHeaderNamesMethod extends Method {
+class HttpServletRequestGetHeaderNamesMethod extends Method {
   HttpServletRequestGetHeaderNamesMethod() {
     this.getDeclaringType() instanceof HttpServletRequest and
     this.hasName("getHeaderNames") and
@@ -145,7 +145,7 @@ class HttpServletRequestGetRequestURIMethod extends Method {
 /**
  * The method `getRemoteUser()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetRemoteUserMethod extends Method {
+class HttpServletRequestGetRemoteUserMethod extends Method {
   HttpServletRequestGetRemoteUserMethod() {
     this.getDeclaringType() instanceof HttpServletRequest and
     this.hasName("getRemoteUser") and
@@ -156,7 +156,7 @@ library class HttpServletRequestGetRemoteUserMethod extends Method {
 /**
  * The method `getInputStream()` or `getReader()` declared in `javax.servlet.ServletRequest`.
  */
-library class ServletRequestGetBodyMethod extends Method {
+class ServletRequestGetBodyMethod extends Method {
   ServletRequestGetBodyMethod() {
     this.getDeclaringType() instanceof ServletRequest and
     (this.hasName("getInputStream") or this.hasName("getReader"))
@@ -239,14 +239,14 @@ class ServletResponseGetOutputStreamMethod extends Method {
 }
 
 /** The class `javax.servlet.http.Cookie`. */
-library class TypeCookie extends Class {
+class TypeCookie extends Class {
   TypeCookie() { this.hasQualifiedName("javax.servlet.http", "Cookie") }
 }
 
 /**
  * The method `getValue(String)` declared in `javax.servlet.http.Cookie`.
  */
-library class CookieGetValueMethod extends Method {
+class CookieGetValueMethod extends Method {
   CookieGetValueMethod() {
     this.getDeclaringType() instanceof TypeCookie and
     this.hasName("getValue") and
@@ -257,7 +257,7 @@ library class CookieGetValueMethod extends Method {
 /**
  * The method `getName()` declared in `javax.servlet.http.Cookie`.
  */
-library class CookieGetNameMethod extends Method {
+class CookieGetNameMethod extends Method {
   CookieGetNameMethod() {
     this.getDeclaringType() instanceof TypeCookie and
     this.hasName("getName") and
@@ -269,7 +269,7 @@ library class CookieGetNameMethod extends Method {
 /**
  * The method `getComment()` declared in `javax.servlet.http.Cookie`.
  */
-library class CookieGetCommentMethod extends Method {
+class CookieGetCommentMethod extends Method {
   CookieGetCommentMethod() {
     this.getDeclaringType() instanceof TypeCookie and
     this.hasName("getComment") and

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/src/change-notes/2022-05-11-insecure-cookie.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Query `java/insecure-cookie` now tolerates setting a cookie's secure flag to `request.isSecure()`. This means servlets that intentionally accept unencrypted connections will no longer raise an alert.

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);
+		}
+
 	}
 }