Merge pull request github#10764 from pwntester/javascript_xss_improvements

JS: Consider other XSS unsafe content-types when reasoning about XSS vulnerabilities

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/dataflow/ReflectedXssCustomizations.qll

@@ -25,13 +25,13 @@ module ReflectedXss {
    * is to prevent us from flagging plain-text or JSON responses as vulnerable.
    */
   class HttpResponseSink extends Sink instanceof Http::ResponseSendArgument {
-    HttpResponseSink() { not exists(getANonHtmlHeaderDefinition(this)) }
+    HttpResponseSink() { not exists(getAXssSafeHeaderDefinition(this)) }
   }
 
   /**
-   * Gets a HeaderDefinition that defines a non-html content-type for `send`.
+   * DEPRECATED: Gets a HeaderDefinition that defines a non-html content-type for `send`.
    */
-  Http::HeaderDefinition getANonHtmlHeaderDefinition(Http::ResponseSendArgument send) {
+  deprecated Http::HeaderDefinition getANonHtmlHeaderDefinition(Http::ResponseSendArgument send) {
     exists(Http::RouteHandler h |
       send.getRouteHandler() = h and
       result = nonHtmlContentTypeHeader(h)
@@ -42,13 +42,49 @@ module ReflectedXss {
   }
 
   /**
-   * Holds if `h` may send a response with a content type other than HTML.
+   * DEPRECATED: Holds if `h` may send a response with a content type other than HTML.
    */
-  Http::HeaderDefinition nonHtmlContentTypeHeader(Http::RouteHandler h) {
+  deprecated 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.*"))
   }
 
+  /**
+   * Gets a HeaderDefinition that defines a XSS safe content-type for `send`.
+   */
+  Http::HeaderDefinition getAXssSafeHeaderDefinition(Http::ResponseSendArgument send) {
+    exists(Http::RouteHandler h |
+      send.getRouteHandler() = h and
+      result = xssSafeContentTypeHeader(h)
+    |
+      // The HeaderDefinition affects a response sent at `send`.
+      headerAffects(result, send)
+    )
+  }
+
+  /**
+   * Gets a content-type that may lead to javascript code being executed in the browser.
+   * ref: https://portswigger.net/web-security/cross-site-scripting/cheat-sheet#content-types
+   */
+  string xssUnsafeContentType() {
+    result =
+      [
+        "text/html", "application/xhtml+xml", "application/xml", "text/xml", "image/svg+xml",
+        "text/xsl", "application/vnd.wap.xhtml+xml", "text/rdf", "application/rdf+xml",
+        "application/mathml+xml", "text/vtt", "text/cache-manifest"
+      ]
+  }
+
+  /**
+   * Holds if `h` may send a response with a content type that is safe for XSS.
+   */
+  Http::HeaderDefinition xssSafeContentTypeHeader(Http::RouteHandler h) {
+    result = h.getAResponseHeader("content-type") and
+    not exists(string tp | result.defines("content-type", tp) |
+      tp.toLowerCase().matches(xssUnsafeContentType() + "%")
+    )
+  }
+
   /**
    * Holds if a header set in `header` is likely to affect a response sent at `sender`.
    */
@@ -61,6 +97,7 @@ module ReflectedXss {
       // There is no dominating header, and `header` is non-local.
       not isLocalHeaderDefinition(header) and
       not exists(Http::HeaderDefinition dominatingHeader |
+        dominatingHeader.getAHeaderName() = "content-type" and
         dominatingHeader.getBasicBlock().(ReachableBasicBlock).dominates(sender.getBasicBlock())
       )
     )

javascript/ql/test/library-tests/frameworks/Express/XSS.qll

@@ -0,0 +1,4 @@
+import javascript
+import semmle.javascript.security.dataflow.ReflectedXssCustomizations
+
+query predicate test_Xss(ReflectedXss::Sink sink, Http::ResponseSendArgument res) { sink = res }

javascript/ql/test/library-tests/frameworks/Express/src/express.js

@@ -34,12 +34,12 @@ app.post('/some/other/path', function(req, res) {
 app.get('/', require('./exportedHandler.js').handler);
 
 function getHandler() {
-    return function (req, res){}
+  return function(req, res) { }
 }
 app.use(getHandler());
 
 function getArrowHandler() {
-    return (req, res) => f();
+  return (req, res) => f();
 }
 app.use(getArrowHandler());
 
@@ -49,3 +49,21 @@ app.post('/headers', function(req, res) {
   req.hostname;
   req.headers[config.headerName];
 });
+
+app.get('/some/xss1', function(req, res) {
+  res.header("Content-Type", "text/html");
+  res.send(req.params.foo)
+  foo(res);
+});
+
+app.get('/some/xss2', function(req, res) {
+  res.header("Content-Type", "application/xml");
+  res.send(req.params.foo)
+  foo(res);
+});
+
+app.get('/some/non-xss1', function(req, res) {
+  res.header("Content-Type", "text/plain");
+  res.send(req.params.foo)
+  foo(res);
+});