diff --git a/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qhelp b/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qhelp new file mode 100644 index 000000000000..06e920b44bf9 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qhelp @@ -0,0 +1,26 @@ + + + +

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.

+ + + +

Always set the secure flag to `true` on a cookie before adding it +to an HTTP response (if the default value is `false`).

+ + + + + +
  • Production Best Practices: Security:Use cookies securely.
    • +
  • NodeJS security cheat sheet:Set cookie flags appropriately.
    • +
  • express-session:cookie.secure.
    • +
  • cookie-session:Cookie Options.
    • +
  • express response.cookie.
    • +
  • Set-Cookie.
    • +
  • js-cookie.
    • +     +     diff --git a/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.ql b/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.ql new file mode 100644 index 000000000000..69b3afb1c7f7 --- /dev/null +++ b/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" diff --git a/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qll b/javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qll new file mode 100644 index 000000000000..7b32c1fce55b --- /dev/null +++ b/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) + } + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.expected b/javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.expected new file mode 100644 index 000000000000..fc98205a151d --- /dev/null +++ b/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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.qlref b/javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.qlref new file mode 100644 index 000000000000..378d5dcae1a0 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-614/InsecureCookies.qlref @@ -0,0 +1 @@ +experimental/Security/CWE-614/InsecureCookie.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-614/test_cookie-session.js b/javascript/ql/test/query-tests/Security/CWE-614/test_cookie-session.js new file mode 100644 index 000000000000..d8d80b0059ef --- /dev/null +++ b/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 + } +})) diff --git a/javascript/ql/test/query-tests/Security/CWE-614/test_express-session.js b/javascript/ql/test/query-tests/Security/CWE-614/test_express-session.js new file mode 100644 index 000000000000..873fe2162cac --- /dev/null +++ b/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 +})) + diff --git a/javascript/ql/test/query-tests/Security/CWE-614/test_httpserver.js b/javascript/ql/test/query-tests/Security/CWE-614/test_httpserver.js new file mode 100644 index 000000000000..fb9fd386850a --- /dev/null +++ b/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'); + }); +} diff --git a/javascript/ql/test/query-tests/Security/CWE-614/test_jscookie.js b/javascript/ql/test/query-tests/Security/CWE-614/test_jscookie.js new file mode 100644 index 000000000000..d53fcecf84b7 --- /dev/null +++ b/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 diff --git a/javascript/ql/test/query-tests/Security/CWE-614/test_responseCookie.js b/javascript/ql/test/query-tests/Security/CWE-614/test_responseCookie.js new file mode 100644 index 000000000000..8d002d594f39 --- /dev/null +++ b/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') +}) +