Merge pull request #6855 from erik-krogh/secCookie

JS: Move cookie queries out of experimental.

Author: erik-krogh

javascript/change-notes/2021-10-26-cookie-queries.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/clear-text-cookie` and `js/client-exposed-cookie` queries have been renamed and moved into non-experimental.

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

@@ -4,6 +4,97 @@
 
 import javascript
 
+/**
+ * Classes and predicates for reasoning about writes to cookies.
+ */
+module CookieWrites {
+  /**
+   * A write to a cookie.
+   */
+  abstract class CookieWrite extends DataFlow::Node {
+    /**
+     * Holds if this cookie is secure, i.e. only transmitted over SSL.
+     */
+    abstract predicate isSecure();
+
+    /**
+     * Holds if this cookie is HttpOnly, i.e. not accessible by JavaScript.
+     */
+    abstract predicate isHttpOnly();
+
+    /**
+     * Holds if the cookie likely is an authentication cookie or otherwise sensitive.
+     */
+    abstract predicate isSensitive();
+
+    /**
+     * Holds if the cookie write happens on a server, i.e. the `httpOnly` flag is relevant.
+     */
+    predicate isServerSide() {
+      any() // holds by default. Client-side cookie writes should extend ClientSideCookieWrite.
+    }
+  }
+
+  /**
+   * A client-side write to a cookie.
+   */
+  abstract class ClientSideCookieWrite extends CookieWrite {
+    final override predicate isHttpOnly() { none() }
+
+    final override predicate isServerSide() { none() }
+  }
+
+  /**
+   * 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" }
+}
+
+/**
+ * Holds if `node` looks like it can contain a sensitive cookie.
+ *
+ * Heuristics:
+ * - `node` contains a string value that looks like a sensitive cookie name
+ * - `node` is a sensitive expression
+ */
+private predicate canHaveSensitiveCookie(DataFlow::Node node) {
+  exists(string s |
+    node.mayHaveStringValue(s) or
+    s = node.(StringOps::ConcatenationRoot).getConstantStringParts()
+  |
+    HeuristicNames::nameIndicatesSensitiveData([s, getCookieName(s)], _)
+  )
+  or
+  node.asExpr() instanceof SensitiveExpr
+}
+
+/**
+ * Gets the cookie name of 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("([^=]*)=.*", 1).trim() }
+
+/**
+ * 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*;?.*$")
+}
+
 /**
  * A model of the `js-cookie` library (https://github.com/js-cookie/js-cookie).
  */
@@ -25,12 +116,22 @@ private module JsCookie {
     }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode,
+    CookieWrites::ClientSideCookieWrite {
     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`.
+      exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
+      )
+    }
+
+    override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
   }
 }
 
@@ -53,12 +154,25 @@ private module BrowserCookies {
     }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode,
+    CookieWrites::ClientSideCookieWrite {
     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`.
+      exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
+      )
+      or
+      // or, an explicit default has been set
+      exists(DataFlow::moduleMember("browser-cookies", "defaults").getAPropertyWrite("secure"))
+    }
+
+    override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
   }
 }
 
@@ -81,11 +195,174 @@ private module LibCookie {
     override PersistentWriteAccess getAWrite() { key = result.(WriteAccess).getKey() }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode,
+    CookieWrites::ClientSideCookieWrite {
     WriteAccess() { this = libMemberCall("serialize") }
 
     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`.
+      exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
+      )
+    }
+
+    override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
+  }
+}
+
+/**
+ * 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`.
+      exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
+      )
+    }
+
+    override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
+
+    override predicate isHttpOnly() {
+      // A cookie is httpOnly if there are cookie options with the `httpOnly` flag set to `true`.
+      // The default is `false`.
+      exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::httpOnly()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe 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 }.
+      exists(DataFlow::Node value | value = getCookieFlagValue(CookieWrites::secure()) |
+        not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
+      )
+    }
+
+    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() { canHaveSensitiveCookie(this) }
+}
+
+/**
+ * A write to `document.cookie`.
+ */
+private class DocumentCookieWrite extends CookieWrites::ClientSideCookieWrite {
+  string cookie;
+  DataFlow::PropWrite write;
+
+  DocumentCookieWrite() {
+    this = write and
+    write = DOM::documentRef().getAPropertyWrite("cookie") and
+    cookie =
+      [
+        any(string s | write.getRhs().mayHaveStringValue(s)),
+        write.getRhs().(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(cookie, CookieWrites::secure())
+  }
+
+  override predicate isSensitive() { canHaveSensitiveCookie(write.getRhs()) }
 }

javascript/ql/lib/semmle/javascript/security/SensitiveActions.qll

@@ -14,11 +14,14 @@ import semmle.javascript.security.internal.SensitiveDataHeuristics
 private import HeuristicNames
 
 /** An expression that might contain sensitive data. */
+cached
 abstract class SensitiveExpr extends Expr {
   /** Gets a human-readable description of this expression for use in alert messages. */
+  cached
   abstract string describe();
 
   /** Gets a classification of the kind of sensitive data this expression might contain. */
+  cached
   abstract SensitiveDataClassification getClassification();
 }
 

javascript/ql/lib/semmle/javascript/security/internal/SensitiveDataHeuristics.qll

@@ -58,7 +58,7 @@ module HeuristicNames {
    */
   string maybeAccountInfo() {
     result = "(?is).*acc(ou)?nt.*" or
-    result = "(?is).*(puid|username|userid).*" or
+    result = "(?is).*(puid|username|userid|session(id|key)).*" or
     result = "(?s).*([uU]|^|_|[a-z](?=U))([uU][iI][dD]).*"
   }
 

javascript/ql/src/Security/CWE-1004/ClientExposedCookie.qhelp

@@ -0,0 +1,40 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Authentication cookies stored by a server can be accessed by a client if the <code>httpOnly</code> flag is not set.
+</p>
+<p>
+An attacker that manages a cross-site scripting (XSS) attack can read the cookie and hijack the session.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Set the <code>httpOnly</code> flag on all cookies that are not needed by the client.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example stores an authentication token in a cookie that can 
+be viewed by the client.
+</p>
+<sample src="examples/ClientExposedCookieGood.js"/>
+<p>
+To force the cookie to be transmitted using SSL, set the <code>secure</code>
+attribute on the cookie.
+</p>
+<sample src="examples/ClientExposedCookieBad.js"/>
+</example>
+
+<references>
+<li>ExpressJS: <a href="https://expressjs.com/en/advanced/best-practice-security.html#use-cookies-securely">Use cookies securely</a>.</li>
+<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Security_Cheat_Sheet.html#set-cookie-flags-appropriately">Set cookie flags appropriately</a>.</li>
+<li>Mozilla: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a>.</li>
+</references>
+
+</qhelp>