From c14a485ca798acbfd9d442ed1844a215e7fb49dc Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 28 Feb 2020 10:42:08 +0100 Subject: [PATCH] recognize more HttpResponseSink by restricting the `hasNonHtmlHeader` check --- .../src/semmle/javascript/frameworks/HTTP.qll | 2 +- .../javascript/frameworks/NodeJSLib.qll | 3 +- .../javascript/security/dataflow/Xss.qll | 56 +++++++++++++-- .../Security/CWE-079/ReflectedXss.expected | 36 ++++++++++ .../CWE-079/ReflectedXssContentTypes.js | 71 +++++++++++++++++++ 5 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js 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 cf2d34844b3..f1c10b1a9df 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..af1206432bf 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -305,18 +305,64 @@ 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) + | + // not the case that the control just exists without potentially going to the worksFor. + not isIrrelevantFor(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 unlikely to affect a resonse send in `sender`. + */ + predicate isIrrelevantFor(HTTP::HeaderDefinition header, HTTP::ResponseSendArgument sender) { + not header.getBasicBlock().getASuccessor*() = sender.getBasicBlock() and + not sender.getBasicBlock().getASuccessor*() = header.getBasicBlock() and + ( + // If there is another header `otherHeader` next to `sender`, then `header` is probably irrelevant. + exists(HTTP::HeaderDefinition otherHeader | not header = otherHeader | + otherHeader.getBasicBlock().getASuccessor*() = sender.getBasicBlock() and + not otherHeader = nonHtmlContentTypeHeader(sender.getRouteHandler()) + ) + or + // Tries to recognize variants of: + // ``` + // response.writeHead(500, {'Content-Type': 'text/plain'}); + // response.end('Some error'); + // return; + // ``` + exists(ReachableBasicBlock headerBlock | headerBlock = header.getBasicBlock() | + headerBlock.getASuccessor() instanceof ControlFlowExitNode and + strictcount(headerBlock.getASuccessor()) = 1 and + not ( + exists(DataFlow::CallNode call | + exists(call.getACallee()) and + call.getBasicBlock() = headerBlock + ) + or + exists(Expr e | e.getBasicBlock() = headerBlock and e instanceof Function) + ) + ) ) } 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..bb930409a61 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js @@ -0,0 +1,71 @@ +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. +}); \ No newline at end of file