Skip to content

Commit c14a485

Browse files
erik-krogherik-krogh
committed
recognize more HttpResponseSink by restricting the hasNonHtmlHeader check
1 parent b210009 commit c14a485

File tree

5 files changed

+161
-7
lines changed

5 files changed

+161
-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: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,18 +305,64 @@ 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+
// not the case that the control just exists without potentially going to the worksFor.
323+
not isIrrelevantFor(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 unlikely to affect a resonse send in `sender`.
337+
*/
338+
predicate isIrrelevantFor(HTTP::HeaderDefinition header, HTTP::ResponseSendArgument sender) {
339+
not header.getBasicBlock().getASuccessor*() = sender.getBasicBlock() and
340+
not sender.getBasicBlock().getASuccessor*() = header.getBasicBlock() and
341+
(
342+
// If there is another header `otherHeader` next to `sender`, then `header` is probably irrelevant.
343+
exists(HTTP::HeaderDefinition otherHeader | not header = otherHeader |
344+
otherHeader.getBasicBlock().getASuccessor*() = sender.getBasicBlock() and
345+
not otherHeader = nonHtmlContentTypeHeader(sender.getRouteHandler())
346+
)
347+
or
348+
// Tries to recognize variants of:
349+
// ```
350+
// response.writeHead(500, {'Content-Type': 'text/plain'});
351+
// response.end('Some error');
352+
// return;
353+
// ```
354+
exists(ReachableBasicBlock headerBlock | headerBlock = header.getBasicBlock() |
355+
headerBlock.getASuccessor() instanceof ControlFlowExitNode and
356+
strictcount(headerBlock.getASuccessor()) = 1 and
357+
not (
358+
exists(DataFlow::CallNode call |
359+
exists(call.getACallee()) and
360+
call.getBasicBlock() = headerBlock
361+
)
362+
or
363+
exists(Expr e | e.getBasicBlock() = headerBlock and e instanceof Function)
364+
)
365+
)
320366
)
321367
}
322368

‎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: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
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+
});

0 commit comments

Comments
 (0)