diff --git a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll index 5eeda3df43b..fd28cd43666 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/HTTP.qll @@ -271,7 +271,7 @@ module HTTP { */ abstract class StandardRouteHandler extends RouteHandler { override HeaderDefinition getAResponseHeader(string name) { - result.(StandardHeaderDefinition).getRouteHandler() = this and + result.getRouteHandler() = this and result.getAHeaderName() = name } diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 1fe604630e7..d7bb078b4cf 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -252,10 +252,11 @@ module NodeJSLib { private class WriteHead extends HeaderDefinition { WriteHead() { astNode.getMethodName() = "writeHead" and - astNode.getNumArgument() > 1 + astNode.getNumArgument() >= 1 } override predicate definesExplicitly(string headerName, Expr headerValue) { + astNode.getNumArgument() > 1 and exists(DataFlow::SourceNode headers, string header | headers.flowsToExpr(astNode.getLastArgument()) and headers.hasPropertyWrite(header, DataFlow::valueNode(headerValue)) and diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll index fb0f94e84da..47aff96c9c3 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -305,18 +305,72 @@ module ReflectedXss { * a content type that does not (case-insensitively) contain the string "html". This * is to prevent us from flagging plain-text or JSON responses as vulnerable. */ - private class HttpResponseSink extends Sink, DataFlow::ValueNode { + class HttpResponseSink extends Sink, DataFlow::ValueNode { override HTTP::ResponseSendArgument astNode; - HttpResponseSink() { not nonHtmlContentType(astNode.getRouteHandler()) } + HttpResponseSink() { not exists(getANonHtmlHeaderDefinition(astNode)) } + } + + /** + * Gets a HeaderDefinition that defines a non-html content-type for `send`. + */ + HTTP::HeaderDefinition getANonHtmlHeaderDefinition(HTTP::ResponseSendArgument send) { + exists(HTTP::RouteHandler h | + send.getRouteHandler() = h and + result = nonHtmlContentTypeHeader(h) + | + // The HeaderDefinition affects a response sent at `send`. + headerAffects(result, send) + ) } /** * Holds if `h` may send a response with a content type other than HTML. */ - private predicate nonHtmlContentType(HTTP::RouteHandler h) { - exists(HTTP::HeaderDefinition hd | hd = h.getAResponseHeader("content-type") | - not exists(string tp | hd.defines("content-type", tp) | tp.regexpMatch("(?i).*html.*")) + HTTP::HeaderDefinition nonHtmlContentTypeHeader(HTTP::RouteHandler h) { + result = h.getAResponseHeader("content-type") and + not exists(string tp | result.defines("content-type", tp) | tp.regexpMatch("(?i).*html.*")) + } + + /** + * Holds if a header set in `header` is likely to affect a response sent at `sender`. + */ + predicate headerAffects(HTTP::HeaderDefinition header, HTTP::ResponseSendArgument sender) { + sender.getRouteHandler() = header.getRouteHandler() and + ( + // `sender` is affected by a dominating `header`. + header.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock()) + or + // There is no dominating header, and `header` is non-local. + not isLocalHeaderDefinition(header) and + not exists(HTTP::HeaderDefinition dominatingHeader | + dominatingHeader.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock()) + ) + ) + } + + /** + * Holds if the HeaderDefinition `header` seems to be local. + * A HeaderDefinition is local if it dominates exactly one `ResponseSendArgument`. + * + * Recognizes variants of: + * ``` + * response.writeHead(500, ...); + * response.end('Some error'); + * return; + * ``` + */ + predicate isLocalHeaderDefinition(HTTP::HeaderDefinition header) { + exists(ReachableBasicBlock headerBlock | + headerBlock = header.getBasicBlock().(ReachableBasicBlock) + | + 1 = + strictcount(HTTP::ResponseSendArgument sender | + sender.getRouteHandler() = header.getRouteHandler() and + header.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock()) + ) and + // doesn't dominate something that looks like a callback. + not exists(Expr e | e instanceof Function | headerBlock.dominates(e.getBasicBlock())) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index e283a5676fa..56a3c4dc6ca 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -3,6 +3,22 @@ nodes | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | | ReflectedXss.js:8:33:8:45 | req.params.id | | ReflectedXss.js:8:33:8:45 | req.params.id | +| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:10:14:10:36 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | +| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | +| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:20:14:20:36 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | +| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | +| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:39:13:39:35 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | +| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | +| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:70:12:70:34 | "FOO: " ... rams.id | +| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | +| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | | etherpad.js:9:5:9:53 | response | | etherpad.js:9:16:9:30 | req.query.jsonp | | etherpad.js:9:16:9:30 | req.query.jsonp | @@ -75,6 +91,22 @@ edges | ReflectedXss.js:8:33:8:45 | req.params.id | 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 | | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | +| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | 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 | +| ReflectedXssContentTypes.js:10:24:10:36 | req.params.id | 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 | +| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | 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 | +| ReflectedXssContentTypes.js:20:24:20:36 | req.params.id | 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 | +| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | 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 | +| ReflectedXssContentTypes.js:39:23:39:35 | req.params.id | 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 | +| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | 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 | +| ReflectedXssContentTypes.js:70:22:70:34 | req.params.id | 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 | | etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response | | etherpad.js:9:5:9:53 | response | etherpad.js:11:12:11:19 | response | | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:9:16:9:53 | req.que ... e + ")" | @@ -134,6 +166,10 @@ edges | tst2.js:14:9:14:9 | p | tst2.js:14:7:14:24 | p | #select | 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 | +| 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 | +| 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 | +| 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 | +| 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 | | 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 | | 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 | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js new file mode 100644 index 00000000000..64acfded0a6 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js @@ -0,0 +1,79 @@ +var express = require('express'); +var app = express(); + +app.get('/user/:id', function (req, res) { + if (whatever) { + res.set('Content-Type', 'text/plain'); + res.send("FOO: " + req.params.id); // OK - content type is plain text + } else { + res.set('Content-Type', 'text/html'); + res.send("FOO: " + req.params.id); // NOT OK - content type is HTML. + } +}); + +app.get('/user/:id', function (req, res) { + if (whatever) { + res.writeHead(200, {'Content-Type': 'application/json'}); + res.send("FOO: " + req.params.id); // OK - content type is JSON + } else { + res.writeHead(404); + res.send("FOO: " + req.params.id); // NOT OK - content type is not set. + } +}); + + +app.get('/user/:id', function (req, res) { + res.writeHead(200, {'Content-Type': 'application/json'}); + if (whatever) { + res.send("FOO: " + req.params.id); // OK - content type is JSON + } else { + res.send("FOO: " + req.params.id); // OK - content type is still JSON + } + res.send("FOO: " + req.params.id); // OK - content type is still JSON +}); + + +app.get('/user/:id', function (req, res) { + if (err) { + res.statusCode = 404; + res.end("FOO: " + req.params.id); // NOT OK + } else { + res.setHeader('Content-Type', 'text/plain;charset=utf8'); + res.end("FOO: " + req.params.id); // OK + } +}); + +function textContentType() { + result = "text/plain"; +} + +app.get('/user/:id', function (req, res) { + if (err) { + res.header({'Content-Type': textContentType()}); + res.end("FOO: " + req.params.id); // OK + } else { + res.setHeader('Content-Type', 'text/plain;charset=utf8'); + res.end("FOO: " + req.params.id); // OK + } +}); + +app.get('/user/:id', function (req, res) { + if (err) { + res.writeHead(200, {'Content-Type': 'application/json'}); + res.send("FOO: " + req.params.id); // OK - content type is JSON + return; + } + doSomething(); + somethingMore(); + while(Math.random()) {}; + res.writeHead(404); + res.send("FOO: " + req.params.id); // NOT OK - content type is not set. +}); + +app.get('/user/:id', function (req, res) { + res.header({'Content-Type': textContentType()}); + myFancyFunction(() => { + res.send("FOO: " + req.params.id); // OK + }); + res.end("FOO: " + req.params.id); // OK +}); \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected index 560dca58553..e8d727f51f8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected @@ -1,4 +1,8 @@ | 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 | +| 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 | +| 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 | +| 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 | +| 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 | | 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 | | 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 | | 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 |