Skip to content

Commit 2cb3d2c

Browse files
erik-krogherik-krogh
committed
documentation overhaul on client-exposed-cookie (and restricting it to server-side)
1 parent ab23fff commit 2cb3d2c

File tree

11 files changed

+116
-72
lines changed

11 files changed

+116
-72
lines changed

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

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,25 @@ module CookieWrites {
2424

2525
/**
2626
* Holds if the cookie is likely an authentication cookie or otherwise sensitive.
27-
* Can never hold for client-side cookies. TODO: Or can it...?
27+
* Can never hold for client-side cookies.
2828
*/
2929
abstract predicate isSensitive();
30+
31+
/**
32+
* Holds if the cookie write happens on a server, that is `httpOnly` flag is relevant.
33+
*/
34+
predicate isServerSide() {
35+
any() // holds by default. Client-side cookie writes should extend ClientSideCookieWrite.
36+
}
37+
}
38+
39+
/**
40+
* A client-side write to a cookie.
41+
*/
42+
abstract class ClientSideCookieWrite extends CookieWrite {
43+
final override predicate isHttpOnly() { none() }
44+
45+
final override predicate isServerSide() { none() }
3046
}
3147

3248
/**
@@ -43,6 +59,7 @@ module CookieWrites {
4359
/**
4460
* A model of the `js-cookie` library (https://github.com/js-cookie/js-cookie).
4561
*/
62+
// TODO: Writes to document.cookie.
4663
private module JsCookie {
4764
/**
4865
* Gets a function call that invokes method `name` of the `js-cookie` library.
@@ -61,7 +78,8 @@ private module JsCookie {
6178
}
6279
}
6380

64-
class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode, CookieWrites::CookieWrite {
81+
class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode,
82+
CookieWrites::ClientSideCookieWrite {
6583
WriteAccess() { this = libMemberCall("set") }
6684

6785
string getKey() { getArgument(0).mayHaveStringValue(result) }
@@ -78,8 +96,6 @@ private module JsCookie {
7896
this.getArgument(0).mayHaveStringValue(s)
7997
), _)
8098
}
81-
82-
override predicate isHttpOnly() { none() } // js-cookie is browser side library and doesn't support HttpOnly
8399
}
84100
}
85101

‎javascript/ql/src/Security/CWE-1004/ClientExposedCookie.qhelp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Authentication cookies stored by a server can be accessed by a client if the <code>httpOnly</code> flag is not set.
9+
<p>
10+
<p>
11+
An attacker that manages a cross-site scripting (XSS) attack can read the cookie and hijack the session.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Set the <code>httpOnly</code> flag on all cookies that are not needed by the client.
18+
</p>
19+
</recommendation>
20+
21+
<references>
22+
23+
<example>
24+
<p>
25+
The following example stores an authentication token in a cookie that can
26+
viewed by the client.
27+
</p>
28+
<sample src="examples/ClientExposedCookieGood.js"/>
29+
<p>
30+
To force the cookie to be transmitted using SSL, set the <code>secure</code>
31+
attribute on the cookie.
32+
</p>
33+
<sample src="examples/ClientExposedCookieBad.js"/>
34+
</example>
35+
36+
<references>
37+
<li>ExpressJS: <a href="https://expressjs.com/en/advanced/best-practice-security.html#use-cookies-securely">Use cookies securely</a>.</li>
38+
<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Security_Cheat_Sheet.html#set-cookie-flags-appropriately">Set cookie flags appropriately</a>.</li>
39+
<li><a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a>.</li>
40+
</references>
41+
42+
</qhelp>

‎javascript/ql/src/Security/CWE-1004/ClientExposedCookie.ql

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Sensitive server cookie exposed to the client
3+
* @description Sensitive cookies set by a server can be read by the client if the `httpOnly` flag is not set.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @security-severity 5.0
7+
* @precision high
8+
* @id js/client-exposed-cookie
9+
* @tags security
10+
* external/cwe/cwe-1004
11+
*/
12+
13+
import javascript
14+
15+
from CookieWrites::CookieWrite cookie
16+
where
17+
cookie.isSensitive() and
18+
cookie.isServerSide() and
19+
not cookie.isHttpOnly()
20+
select cookie, "Sensitive server cookie is missing 'httpOnly' flag."

‎javascript/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.qhelp

Lines changed: 0 additions & 25 deletions
This file was deleted.

‎javascript/ql/src/Security/CWE-1004/CookieWithoutHttpOnly.ql

Lines changed: 0 additions & 23 deletions
This file was deleted.

‎javascript/ql/src/Security/CWE-1004/examples/ClientExposedCookieBad.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const http = require('http');
2+
3+
const server = http.createServer((req, res) => {
4+
res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}`);
5+
res.writeHead(200, { 'Content-Type': 'text/html' });
6+
res.end('<h2>Hello world</h2>');
7+
});

‎javascript/ql/src/Security/CWE-1004/examples/ClientExposedCookieGood.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const http = require('http');
2+
3+
const server = http.createServer((req, res) => {
4+
res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}; secure; httpOnly`);
5+
res.writeHead(200, { 'Content-Type': 'text/html' });
6+
res.end('<h2>Hello world</h2>');
7+
});

‎javascript/ql/test/query-tests/Security/CWE-1004/ClientExposedCookie.expected

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
| tst-httpOnly.js:11:9:15:2 | session ... BAD\\n}) | Sensitive server cookie is missing 'httpOnly' flag. |
2+
| tst-httpOnly.js:29:9:29:21 | session(sess) | Sensitive server cookie is missing 'httpOnly' flag. |
3+
| tst-httpOnly.js:38:9:38:22 | session(sess2) | Sensitive server cookie is missing 'httpOnly' flag. |
4+
| tst-httpOnly.js:47:9:47:22 | session(sess3) | Sensitive server cookie is missing 'httpOnly' flag. |
5+
| tst-httpOnly.js:51:9:55:2 | session ... BAD\\n}) | Sensitive server cookie is missing 'httpOnly' flag. |
6+
| tst-httpOnly.js:68:5:73:10 | res.coo ... }) | Sensitive server cookie is missing 'httpOnly' flag. |
7+
| tst-httpOnly.js:78:5:81:10 | res.coo ... }) | Sensitive server cookie is missing 'httpOnly' flag. |
8+
| tst-httpOnly.js:101:5:101:43 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
9+
| tst-httpOnly.js:109:5:109:43 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
10+
| tst-httpOnly.js:118:5:118:43 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
11+
| tst-httpOnly.js:137:5:137:43 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
12+
| tst-httpOnly.js:148:5:148:41 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
13+
| tst-httpOnly.js:159:5:159:43 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
14+
| tst-httpOnly.js:170:5:170:40 | res.coo ... ptions) | Sensitive server cookie is missing 'httpOnly' flag. |
15+
| tst-httpOnly.js:209:37:209:51 | "authKey=ninja" | Sensitive server cookie is missing 'httpOnly' flag. |
16+
| tst-httpOnly.js:229:38:229:52 | "authKey=ninja" | Sensitive server cookie is missing 'httpOnly' flag. |
17+
| tst-httpOnly.js:289:37:289:59 | `authKe ... {attr}` | Sensitive server cookie is missing 'httpOnly' flag. |
18+
| tst-httpOnly.js:303:9:307:2 | session ... BAD\\n}) | Sensitive server cookie is missing 'httpOnly' flag. |
19+
| tst-httpOnly.js:320:9:324:2 | session ... tter\\n}) | Sensitive server cookie is missing 'httpOnly' flag. |

‎javascript/ql/test/query-tests/Security/CWE-1004/ClientExposedCookie.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-1004/ClientExposedCookie.ql

‎javascript/ql/test/query-tests/Security/CWE-1004/CookieWithoutHttpOnly.expected

Lines changed: 0 additions & 19 deletions
This file was deleted.

0 commit comments

Comments
 (0)