port experimental cookie models to non-experimental

Author: erik-krogh

javascript/ql/lib/semmle/javascript/frameworks/CookieLibraries.qll

@@ -24,9 +24,20 @@ module CookieWrites {
 
     /**
      * Holds if the cookie is likely an authentication cookie or otherwise sensitive.
+     * Can never hold for client-side cookies. TODO: Or can it...?
      */
     abstract predicate isSensitive();
   }
+
+  /**
+   * The flag that indicates that a cookie is secure.
+   */
+  string secure() { result = "secure" }
+
+  /**
+   * The flag that indicates that a cookie is HttpOnly.
+   */
+  string httpOnly() { result = "httpOnly" }
 }
 
 /**
@@ -50,13 +61,21 @@ private module JsCookie {
     }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
-    // TODO: CookieWrite
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode, CookieWrites::CookieWrite {
     WriteAccess() { this = libMemberCall("set") }
 
     string getKey() { getArgument(0).mayHaveStringValue(result) }
 
     override DataFlow::Node getValue() { result = getArgument(1) }
+
+    override predicate isSecure() {
+      // A cookie is secure if there are cookie options with the `secure` flag set to `true`.
+      this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
+    }
+
+    override predicate isSensitive() { none() } // TODO: Maybe it can be sensitive?
+
+    override predicate isHttpOnly() { none() } // js-cookie is browser side library and doesn't support HttpOnly
   }
 }
 
@@ -117,3 +136,158 @@ private module LibCookie {
     override DataFlow::Node getValue() { result = getArgument(1) }
   }
 }
+
+/**
+ * A model of cookies in an express application.
+ */
+private module ExpressCookies {
+  /**
+   * A cookie set using `response.cookie` from `express` module (https://expressjs.com/en/api.html#res.cookie).
+   */
+  private class InsecureExpressCookieResponse extends CookieWrites::CookieWrite,
+    DataFlow::MethodCallNode {
+    InsecureExpressCookieResponse() { this.asExpr() instanceof Express::SetCookie }
+
+    override predicate isSecure() {
+      // A cookie is secure if there are cookie options with the `secure` flag set to `true`.
+      // The default is `false`.
+      this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
+    }
+
+    override predicate isSensitive() {
+      HeuristicNames::nameIndicatesSensitiveData(any(string s |
+          this.getArgument(0).mayHaveStringValue(s)
+        ), _)
+      or
+      this.getArgument(0).asExpr() instanceof SensitiveExpr
+    }
+
+    override predicate isHttpOnly() {
+      // A cookie is httpOnly if there are cookie options with the `httpOnly` flag set to `true`.
+      // The default is `false`.
+      this.getOptionArgument(2, CookieWrites::httpOnly()).mayHaveBooleanValue(true)
+    }
+  }
+
+  /**
+   * A cookie set using the `express` module `cookie-session` (https://github.com/expressjs/cookie-session).
+   */
+  class InsecureCookieSession extends ExpressLibraries::CookieSession::MiddlewareInstance,
+    CookieWrites::CookieWrite {
+    private DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getOptionArgument(0, flag)
+    }
+
+    override predicate isSecure() {
+      // The flag `secure` is set to `false` by default for HTTP, `true` by default for HTTPS (https://github.com/expressjs/cookie-session#cookie-options).
+      // A cookie is secure if the `secure` flag is not explicitly set to `false`.
+      not getCookieFlagValue(CookieWrites::secure()).mayHaveBooleanValue(false)
+    }
+
+    override predicate isSensitive() {
+      any() // It is a session cookie, likely auth sensitive
+    }
+
+    override predicate isHttpOnly() {
+      // The flag `httpOnly` is set to `true` by default (https://github.com/expressjs/cookie-session#cookie-options).
+      // A cookie is httpOnly if the `httpOnly` flag is not explicitly set to `false`.
+      not getCookieFlagValue(CookieWrites::httpOnly()).mayHaveBooleanValue(false)
+    }
+  }
+
+  /**
+   * A cookie set using the `express` module `express-session` (https://github.com/expressjs/session).
+   */
+  class InsecureExpressSessionCookie extends ExpressLibraries::ExpressSession::MiddlewareInstance,
+    CookieWrites::CookieWrite {
+    private DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getOption("cookie").getALocalSource().getAPropertyWrite(flag).getRhs()
+    }
+
+    override predicate isSecure() {
+      // The flag `secure` is not set by default (https://github.com/expressjs/session#Cookiesecure).
+      // The default value for cookie options is { path: '/', httpOnly: true, secure: false, maxAge: null }.
+      // A cookie is secure if there are the cookie options with the `secure` flag set to `true` or to `auto`.
+      getCookieFlagValue(CookieWrites::secure()).mayHaveBooleanValue(true) or
+      getCookieFlagValue(CookieWrites::secure()).mayHaveStringValue("auto")
+    }
+
+    override predicate isSensitive() {
+      any() // It is a session cookie, likely auth sensitive
+    }
+
+    override predicate isHttpOnly() {
+      // The flag `httpOnly` is set by default (https://github.com/expressjs/session#Cookiesecure).
+      // The default value for cookie options is { path: '/', httpOnly: true, secure: false, maxAge: null }.
+      // A cookie is httpOnly if the `httpOnly` flag is not explicitly set to `false`.
+      not getCookieFlagValue(CookieWrites::httpOnly()).mayHaveBooleanValue(false)
+    }
+  }
+}
+
+/**
+ * A cookie set using `Set-Cookie` header of an `HTTP` response, where a raw header is used.
+ * (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie).
+ * This class does not model the Express implementation of `HTTP::CookieDefintion`
+ * as the express implementation does not use raw headers.
+ *
+ * In case an array is passed `setHeader("Set-Cookie", [...]` it sets multiple cookies.
+ * We model a `CookieWrite` for each array element.
+ */
+private class HTTPCookieWrite extends CookieWrites::CookieWrite {
+  string header;
+
+  HTTPCookieWrite() {
+    exists(HTTP::CookieDefinition setCookie |
+      this.asExpr() = setCookie.getHeaderArgument() and
+      not this instanceof DataFlow::ArrayCreationNode
+      or
+      this = setCookie.getHeaderArgument().flow().(DataFlow::ArrayCreationNode).getAnElement()
+    ) and
+    header =
+      [
+        any(string s | this.mayHaveStringValue(s)),
+        this.(StringOps::ConcatenationRoot).getConstantStringParts()
+      ]
+  }
+
+  override predicate isSecure() {
+    // A cookie is secure if the `secure` flag is specified in the cookie definition.
+    //  The default is `false`.
+    hasCookieAttribute(header, CookieWrites::secure())
+  }
+
+  override predicate isHttpOnly() {
+    // A cookie is httpOnly if the `httpOnly` flag is specified in the cookie definition.
+    // The default is `false`.
+    hasCookieAttribute(header, CookieWrites::httpOnly())
+  }
+
+  override predicate isSensitive() {
+    HeuristicNames::nameIndicatesSensitiveData(getCookieName(header), _)
+  }
+
+  /**
+   * Gets cookie name from a `Set-Cookie` header value.
+   * The header value always starts with `<cookie-name>=<cookie-value>` optionally followed by attributes:
+   * `<cookie-name>=<cookie-value>; Domain=<domain-value>; Secure; HttpOnly`
+   */
+  bindingset[s]
+  private string getCookieName(string s) {
+    result = s.regexpCapture("\\s*\\b([^=\\s]*)\\b\\s*=.*", 1)
+  }
+
+  /**
+   * Holds if the `Set-Cookie` header value contains the specified attribute
+   * 1. The attribute is case insensitive
+   * 2. It always starts with a pair `<cookie-name>=<cookie-value>`.
+   *    If the attribute is present there must be `;` after the pair.
+   *    Other attributes like `Domain=`, `Path=`, etc. may come after the pair:
+   *    `<cookie-name>=<cookie-value>; Domain=<domain-value>; Secure; HttpOnly`
+   * See `https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie`
+   */
+  bindingset[s, attribute]
+  private predicate hasCookieAttribute(string s, string attribute) {
+    s.regexpMatch("(?i).*;\\s*" + attribute + "\\b\\s*;?.*$")
+  }
+}

javascript/ql/src/experimental/Security/CWE-1004/CookieWithoutHttpOnly.qhelp renamed to javascript/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.qhelp

@@ -13,10 +13,12 @@
  */
 
 import javascript
-import experimental.semmle.javascript.security.InsecureCookie::Cookie as ExperimentalCookie
+import experimental.semmle.javascript.security.InsecureCookie::Cookie as ExperimentalCookie // TODO: Remove.
 
 from DataFlow::Node node
 where
+// TODO: Only for sensitive cookies? (e.g. auth cookies)
+// TODO: Give all descriptions, qlhelp, qldocs, an overhaul. Consider precisions, severity, cwes. 
   exists(ExperimentalCookie::CookieWrite cookie | cookie = node |
     cookie.isSensitive() and not cookie.isHttpOnly()
   )

javascript/ql/src/experimental/Security/CWE-1004/CookieWithoutHttpOnly.ql renamed to javascript/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql

@@ -11,7 +11,7 @@
  */
 
 import javascript
-import experimental.semmle.javascript.security.InsecureCookie::Cookie as ExperimentalCookie
+import experimental.semmle.javascript.security.InsecureCookie::Cookie as ExperimentalCookie // TODO: Remove
 
 from DataFlow::Node node
 where