add more cookie models

Author: erik-krogh

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

@@ -59,7 +59,6 @@ module CookieWrites {
 /**
  * A model of the `js-cookie` library (https://github.com/js-cookie/js-cookie).
  */
-// TODO: Writes to document.cookie.
 private module JsCookie {
   /**
    * Gets a function call that invokes method `name` of the `js-cookie` library.
@@ -118,13 +117,27 @@ private module BrowserCookies {
     }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
-    // TODO: CookieWrite
+  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`.
+      this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
+      or
+      // or, an explicit default has been set
+      exists(DataFlow::moduleMember("browser-cookies", "defaults").getAPropertyWrite("secure"))
+    }
+
+    override predicate isSensitive() {
+      HeuristicNames::nameIndicatesSensitiveData(any(string s |
+          this.getArgument(0).mayHaveStringValue(s)
+        ), _)
+    }
   }
 }
 
@@ -147,13 +160,24 @@ private module LibCookie {
     override PersistentWriteAccess getAWrite() { key = result.(WriteAccess).getKey() }
   }
 
-  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
-    // TODO: CookieWrite
+  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`.
+      this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
+    }
+
+    override predicate isSensitive() {
+      HeuristicNames::nameIndicatesSensitiveData(any(string s |
+          this.getArgument(0).mayHaveStringValue(s)
+        ), _)
+    }
   }
 }
 
@@ -286,28 +310,56 @@ private class HTTPCookieWrite extends CookieWrites::CookieWrite {
   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)
+/**
+ * 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*;?.*$")
+}
+
+/**
+ * A write to `document.cookie`.
+ */
+private class DocumentCookieWrite extends CookieWrites::ClientSideCookieWrite {
+  string cookie;
+
+  DocumentCookieWrite() {
+    exists(DataFlow::PropWrite write | this = write |
+      write = DOM::documentRef().getAPropertyWrite("cookie") and
+      cookie =
+        [
+          any(string s | write.getRhs().mayHaveStringValue(s)),
+          write.getRhs().(StringOps::ConcatenationRoot).getConstantStringParts()
+        ]
+    )
   }
 
-  /**
-   * 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*;?.*$")
+  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() {
+    HeuristicNames::nameIndicatesSensitiveData(getCookieName(cookie), _)
   }
 }

javascript/ql/test/query-tests/Security/CWE-614/ClearTextCookie.expected

@@ -10,3 +10,6 @@
 | tst-cleartextCookie.js:124:9:124:21 | session(sess) | Sensitive cookie sent without enforcing SSL encryption |
 | tst-cleartextCookie.js:148:9:156:2 | session ... Date\\n}) | Sensitive cookie sent without enforcing SSL encryption |
 | tst-cleartextCookie.js:160:33:160:58 | `authKe ... key()}` | Sensitive cookie sent without enforcing SSL encryption |
+| tst-cleartextCookie.js:173:5:173:19 | document.cookie | Sensitive cookie sent without enforcing SSL encryption |
+| tst-cleartextCookie.js:177:5:177:41 | cookies ... hkey()) | Sensitive cookie sent without enforcing SSL encryption |
+| tst-cleartextCookie.js:182:5:182:46 | cookie. ... hkey()) | Sensitive cookie sent without enforcing SSL encryption |

javascript/ql/test/query-tests/Security/CWE-614/tst-cleartextCookie.js

@@ -166,4 +166,19 @@ http.createServer((req, res) => {
     res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}; secure; httpOnly`); // OK
     res.writeHead(200, { 'Content-Type': 'text/html' });
     res.end('<h2>Hello world</h2>');
-});
+});
+
+function clientCookies() {
+    document.cookie = `authKey=${makeAuthkey()}; secure`; // OK
+    document.cookie = `authKey=${makeAuthkey()}`; // NOT OK
+
+    var cookies = require('browser-cookies');
+    
+    cookies.set('authKey', makeAuthkey()); // NOT OK
+    cookies.set('authKey', makeAuthkey(), { secure: true, expires: 7 }); // OK
+
+    const cookie = require('cookie');
+
+    cookie.serialize('authKey', makeAuthkey()); // NOT OK
+    cookie.serialize('authKey', makeAuthkey(), { secure: true, expires: 7 }); // OK
+}