Merge pull request #3978 from dellalibera/js/insecure-cookies

Approved by esbena

javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Failing to set the 'secure' flag on a cookie can cause it to be sent in cleartext.
+This makes it easier for an attacker to intercept.</p>
+</overview>
+<recommendation>
+
+<p>Always set the <code>secure</code> flag to `true` on a cookie before adding it
+to an HTTP response (if the default value is `false`).</p>
+
+</recommendation>
+
+<references>
+
+<li>Production Best Practices: Security:<a href="https://expressjs.com/en/advanced/best-practice-security.html#use-cookies-securely">Use cookies securely</a>.</li>
+<li>NodeJS security cheat sheet:<a href="https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Security_Cheat_Sheet.html#set-cookie-flags-appropriately">Set cookie flags appropriately</a>.</li>
+<li>express-session:<a href="https://github.com/expressjs/session#cookiesecure">cookie.secure</a>.</li>
+<li>cookie-session:<a href="https://github.com/expressjs/cookie-session#cookie-options">Cookie Options</a>.</li>
+<li><a href="https://expressjs.com/en/api.html#res.cookie">express response.cookie</a>.</li>
+<li><a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a>.</li>
+<li><a href="https://github.com/js-cookie/js-cookie">js-cookie</a>.</li>
+</references>
+</qhelp>

javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Failure to set secure cookies
+ * @description Insecure cookies may be sent in cleartext, which makes them vulnerable to
+ *              interception.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/insecure-cookie
+ * @tags security
+ *       external/cwe/cwe-614
+ */
+
+import javascript
+import InsecureCookie::Cookie
+
+from Cookie cookie
+where not cookie.isSecure()
+select cookie, "Cookie is added to response without the 'secure' flag being set to true"

javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qll

@@ -0,0 +1,146 @@
+/**
+ * Provides classes for reasoning about cookies added to response without the 'secure' flag being set.
+ * A cookie without the 'secure' flag being set can be intercepted and read by a malicious user.
+ */
+
+import javascript
+
+module Cookie {
+  /**
+   * `secure` property of the cookie options.
+   */
+  string flag() { result = "secure" }
+
+  /**
+   * Abstract class to represent different cases of insecure cookie settings.
+   */
+  abstract class Cookie extends DataFlow::Node {
+    /**
+     * Gets the name of the middleware/library used to set the cookie.
+     */
+    abstract string getKind();
+
+    /**
+     * Gets the options used to set this cookie, if any.
+     */
+    abstract DataFlow::Node getCookieOptionsArgument();
+
+    /**
+     * Holds if this cookie is secure.
+     */
+    abstract predicate isSecure();
+  }
+
+  /**
+   * A cookie set using the `express` module `cookie-session` (https://github.com/expressjs/cookie-session).
+   */
+  class InsecureCookieSession extends ExpressLibraries::CookieSession::MiddlewareInstance, Cookie {
+    override string getKind() { result = "cookie-session" }
+
+    override DataFlow::SourceNode getCookieOptionsArgument() { result = this.getOption("cookie") }
+
+    private DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs()
+    }
+
+    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(flag()).mayHaveBooleanValue(false)
+    }
+  }
+
+  /**
+   * A cookie set using the `express` module `express-session` (https://github.com/expressjs/session).
+   */
+  class InsecureExpressSessionCookie extends ExpressLibraries::ExpressSession::MiddlewareInstance,
+    Cookie {
+    override string getKind() { result = "express-session" }
+
+    override DataFlow::SourceNode getCookieOptionsArgument() { result = this.getOption("cookie") }
+
+    private DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs()
+    }
+
+    override predicate isSecure() {
+      // The flag `secure` is not set by default (https://github.com/expressjs/session#Cookieecure).
+      // 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(flag()).mayHaveBooleanValue(true) or
+      getCookieFlagValue(flag()).mayHaveStringValue("auto")
+    }
+  }
+
+  /**
+   * A cookie set using `response.cookie` from `express` module (https://expressjs.com/en/api.html#res.cookie).
+   */
+  class InsecureExpressCookieResponse extends Cookie, DataFlow::MethodCallNode {
+    InsecureExpressCookieResponse() { this.calls(any(Express::ResponseExpr r).flow(), "cookie") }
+
+    override string getKind() { result = "response.cookie" }
+
+    override DataFlow::SourceNode getCookieOptionsArgument() {
+      result = this.getLastArgument().getALocalSource()
+    }
+
+    private DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs()
+    }
+
+    override predicate isSecure() {
+      // A cookie is secure if there are cookie options with the `secure` flag set to `true`.
+      getCookieFlagValue(flag()).mayHaveBooleanValue(true)
+    }
+  }
+
+  /**
+   * A cookie set using `Set-Cookie` header of an `HTTP` response (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie).
+   */
+  class InsecureSetCookieHeader extends Cookie {
+    InsecureSetCookieHeader() {
+      this.asExpr() = any(HTTP::SetCookieHeader setCookie).getHeaderArgument()
+    }
+
+    override string getKind() { result = "set-cookie header" }
+
+    override DataFlow::Node getCookieOptionsArgument() {
+      result.asExpr() = this.asExpr().(ArrayExpr).getAnElement()
+    }
+
+    override predicate isSecure() {
+      // A cookie is secure if the 'secure' flag is specified in the cookie definition.
+      exists(string s |
+        getCookieOptionsArgument().mayHaveStringValue(s) and
+        s.regexpMatch("(.*;)?\\s*secure.*")
+      )
+    }
+  }
+
+  /**
+   * A cookie set using `js-cookie` library (https://github.com/js-cookie/js-cookie).
+   */
+  class InsecureJsCookie extends Cookie {
+    InsecureJsCookie() {
+      this =
+        [DataFlow::globalVarRef("Cookie"),
+            DataFlow::globalVarRef("Cookie").getAMemberCall("noConflict"),
+            DataFlow::moduleImport("js-cookie")].getAMemberCall("set")
+    }
+
+    override string getKind() { result = "js-cookie" }
+
+    override DataFlow::SourceNode getCookieOptionsArgument() {
+      result = this.(DataFlow::CallNode).getAnArgument().getALocalSource()
+    }
+
+    DataFlow::Node getCookieFlagValue(string flag) {
+      result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs()
+    }
+
+    override predicate isSecure() {
+      // A cookie is secure if there are cookie options with the `secure` flag set to `true`.
+      getCookieFlagValue(flag()).mayHaveBooleanValue(true)
+    }
+  }
+}

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

@@ -0,0 +1,9 @@
+| test_cookie-session.js:18:9:28:2 | session ...    }\\n}) | Cookie is added to response without the 'secure' flag being set to true |
+| test_express-session.js:5:9:8:2 | session ... T OK\\n}) | Cookie is added to response without the 'secure' flag being set to true |
+| test_express-session.js:10:9:13:2 | session ... T OK\\n}) | Cookie is added to response without the 'secure' flag being set to true |
+| test_express-session.js:15:9:18:2 | session ... T OK\\n}) | Cookie is added to response without the 'secure' flag being set to true |
+| test_express-session.js:25:9:25:21 | session(sess) | Cookie is added to response without the 'secure' flag being set to true |
+| test_httpserver.js:7:37:7:73 | ["type= ... cript"] | Cookie is added to response without the 'secure' flag being set to true |
+| test_jscookie.js:2:1:2:48 | js_cook ... alse }) | Cookie is added to response without the 'secure' flag being set to true |
+| test_responseCookie.js:5:5:10:10 | res.coo ...      }) | Cookie is added to response without the 'secure' flag being set to true |
+| test_responseCookie.js:20:5:20:40 | res.coo ... ptions) | Cookie is added to response without the 'secure' flag being set to true |

javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE-614/InsecureCookie.ql

javascript/ql/test/query-tests/Security/CWE-614/test_cookie-session.js

@@ -0,0 +1,28 @@
+const express = require('express')
+const app = express()
+const session = require('cookie-session')
+const expiryDate = new Date(Date.now() + 60 * 60 * 1000)
+
+app.use(session({
+    name: 'session',
+    keys: ['key1', 'key2'],
+    cookie: {
+        secure: true, // OK
+        httpOnly: true,
+        domain: 'example.com',
+        path: 'foo/bar',
+        expires: expiryDate
+    }
+}))
+
+app.use(session({
+    name: 'session',
+    keys: ['key1', 'key2'],
+    cookie: {
+        secure: false, // NOT OK
+        httpOnly: true,
+        domain: 'example.com',
+        path: 'foo/bar',
+        expires: expiryDate
+    }
+}))

javascript/ql/test/query-tests/Security/CWE-614/test_express-session.js

@@ -0,0 +1,33 @@
+const express = require('express')
+const app = express()
+const session = require('express-session')
+
+app.use(session({
+    secret: 'secret',
+    cookie: { secure: false } // NOT OK
+}))
+
+app.use(session({
+    secret: 'secret'
+    // NOT OK
+}))
+
+app.use(session({
+    secret: 'secret',
+    cookie: {} // NOT OK
+}))
+
+const sess = {
+    secret: 'secret',
+    cookie: { secure: false } // NOT OK
+}
+
+app.use(session(sess))
+
+
+app.set('trust proxy', 1)
+app.use(session({
+    secret: 'secret',
+    cookie: { secure: true } // OK
+}))
+

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

@@ -0,0 +1,22 @@
+const http = require('http');
+
+function test1() {
+    const server = http.createServer((req, res) => {
+        res.setHeader('Content-Type', 'text/html');
+        // NOT OK
+        res.setHeader("Set-Cookie", ["type=ninja", "language=javascript"]);
+        res.writeHead(200, { 'Content-Type': 'text/plain' });
+        res.end('ok');
+    });
+}
+
+
+function test2() {
+    const server = http.createServer((req, res) => {
+        res.setHeader('Content-Type', 'text/html');
+        // OK
+        res.setHeader("Set-Cookie", ["type=ninja; Secure", "language=javascript; secure"]);
+        res.writeHead(200, { 'Content-Type': 'text/plain' });
+        res.end('ok');
+    });
+}

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

@@ -0,0 +1,3 @@
+const js_cookie = require('js-cookie')
+js_cookie.set('key', 'value', { secure: false }); // NOT OK
+js_cookie.set('key', 'value', { secure: true }); // OK

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

@@ -0,0 +1,33 @@
+const express = require('express')
+const app = express()
+
+app.get('/a', function (req, res, next) {
+    res.cookie('name', 'value',
+        {
+            maxAge: 9000000000,
+            httpOnly: true,
+            secure: false // NOT OK
+        });
+    res.end('ok')
+})
+
+app.get('/b', function (req, res, next) {
+    let options = {
+        maxAge: 9000000000,
+        httpOnly: true,
+        secure: false // NOT OK
+    }
+    res.cookie('name', 'value', options);
+    res.end('ok')
+})
+
+app.get('/c', function (req, res, next) {
+    res.cookie('name', 'value',
+        {
+            maxAge: 9000000000,
+            httpOnly: true,
+            secure: true // OK
+        });
+    res.end('ok')
+})
+