Skip to content

Commit 038438e

Browse files
committed
assume that setting the secure/httpOnly flag to some unknown value is good
1 parent 5228196 commit 038438e

File tree

2 files changed

+33
-8
lines changed

2 files changed

+33
-8
lines changed

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

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,9 @@ private module JsCookie {
126126

127127
override predicate isSecure() {
128128
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
129-
this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
129+
exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
130+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
131+
)
130132
}
131133

132134
override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
@@ -162,7 +164,9 @@ private module BrowserCookies {
162164

163165
override predicate isSecure() {
164166
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
165-
this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
167+
exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
168+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
169+
)
166170
or
167171
// or, an explicit default has been set
168172
exists(DataFlow::moduleMember("browser-cookies", "defaults").getAPropertyWrite("secure"))
@@ -201,7 +205,9 @@ private module LibCookie {
201205

202206
override predicate isSecure() {
203207
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
204-
this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
208+
exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
209+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
210+
)
205211
}
206212

207213
override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
@@ -222,15 +228,19 @@ private module ExpressCookies {
222228
override predicate isSecure() {
223229
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
224230
// The default is `false`.
225-
this.getOptionArgument(2, CookieWrites::secure()).mayHaveBooleanValue(true)
231+
exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::secure()) |
232+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
233+
)
226234
}
227235

228236
override predicate isSensitive() { canHaveSensitiveCookie(this.getArgument(0)) }
229237

230238
override predicate isHttpOnly() {
231239
// A cookie is httpOnly if there are cookie options with the `httpOnly` flag set to `true`.
232240
// The default is `false`.
233-
this.getOptionArgument(2, CookieWrites::httpOnly()).mayHaveBooleanValue(true)
241+
exists(DataFlow::Node value | value = this.getOptionArgument(2, CookieWrites::httpOnly()) |
242+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
243+
)
234244
}
235245
}
236246

@@ -272,9 +282,9 @@ private module ExpressCookies {
272282
override predicate isSecure() {
273283
// The flag `secure` is not set by default (https://github.com/expressjs/session#Cookiesecure).
274284
// The default value for cookie options is { path: '/', httpOnly: true, secure: false, maxAge: null }.
275-
// A cookie is secure if there are the cookie options with the `secure` flag set to `true` or to `auto`.
276-
getCookieFlagValue(CookieWrites::secure()).mayHaveBooleanValue(true) or
277-
getCookieFlagValue(CookieWrites::secure()).mayHaveStringValue("auto")
285+
exists(DataFlow::Node value | value = getCookieFlagValue(CookieWrites::secure()) |
286+
not value.mayHaveBooleanValue(false) // anything but `false` is accepted as being maybe true
287+
)
278288
}
279289

280290
override predicate isSensitive() {

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,3 +196,18 @@ http.createServer((req, res) => {
196196
res.writeHead(200, { 'Content-Type': 'text/plain' });
197197
res.end('ok');
198198
});
199+
200+
(function mightBeSecures() {
201+
const express = require('express')
202+
const app = express()
203+
const session = require('express-session')
204+
205+
app.use(session({
206+
secret: config.sessionSecret,
207+
cookie: {
208+
httpOnly: config.sessionCookie.httpOnly,
209+
secure: config.sessionCookie.secure && config.secure.ssl
210+
},
211+
name: config.sessionKey
212+
}));
213+
})();

0 commit comments

Comments
 (0)