Skip to content

Commit 3aec9c1

Browse files
author
edvraa
committed
Cookies without HttpOnly
1 parent 578ce1e commit 3aec9c1

14 files changed

Lines changed: 515 additions & 33 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Cookies without <code>HttpOnly</code> flag are accessible to JavaScript running in the same origin. In case of
7+
Cross-Site Scripting (XSS) vulnerability the cookie can be stolen by malicious script.</p>
8+
</overview>
9+
<recommendation>
10+
11+
<p>Protect sensitive cookies, such as related to authentication, by setting <code>HttpOnly</code> to <code>true</code> to make
12+
them not accessible to JavaScript.</p>
13+
14+
</recommendation>
15+
16+
<references>
17+
18+
<li>Production Best Practices: Security:<a href="https://expressjs.com/en/advanced/best-practice-security.html#use-cookies-securely">Use cookies securely</a>.</li>
19+
<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>
20+
<li>express-session:<a href="https://github.com/expressjs/session#cookiehttponly">cookie.httpOnly</a>.</li>
21+
<li>cookie-session:<a href="https://github.com/expressjs/cookie-session#cookie-options">Cookie Options</a>.</li>
22+
<li><a href="https://expressjs.com/en/api.html#res.cookie">express response.cookie</a>.</li>
23+
<li><a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a>.</li>
24+
</references>
25+
</qhelp>
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name 'HttpOnly' attribute is not set to true
3+
* @description Omitting the 'HttpOnly' attribute for security sensitive data allows
4+
* malicious JavaScript to steal it in case of XSS vulnerability. Always set
5+
* 'HttpOnly' to 'true' to authentication related cookie to make it
6+
* not accessible by JavaScript.
7+
* @kind problem
8+
* @problem.severity warning
9+
* @precision high
10+
* @id js/cookie-httponly-not-set
11+
* @tags security
12+
* external/cwe/cwe-1004
13+
*/
14+
15+
import javascript
16+
import semmle.javascript.security.InsecureCookie::Cookie
17+
18+
from Cookie cookie
19+
where cookie.isAuthNotHttpOnly()
20+
select cookie, "Cookie attribute 'HttpOnly' is not set to true."

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*/
1212

1313
import javascript
14-
import InsecureCookie::Cookie
14+
import semmle.javascript.security.InsecureCookie::Cookie
1515

1616
from Cookie cookie
1717
where not cookie.isSecure()

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,11 @@ class InvokeNode extends DataFlow::SourceNode {
165165
getOptionsArgument(i).hasPropertyWrite(name, result)
166166
}
167167

168+
/**
169+
* Holds if the `i`th argument of this invocation is an object literal set to `result`.
170+
*/
168171
pragma[noinline]
169-
private ObjectLiteralNode getOptionsArgument(int i) { result.flowsTo(getArgument(i)) }
172+
ObjectLiteralNode getOptionsArgument(int i) { result.flowsTo(getArgument(i)) }
170173

171174
/** Gets an abstract value representing possible callees of this call site. */
172175
final AbstractValue getACalleeValue() {

javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.qll renamed to javascript/ql/src/semmle/javascript/security/InsecureCookie.qll

Lines changed: 114 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* Provides classes for reasoning about cookies added to response without the 'secure' flag being set.
33
* A cookie without the 'secure' flag being set can be intercepted and read by a malicious user.
4+
* A cookie without the 'httponly' flag being set can be read by an injected JavaScript
45
*/
56

67
import javascript
@@ -9,7 +10,12 @@ module Cookie {
910
/**
1011
* `secure` property of the cookie options.
1112
*/
12-
string flag() { result = "secure" }
13+
string secureFlag() { result = "secure" }
14+
15+
/**
16+
* `httpOnly` property of the cookie options.
17+
*/
18+
string httpOnlyFlag() { result = "httpOnly" }
1319

1420
/**
1521
* Abstract class to represent different cases of insecure cookie settings.
@@ -29,6 +35,38 @@ module Cookie {
2935
* Holds if this cookie is secure.
3036
*/
3137
abstract predicate isSecure();
38+
39+
/**
40+
* Holds if this cookie is HttpOnly.
41+
*/
42+
abstract predicate isHttpOnly();
43+
44+
/**
45+
* Holds if the cookie is authentication sensitive and lacks HttpOnly.
46+
*/
47+
abstract predicate isAuthNotHttpOnly();
48+
49+
/**
50+
* Holds if the expression is a variable with a sensitive name.
51+
*/
52+
predicate isAuthVariable(DataFlow::Node expr) {
53+
exists(string val |
54+
(
55+
val = expr.getStringValue() or
56+
val = expr.asExpr().(VarAccess).getName()
57+
) and
58+
regexpMatchAuth(val)
59+
)
60+
}
61+
62+
/**
63+
* Holds if the string contains sensitive auth keyword, but not antiforgery token.
64+
*/
65+
bindingset[val]
66+
predicate regexpMatchAuth(string val) {
67+
val.regexpMatch("(?i).*(session|login|token|user|auth|credential).*") and
68+
not val.regexpMatch("(?i).*(xsrf|csrf|forgery).*")
69+
}
3270
}
3371

3472
/**
@@ -37,7 +75,7 @@ module Cookie {
3775
class InsecureCookieSession extends ExpressLibraries::CookieSession::MiddlewareInstance, Cookie {
3876
override string getKind() { result = "cookie-session" }
3977

40-
override DataFlow::SourceNode getCookieOptionsArgument() { result = this.getOption("cookie") }
78+
override DataFlow::SourceNode getCookieOptionsArgument() { result = this.getOptionsArgument(0) }
4179

4280
private DataFlow::Node getCookieFlagValue(string flag) {
4381
result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs()
@@ -46,7 +84,17 @@ module Cookie {
4684
override predicate isSecure() {
4785
// The flag `secure` is set to `false` by default for HTTP, `true` by default for HTTPS (https://github.com/expressjs/cookie-session#cookie-options).
4886
// A cookie is secure if the `secure` flag is not explicitly set to `false`.
49-
not getCookieFlagValue(flag()).mayHaveBooleanValue(false)
87+
not getCookieFlagValue(secureFlag()).mayHaveBooleanValue(false)
88+
}
89+
90+
override predicate isAuthNotHttpOnly() {
91+
not isHttpOnly() // It is a session cookie, likely auth sensitive
92+
}
93+
94+
override predicate isHttpOnly() {
95+
// The flag `httpOnly` is set to `true` by default (https://github.com/expressjs/cookie-session#cookie-options).
96+
// A cookie is httpOnly if the `httpOnly` flag is not explicitly set to `false`.
97+
not getCookieFlagValue(httpOnlyFlag()).mayHaveBooleanValue(false)
5098
}
5199
}
52100

@@ -67,8 +115,19 @@ module Cookie {
67115
// The flag `secure` is not set by default (https://github.com/expressjs/session#Cookieecure).
68116
// The default value for cookie options is { path: '/', httpOnly: true, secure: false, maxAge: null }.
69117
// A cookie is secure if there are the cookie options with the `secure` flag set to `true` or to `auto`.
70-
getCookieFlagValue(flag()).mayHaveBooleanValue(true) or
71-
getCookieFlagValue(flag()).mayHaveStringValue("auto")
118+
getCookieFlagValue(secureFlag()).mayHaveBooleanValue(true) or
119+
getCookieFlagValue(secureFlag()).mayHaveStringValue("auto")
120+
}
121+
122+
override predicate isAuthNotHttpOnly() {
123+
not isHttpOnly() // It is a session cookie, likely auth sensitive
124+
}
125+
126+
override predicate isHttpOnly() {
127+
// The flag `httpOnly` is set by default (https://github.com/expressjs/session#Cookieecure).
128+
// The default value for cookie options is { path: '/', httpOnly: true, secure: false, maxAge: null }.
129+
// A cookie is httpOnly if the `httpOnly` flag is not explicitly set to `false`.
130+
not getCookieFlagValue(httpOnlyFlag()).mayHaveBooleanValue(false)
72131
}
73132
}
74133

@@ -90,7 +149,19 @@ module Cookie {
90149

91150
override predicate isSecure() {
92151
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
93-
getCookieFlagValue(flag()).mayHaveBooleanValue(true)
152+
// The default is `false`.
153+
getCookieFlagValue(secureFlag()).mayHaveBooleanValue(true)
154+
}
155+
156+
override predicate isAuthNotHttpOnly() {
157+
isAuthVariable(this.getArgument(0)) and
158+
not isHttpOnly()
159+
}
160+
161+
override predicate isHttpOnly() {
162+
// A cookie is httpOnly if there are cookie options with the `httpOnly` flag set to `true`.
163+
// The default is `false`.
164+
getCookieFlagValue(httpOnlyFlag()).mayHaveBooleanValue(true)
94165
}
95166
}
96167

@@ -105,14 +176,42 @@ module Cookie {
105176
override string getKind() { result = "set-cookie header" }
106177

107178
override DataFlow::Node getCookieOptionsArgument() {
108-
result.asExpr() = this.asExpr().(ArrayExpr).getAnElement()
179+
if this.asExpr() instanceof ArrayExpr
180+
then result.asExpr() = this.asExpr().(ArrayExpr).getAnElement()
181+
else result.asExpr() = this.asExpr()
109182
}
110183

111184
override predicate isSecure() {
112185
// A cookie is secure if the 'secure' flag is specified in the cookie definition.
113-
exists(string s |
114-
getCookieOptionsArgument().mayHaveStringValue(s) and
115-
s.regexpMatch("(.*;)?\\s*secure.*")
186+
// The default is `false`.
187+
forall(DataFlow::Node n | n = getCookieOptionsArgument() |
188+
exists(string s |
189+
n.mayHaveStringValue(s) and
190+
s.regexpMatch("(?i).*;\\s*secure\\s*;?.*$")
191+
)
192+
)
193+
}
194+
195+
override predicate isAuthNotHttpOnly() {
196+
exists(DataFlow::Node n | n = getCookieOptionsArgument() |
197+
exists(string s |
198+
n.mayHaveStringValue(s) and
199+
(
200+
not s.regexpMatch("(?i).*;\\s*httponly\\s*;?.*$") and
201+
regexpMatchAuth(s.regexpCapture("\\s*([^=\\s]*)\\s*=.*", 1))
202+
)
203+
)
204+
)
205+
}
206+
207+
override predicate isHttpOnly() {
208+
// A cookie is httpOnly if the 'httpOnly' flag is specified in the cookie definition.
209+
// The default is `false`.
210+
forall(DataFlow::Node n | n = getCookieOptionsArgument() |
211+
exists(string s |
212+
n.mayHaveStringValue(s) and
213+
s.regexpMatch("(?i).*;\\s*httponly\\s*;?.*$")
214+
)
116215
)
117216
}
118217
}
@@ -142,7 +241,11 @@ module Cookie {
142241

143242
override predicate isSecure() {
144243
// A cookie is secure if there are cookie options with the `secure` flag set to `true`.
145-
getCookieFlagValue(flag()).mayHaveBooleanValue(true)
244+
getCookieFlagValue(secureFlag()).mayHaveBooleanValue(true)
146245
}
246+
247+
override predicate isAuthNotHttpOnly() { none() }
248+
249+
override predicate isHttpOnly() { none() } // js-cookie is browser side library and doesn't support HttpOnly
147250
}
148251
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
| test_cookie-session.js:12:9:16:2 | session ... BAD\\n}) | Cookie attribute 'HttpOnly' is not set to true. |
2+
| test_cookie-session.js:30:9:30:21 | session(sess) | Cookie attribute 'HttpOnly' is not set to true. |
3+
| test_cookie-session.js:39:9:39:22 | session(sess2) | Cookie attribute 'HttpOnly' is not set to true. |
4+
| test_cookie-session.js:48:9:48:22 | session(sess2) | Cookie attribute 'HttpOnly' is not set to true. |
5+
| test_express-session.js:11:9:15:2 | session ... BAD\\n}) | Cookie attribute 'HttpOnly' is not set to true. |
6+
| test_express-session.js:28:9:32:2 | session ... tter\\n}) | Cookie attribute 'HttpOnly' is not set to true. |
7+
| test_httpserver.js:7:37:7:48 | "auth=ninja" | Cookie attribute 'HttpOnly' is not set to true. |
8+
| test_httpserver.js:27:37:27:70 | ["auth= ... cript"] | Cookie attribute 'HttpOnly' is not set to true. |
9+
| test_httpserver.js:57:37:57:80 | ["auth= ... cript"] | Cookie attribute 'HttpOnly' is not set to true. |
10+
| test_responseCookie.js:15:5:20:10 | res.coo ... }) | Cookie attribute 'HttpOnly' is not set to true. |
11+
| test_responseCookie.js:25:5:28:10 | res.coo ... }) | Cookie attribute 'HttpOnly' is not set to true. |
12+
| test_responseCookie.js:48:5:48:43 | res.coo ... ptions) | Cookie attribute 'HttpOnly' is not set to true. |
13+
| test_responseCookie.js:56:5:56:43 | res.coo ... ptions) | Cookie attribute 'HttpOnly' is not set to true. |
14+
| test_responseCookie.js:65:5:65:43 | res.coo ... ptions) | Cookie attribute 'HttpOnly' is not set to true. |
15+
| test_responseCookie.js:84:5:84:43 | res.coo ... ptions) | Cookie attribute 'HttpOnly' is not set to true. |
16+
| test_responseCookie.js:95:5:95:41 | res.coo ... ptions) | Cookie attribute 'HttpOnly' is not set to true. |
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE-1004/CookieWithoutHttpOnly.ql
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
const express = require('express')
2+
const app = express()
3+
const session = require('cookie-session')
4+
const expiryDate = new Date(Date.now() + 60 * 60 * 1000)
5+
6+
app.use(session({
7+
name: 'session',
8+
keys: ['key1', 'key2'],
9+
httpOnly: true, // GOOD
10+
}))
11+
12+
app.use(session({
13+
name: 'session',
14+
keys: ['key1', 'key2'],
15+
httpOnly: false // BAD
16+
}))
17+
18+
app.use(session({
19+
name: 'session',
20+
keys: ['key1', 'key2'],
21+
secure: true // GOOD, httpOnly is true by default
22+
}))
23+
24+
var sess = {
25+
name: 'session',
26+
keys: ['key1', 'key2'],
27+
}
28+
29+
sess.httpOnly = false;
30+
app.use(session(sess)) // BAD
31+
32+
var sess2 = {
33+
name: 'session',
34+
keys: ['key1', 'key2'],
35+
httpOnly: true,
36+
}
37+
38+
sess2.httpOnly = false;
39+
app.use(session(sess2)) // BAD
40+
41+
var sess2 = {
42+
name: 'mycookie',
43+
keys: ['key1', 'key2'],
44+
httpOnly: true,
45+
}
46+
47+
sess2.httpOnly = false;
48+
app.use(session(sess2)) // BAD, It is a session cookie, name doesn't matter
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
const express = require('express')
2+
const app = express()
3+
const session = require('express-session')
4+
5+
app.use(session({
6+
name: 'session',
7+
keys: ['key1', 'key2'],
8+
cookie: { httpOnly: true }, // GOOD
9+
}))
10+
11+
app.use(session({
12+
name: 'session',
13+
keys: ['key1', 'key2'],
14+
cookie: { httpOnly: false } // BAD
15+
}))
16+
17+
app.use(session({
18+
name: 'session',
19+
keys: ['key1', 'key2'],
20+
cookie: { secure: true } // GOOD, httpOnly is true by default
21+
}))
22+
23+
app.use(session({ // GOOD, httpOnly is true by default
24+
name: 'session',
25+
keys: ['key1', 'key2']
26+
}))
27+
28+
app.use(session({
29+
name: 'mycookie',
30+
keys: ['key1', 'key2'],
31+
cookie: { httpOnly: false } // BAD, It is a session cookie, name doesn't matter
32+
}))

0 commit comments

Comments
 (0)