Revamp source of the query

luchua-bc · luchua-bc · commit b366ffa69e98 · 2021-03-03T13:38:18.000Z
diff --git a/java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp b/java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp
@@ -2,7 +2,7 @@
 <qhelp>
 
   <overview>
-    <p>Cross-Site Scripting (XSS) is categorized as one of the OWASP Top 10 Security Vulnerabilities. The <code>HttpOnly</code> flag directs compatible browsers to prevent client-side script from accessing cookies. Including the HttpOnly flag in the Set-Cookie HTTP response header for a sensitive cookie helps mitigate the risk associated with XSS where an attacker's script code attempts to read the contents of a cookie and exfiltrate information obtained.</p>
+    <p>Cross-Site Scripting (XSS) is categorized as one of the OWASP Top 10 Security Vulnerabilities. The <code>HttpOnly</code> flag directs compatible browsers to prevent client-side script from accessing cookies. Including the <code>HttpOnly</code> flag in the Set-Cookie HTTP response header for a sensitive cookie helps mitigate the risk associated with XSS where an attacker's script code attempts to read the contents of a cookie and exfiltrate information obtained.</p>
   </overview>
 
   <recommendation>
diff --git a/java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql b/java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql
@@ -9,7 +9,6 @@
 
 import java
 import semmle.code.java.frameworks.Servlets
-import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.dataflow.TaintTracking
 import DataFlow::PathGraph
 
@@ -18,16 +17,16 @@ string getSensitiveCookieNameRegex() { result = "(?i).*(auth|session|token|key|c
 
 /** Holds if a string is concatenated with the name of a sensitive cookie. */
 predicate isSensitiveCookieNameExpr(Expr expr) {
-  expr.(StringLiteral)
-      .getRepresentedString()
+  expr.(CompileTimeConstantExpr)
+      .getStringValue()
       .toLowerCase()
       .regexpMatch(getSensitiveCookieNameRegex()) or
   isSensitiveCookieNameExpr(expr.(AddExpr).getAnOperand())
 }
 
 /** Holds if a string is concatenated with the `HttpOnly` flag. */
 predicate hasHttpOnlyExpr(Expr expr) {
-  expr.(StringLiteral).getRepresentedString().toLowerCase().matches("%httponly%") or
+  expr.(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%") or
   hasHttpOnlyExpr(expr.(AddExpr).getAnOperand())
 }
 
@@ -38,37 +37,34 @@ class SetCookieMethodAccess extends MethodAccess {
       this.getMethod() instanceof ResponseAddHeaderMethod or
       this.getMethod() instanceof ResponseSetHeaderMethod
     ) and
-    this.getArgument(0).(StringLiteral).getRepresentedString().toLowerCase() = "set-cookie"
+    this.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "set-cookie"
   }
 }
 
 /** Sensitive cookie name used in a `Cookie` constructor or a `Set-Cookie` call. */
 class SensitiveCookieNameExpr extends Expr {
   SensitiveCookieNameExpr() {
-    isSensitiveCookieNameExpr(this) and
-    (
-      exists(
-        ClassInstanceExpr cie // new Cookie("jwt_token", token)
-      |
-        (
-          cie.getConstructor().getDeclaringType().hasQualifiedName("javax.servlet.http", "Cookie") or
-          cie.getConstructor()
-              .getDeclaringType()
-              .getASupertype*()
-              .hasQualifiedName("javax.ws.rs.core", "Cookie") or
-          cie.getConstructor()
-              .getDeclaringType()
-              .getASupertype*()
-              .hasQualifiedName("jakarta.ws.rs.core", "Cookie")
-        ) and
-        DataFlow::localExprFlow(this, cie.getArgument(0))
-      )
-      or
-      exists(
-        SetCookieMethodAccess ma // response.addHeader("Set-Cookie: token=" +authId + ";HttpOnly;Secure")
-      |
-        DataFlow::localExprFlow(this, ma.getArgument(1))
-      )
+    exists(
+      ClassInstanceExpr cie, Expr e // new Cookie("jwt_token", token)
+    |
+      (
+        cie.getConstructor().getDeclaringType().hasQualifiedName("javax.servlet.http", "Cookie") or
+        cie.getConstructor()
+            .getDeclaringType()
+            .getASupertype*()
+            .hasQualifiedName(["javax.ws.rs.core", "jakarta.ws.rs.core"], "Cookie")
+      ) and
+      this = cie and
+      isSensitiveCookieNameExpr(e) and
+      DataFlow::localExprFlow(e, cie.getArgument(0))
+    )
+    or
+    exists(
+      SetCookieMethodAccess ma, Expr e // response.addHeader("Set-Cookie: token=" +authId + ";HttpOnly;Secure")
+    |
+      this = ma.getArgument(1) and
+      isSensitiveCookieNameExpr(e) and
+      DataFlow::localExprFlow(e, ma.getArgument(1))
     )
   }
 }
@@ -78,15 +74,20 @@ class CookieResponseSink extends DataFlow::ExprNode {
   CookieResponseSink() {
     exists(MethodAccess ma |
       (
-        ma.getMethod() instanceof ResponseAddCookieMethod or
-        ma instanceof SetCookieMethodAccess
-      ) and
-      ma.getAnArgument() = this.getExpr()
+        ma.getMethod() instanceof ResponseAddCookieMethod and
+        this.getExpr() = ma.getArgument(0)
+        or
+        ma instanceof SetCookieMethodAccess and
+        this.getExpr() = ma.getArgument(1)
+      )
     )
   }
 }
 
-/** Holds if the `node` is a method call of `setHttpOnly(true)` on a cookie. */
+/**
+ * Holds if `node` is an access to a variable which has `setHttpOnly(true)` called on it and is also
+ * the first argument to a call to the method `addCookie` of `javax.servlet.http.HttpServletResponse`.
+ */
 predicate setHttpOnlyMethodAccess(DataFlow::Node node) {
   exists(
     MethodAccess addCookie, Variable cookie, MethodAccess m // jwtCookie.setHttpOnly(true)
@@ -100,28 +101,28 @@ predicate setHttpOnlyMethodAccess(DataFlow::Node node) {
   )
 }
 
-/** Holds if the `node` is a method call of `Set-Cookie` header with the `HttpOnly` flag whose cookie name is sensitive. */
+/**
+ * Holds if `node` is a string that contains `httponly` and which flows to the second argument
+ * of a method to set a cookie.
+ */
 predicate setHttpOnlyInSetCookie(DataFlow::Node node) {
   exists(SetCookieMethodAccess sa |
     hasHttpOnlyExpr(node.asExpr()) and
     DataFlow::localExprFlow(node.asExpr(), sa.getArgument(1))
   )
 }
 
-/** Holds if the `node` is an invocation of a JAX-RS `NewCookie` constructor that sets `HttpOnly` to true. */
-predicate setHttpOnlyInNewCookie(DataFlow::Node node) {
-  exists(ClassInstanceExpr cie |
-    cie.getConstructor().getDeclaringType().hasName("NewCookie") and
-    DataFlow::localExprFlow(node.asExpr(), cie.getArgument(0)) and
-    (
-      cie.getNumArgument() = 6 and cie.getArgument(5).(BooleanLiteral).getBooleanValue() = true // NewCookie(Cookie cookie, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
-      or
-      cie.getNumArgument() = 8 and
-      cie.getArgument(6).getType() instanceof BooleanType and
-      cie.getArgument(7).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly)
-      or
-      cie.getNumArgument() = 10 and cie.getArgument(9).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, int version, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
-    )
+/** Holds if `cie` is an invocation of a JAX-RS `NewCookie` constructor that sets `HttpOnly` to true. */
+predicate setHttpOnlyInNewCookie(ClassInstanceExpr cie) {
+  cie.getConstructedType().hasQualifiedName(["javax.ws.rs.core", "jakarta.ws.rs.core"], "NewCookie") and
+  (
+    cie.getNumArgument() = 6 and cie.getArgument(5).(BooleanLiteral).getBooleanValue() = true // NewCookie(Cookie cookie, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
+    or
+    cie.getNumArgument() = 8 and
+    cie.getArgument(6).getType() instanceof BooleanType and
+    cie.getArgument(7).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly)
+    or
+    cie.getNumArgument() = 10 and cie.getArgument(9).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, int version, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
   )
 }
 
@@ -145,7 +146,10 @@ predicate isTestMethod(DataFlow::Node node) {
   )
 }
 
-/** A taint configuration tracking flow from a sensitive cookie without HttpOnly flag set to its HTTP response. */
+/**
+ * A taint configuration tracking flow from a sensitive cookie without the `HttpOnly` flag
+ * set to its HTTP response.
+ */
 class MissingHttpOnlyConfiguration extends TaintTracking::Configuration {
   MissingHttpOnlyConfiguration() { this = "MissingHttpOnlyConfiguration" }
 
@@ -159,25 +163,17 @@ class MissingHttpOnlyConfiguration extends TaintTracking::Configuration {
     // cookie.setHttpOnly(true)
     setHttpOnlyMethodAccess(node)
     or
-    // response.addHeader("Set-Cookie: token=" +authId + ";HttpOnly;Secure")
+    // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure")
     setHttpOnlyInSetCookie(node)
     or
     // new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true)
-    setHttpOnlyInNewCookie(node)
+    setHttpOnlyInNewCookie(node.asExpr())
     or
     // Test class or method
     isTestMethod(node)
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(
-      ClassInstanceExpr cie // `NewCookie` constructor
-    |
-      cie.getAnArgument() = pred.asExpr() and
-      cie = succ.asExpr() and
-      cie.getConstructor().getDeclaringType().hasName("NewCookie")
-    )
-    or
     exists(
       MethodAccess ma // `toString` call on a cookie object
     |
diff --git a/java/ql/test/experimental/query-tests/security/CWE-1004/SensitiveCookieNotHttpOnly.expected b/java/ql/test/experimental/query-tests/security/CWE-1004/SensitiveCookieNotHttpOnly.expected
@@ -1,13 +1,13 @@
 edges
-| SensitiveCookieNotHttpOnly.java:22:38:22:48 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie |
-| SensitiveCookieNotHttpOnly.java:49:56:49:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) |
+| SensitiveCookieNotHttpOnly.java:22:27:22:60 | new Cookie(...) : Cookie | SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie |
+| SensitiveCookieNotHttpOnly.java:49:42:49:113 | new NewCookie(...) : NewCookie | SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) |
 nodes
-| SensitiveCookieNotHttpOnly.java:22:38:22:48 | "jwt_token" : String | semmle.label | "jwt_token" : String |
+| SensitiveCookieNotHttpOnly.java:22:27:22:60 | new Cookie(...) : Cookie | semmle.label | new Cookie(...) : Cookie |
 | SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie | semmle.label | jwtCookie |
 | SensitiveCookieNotHttpOnly.java:39:42:39:69 | ... + ... | semmle.label | ... + ... |
+| SensitiveCookieNotHttpOnly.java:49:42:49:113 | new NewCookie(...) : NewCookie | semmle.label | new NewCookie(...) : NewCookie |
 | SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) | semmle.label | toString(...) |
-| SensitiveCookieNotHttpOnly.java:49:56:49:75 | "session-access-key" : String | semmle.label | "session-access-key" : String |
 #select
-| SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie | SensitiveCookieNotHttpOnly.java:22:38:22:48 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:22:38:22:48 | "jwt_token" | This sensitive cookie |
+| SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie | SensitiveCookieNotHttpOnly.java:22:27:22:60 | new Cookie(...) : Cookie | SensitiveCookieNotHttpOnly.java:28:28:28:36 | jwtCookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:22:27:22:60 | new Cookie(...) | This sensitive cookie |
 | SensitiveCookieNotHttpOnly.java:39:42:39:69 | ... + ... | SensitiveCookieNotHttpOnly.java:39:42:39:69 | ... + ... | SensitiveCookieNotHttpOnly.java:39:42:39:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:39:42:39:69 | ... + ... | This sensitive cookie |
-| SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) | SensitiveCookieNotHttpOnly.java:49:56:49:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:49:56:49:75 | "session-access-key" | This sensitive cookie |
+| SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) | SensitiveCookieNotHttpOnly.java:49:42:49:113 | new NewCookie(...) : NewCookie | SensitiveCookieNotHttpOnly.java:49:42:49:124 | toString(...) | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:49:42:49:113 | new NewCookie(...) | This sensitive cookie |
diff --git a/java/ql/test/stubs/jsr311-api-1.1.1/javax/ws/rs/core/Cookie.java b/java/ql/test/stubs/jsr311-api-1.1.1/javax/ws/rs/core/Cookie.java
@@ -51,10 +51,16 @@
  * @since 1.0
  */
 public class Cookie {
+
     /**
      * Cookies using the default version correspond to RFC 2109.
      */
     public static final int DEFAULT_VERSION = 1;
+    private final String name;
+    private final String value;
+    private final int version;
+    private final String path;
+    private final String domain;
 
     /**
      * Create a new instance.
@@ -68,6 +74,11 @@ public class Cookie {
      */
     public Cookie(final String name, final String value, final String path, final String domain, final int version)
             throws IllegalArgumentException {
+        this.name = name;
+        this.value = value;
+        this.version = version;
+        this.domain = domain;
+        this.path = path;
     }
 
     /**
@@ -81,6 +92,7 @@ public Cookie(final String name, final String value, final String path, final St
      */
     public Cookie(final String name, final String value, final String path, final String domain)
             throws IllegalArgumentException {
+        this(name, value, path, domain, DEFAULT_VERSION);
     }
 
     /**
@@ -92,6 +104,7 @@ public Cookie(final String name, final String value, final String path, final St
      */
     public Cookie(final String name, final String value)
             throws IllegalArgumentException {
+        this(name, value, null, null);
     }
 
     /**
@@ -112,7 +125,7 @@ public static Cookie valueOf(final String value) {
      * @return the cookie name.
      */
     public String getName() {
-        return null;
+        return name;
     }
 
     /**
@@ -121,7 +134,7 @@ public String getName() {
      * @return the cookie value.
      */
     public String getValue() {
-        return null;
+        return value;
     }
 
     /**
@@ -130,7 +143,7 @@ public String getValue() {
      * @return the cookie version.
      */
     public int getVersion() {
-        return -1;
+        return version;
     }
 
     /**
@@ -139,7 +152,7 @@ public int getVersion() {
      * @return the cookie domain.
      */
     public String getDomain() {
-        return null;
+        return domain;
     }
 
     /**
@@ -148,40 +161,6 @@ public String getDomain() {
      * @return the cookie path.
      */
     public String getPath() {
-        return null;
-    }
-
-    /**
-     * Convert the cookie to a string suitable for use as the value of the
-     * corresponding HTTP header.
-     *
-     * @return a stringified cookie.
-     */
-    @Override
-    public String toString() {
-        return null;
-    }
-
-    /**
-     * Generate a hash code by hashing all of the cookies properties.
-     *
-     * @return the cookie hash code.
-     */
-    @Override
-    public int hashCode() {
-        return -1;
-    }
-
-    /**
-     * Compare for equality.
-     *
-     * @param obj the object to compare to.
-     * @return {@code true}, if the object is a {@code Cookie} with the same
-     *         value for all properties, {@code false} otherwise.
-     */
-    @SuppressWarnings({"StringEquality", "RedundantIfStatement"})
-    @Override
-    public boolean equals(final Object obj) {
-        return true;
+        return path;
     }
-}
+}
diff --git a/java/ql/test/stubs/jsr311-api-1.1.1/javax/ws/rs/core/NewCookie.java b/java/ql/test/stubs/jsr311-api-1.1.1/javax/ws/rs/core/NewCookie.java
@@ -320,40 +320,4 @@ public boolean isHttpOnly() {
     public Cookie toCookie() {
         return null;
     }
-
-    /**
-     * Convert the cookie to a string suitable for use as the value of the
-     * corresponding HTTP header.
-     *
-     * @return a stringified cookie.
-     */
-    @Override
-    public String toString() {
-        return null;
-    }
-
-    /**
-     * Generate a hash code by hashing all of the properties.
-     *
-     * @return the hash code.
-     */
-    @Override
-    public int hashCode() {
-        return -1;
-    }
-
-    /**
-     * Compare for equality. Use {@link #toCookie()} to compare a
-     * {@code NewCookie} to a {@code Cookie} considering only the common
-     * properties.
-     *
-     * @param obj the object to compare to
-     * @return true if the object is a {@code NewCookie} with the same value for
-     *         all properties, false otherwise.
-     */
-    @SuppressWarnings({"StringEquality", "RedundantIfStatement"})
-    @Override
-    public boolean equals(Object obj) {
-        return true;
-    }
 }
