Merge pull request #2955 from erik-krogh/BetterHeader

Approved by asgerf
2020-03-05 08:24:43 +00:00 · 2020-03-05 08:24:43 +00:00 · 85ee5fc988
--- 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
      }

--- 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
--- 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()))
    )
  }

--- 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 |
--- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssContentTypes.js
+++ 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
+});
--- 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 |