Skip to content

Commit 85ee5fc

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#2955 from erik-krogh/BetterHeader
Approved by asgerf
2 parents 98034aa + bc13204 commit 85ee5fc

File tree

6 files changed

+181
-7
lines changed

6 files changed

+181
-7
lines changed

‎javascript/ql/src/semmle/javascript/frameworks/HTTP.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ module HTTP {
271271
*/
272272
abstract class StandardRouteHandler extends RouteHandler {
273273
override HeaderDefinition getAResponseHeader(string name) {
274-
result.(StandardHeaderDefinition).getRouteHandler() = this and
274+
result.getRouteHandler() = this and
275275
result.getAHeaderName() = name
276276
}
277277

‎javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,11 @@ module NodeJSLib {
252252
private class WriteHead extends HeaderDefinition {
253253
WriteHead() {
254254
astNode.getMethodName() = "writeHead" and
255-
astNode.getNumArgument() > 1
255+
astNode.getNumArgument() >= 1
256256
}
257257

258258
override predicate definesExplicitly(string headerName, Expr headerValue) {
259+
astNode.getNumArgument() > 1 and
259260
exists(DataFlow::SourceNode headers, string header |
260261
headers.flowsToExpr(astNode.getLastArgument()) and
261262
headers.hasPropertyWrite(header, DataFlow::valueNode(headerValue)) and

‎javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,18 +305,72 @@ module ReflectedXss {
305305
* a content type that does not (case-insensitively) contain the string "html". This
306306
* is to prevent us from flagging plain-text or JSON responses as vulnerable.
307307
*/
308-
private class HttpResponseSink extends Sink, DataFlow::ValueNode {
308+
class HttpResponseSink extends Sink, DataFlow::ValueNode {
309309
override HTTP::ResponseSendArgument astNode;
310310

311-
HttpResponseSink() { not nonHtmlContentType(astNode.getRouteHandler()) }
311+
HttpResponseSink() { not exists(getANonHtmlHeaderDefinition(astNode)) }
312+
}
313+
314+
/**
315+
* Gets a HeaderDefinition that defines a non-html content-type for `send`.
316+
*/
317+
HTTP::HeaderDefinition getANonHtmlHeaderDefinition(HTTP::ResponseSendArgument send) {
318+
exists(HTTP::RouteHandler h |
319+
send.getRouteHandler() = h and
320+
result = nonHtmlContentTypeHeader(h)
321+
|
322+
// The HeaderDefinition affects a response sent at `send`.
323+
headerAffects(result, send)
324+
)
312325
}
313326

314327
/**
315328
* Holds if `h` may send a response with a content type other than HTML.
316329
*/
317-
private predicate nonHtmlContentType(HTTP::RouteHandler h) {
318-
exists(HTTP::HeaderDefinition hd | hd = h.getAResponseHeader("content-type") |
319-
not exists(string tp | hd.defines("content-type", tp) | tp.regexpMatch("(?i).*html.*"))
330+
HTTP::HeaderDefinition nonHtmlContentTypeHeader(HTTP::RouteHandler h) {
331+
result = h.getAResponseHeader("content-type") and
332+
not exists(string tp | result.defines("content-type", tp) | tp.regexpMatch("(?i).*html.*"))
333+
}
334+
335+
/**
336+
* Holds if a header set in `header` is likely to affect a response sent at `sender`.
337+
*/
338+
predicate headerAffects(HTTP::HeaderDefinition header, HTTP::ResponseSendArgument sender) {
339+
sender.getRouteHandler() = header.getRouteHandler() and
340+
(
341+
// `sender` is affected by a dominating `header`.
342+
header.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
343+
or
344+
// There is no dominating header, and `header` is non-local.
345+
not isLocalHeaderDefinition(header) and
346+
not exists(HTTP::HeaderDefinition dominatingHeader |
347+
dominatingHeader.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
348+
)
349+
)
350+
}
351+
352+
/**
353+
* Holds if the HeaderDefinition `header` seems to be local.
354+
* A HeaderDefinition is local if it dominates exactly one `ResponseSendArgument`.
355+
*
356+
* Recognizes variants of:
357+
* ```
358+
* response.writeHead(500, ...);
359+
* response.end('Some error');
360+
* return;
361+
* ```
362+
*/
363+
predicate isLocalHeaderDefinition(HTTP::HeaderDefinition header) {
364+
exists(ReachableBasicBlock headerBlock |
365+
headerBlock = header.getBasicBlock().(ReachableBasicBlock)
366+
|
367+
1 =
368+
strictcount(HTTP::ResponseSendArgument sender |
369+
sender.getRouteHandler() = header.getRouteHandler() and
370+
header.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
371+
) and
372+
// doesn't dominate something that looks like a callback.
373+
not exists(Expr e | e instanceof Function | headerBlock.dominates(e.getBasicBlock()))
320374
)
321375
}
322376

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,22 @@ nodes
33
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
44
| ReflectedXss.js:8:33:8:45 | req.params.id |
55
| ReflectedXss.js:8:33:8:45 | req.params.id |
6+
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
7+
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
8+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
9+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id |
10+
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
11+
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
12+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id |
13+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id |
14+
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
15+
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
16+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id |
17+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id |
18+
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
19+
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
20+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
21+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id |
622
| etherpad.js:9:5:9:53 | response |
723
| etherpad.js:9:16:9:30 | req.query.jsonp |
824
| etherpad.js:9:16:9:30 | req.query.jsonp |
@@ -75,6 +91,22 @@ edges
7591
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
7692
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
7793
| ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id |
94+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
95+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
96+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
97+
| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id |
98+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
99+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
100+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
101+
| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id |
102+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
103+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
104+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
105+
| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id |
106+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
107+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
108+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
109+
| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id |
78110
| etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
79111
| etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response |
80112
| etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" |
@@ -134,6 +166,10 @@ edges
134166
| tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p |
135167
#select
136168
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
169+
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
170+
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
171+
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
172+
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
137173
| etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
138174
| exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
139175
| formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
app.get('/user/:id', function (req, res) {
5+
if (whatever) {
6+
res.set('Content-Type', 'text/plain');
7+
res.send("FOO: " + req.params.id); // OK - content type is plain text
8+
} else {
9+
res.set('Content-Type', 'text/html');
10+
res.send("FOO: " + req.params.id); // NOT OK - content type is HTML.
11+
}
12+
});
13+
14+
app.get('/user/:id', function (req, res) {
15+
if (whatever) {
16+
res.writeHead(200, {'Content-Type': 'application/json'});
17+
res.send("FOO: " + req.params.id); // OK - content type is JSON
18+
} else {
19+
res.writeHead(404);
20+
res.send("FOO: " + req.params.id); // NOT OK - content type is not set.
21+
}
22+
});
23+
24+
25+
app.get('/user/:id', function (req, res) {
26+
res.writeHead(200, {'Content-Type': 'application/json'});
27+
if (whatever) {
28+
res.send("FOO: " + req.params.id); // OK - content type is JSON
29+
} else {
30+
res.send("FOO: " + req.params.id); // OK - content type is still JSON
31+
}
32+
res.send("FOO: " + req.params.id); // OK - content type is still JSON
33+
});
34+
35+
36+
app.get('/user/:id', function (req, res) {
37+
if (err) {
38+
res.statusCode = 404;
39+
res.end("FOO: " + req.params.id); // NOT OK
40+
} else {
41+
res.setHeader('Content-Type', 'text/plain;charset=utf8');
42+
res.end("FOO: " + req.params.id); // OK
43+
}
44+
});
45+
46+
function textContentType() {
47+
result = "text/plain";
48+
}
49+
50+
app.get('/user/:id', function (req, res) {
51+
if (err) {
52+
res.header({'Content-Type': textContentType()});
53+
res.end("FOO: " + req.params.id); // OK
54+
} else {
55+
res.setHeader('Content-Type', 'text/plain;charset=utf8');
56+
res.end("FOO: " + req.params.id); // OK
57+
}
58+
});
59+
60+
app.get('/user/:id', function (req, res) {
61+
if (err) {
62+
res.writeHead(200, {'Content-Type': 'application/json'});
63+
res.send("FOO: " + req.params.id); // OK - content type is JSON
64+
return;
65+
}
66+
doSomething();
67+
somethingMore();
68+
while(Math.random()) {};
69+
res.writeHead(404);
70+
res.send("FOO: " + req.params.id); // NOT OK - content type is not set.
71+
});
72+
73+
app.get('/user/:id', function (req, res) {
74+
res.header({'Content-Type': textContentType()});
75+
myFancyFunction(() => {
76+
res.send("FOO: " + req.params.id); // OK
77+
});
78+
res.end("FOO: " + req.params.id); // OK
79+
});

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
| ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
2+
| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | user-provided value |
3+
| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | user-provided value |
4+
| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | user-provided value |
5+
| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | user-provided value |
26
| exception-xss.js:190:12:190:24 | req.params.id | Cross-site scripting vulnerability due to $@. | exception-xss.js:190:12:190:24 | req.params.id | user-provided value |
37
| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
48
| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |

0 commit comments

Comments
 (0)