Merge pull request #9718 from asgerf/js/case-sensitive-middleware

JS: Add 'case sensitive middleware' query

javascript/ql/lib/semmle/javascript/Routing.qll

@@ -148,6 +148,18 @@ module Routing {
       this instanceof MkRouter
     }
 
+    /**
+     * Like `mayResumeDispatch` but without the assumption that functions with an unknown
+     * implementation invoke their continuation.
+     */
+    predicate definitelyResumesDispatch() {
+      this.getLastChild().definitelyResumesDispatch()
+      or
+      exists(this.(RouteHandler).getAContinuationInvocation())
+      or
+      this instanceof MkRouter
+    }
+
     /** Gets the parent of this node, provided that this node may invoke its continuation. */
     private Node getContinuationParent() {
       result = this.getParent() and

javascript/ql/lib/semmle/javascript/frameworks/Express.qll

@@ -33,6 +33,11 @@ module Express {
     or
     // `app = [new] express.Router()`
     result = DataFlow::moduleMember("express", "Router").getAnInvocation()
+    or
+    exists(DataFlow::SourceNode app |
+      app.hasUnderlyingType("probot/lib/application", "Application") and
+      result = app.getAMethodCall("route")
+    )
   }
 
   /**
@@ -1043,4 +1048,22 @@ module Express {
 
     override DataFlow::SourceNode getOutput() { result = this.getCallback(2).getParameter(1) }
   }
+
+  private class ResumeDispatchRefinement extends Routing::RouteHandler {
+    ResumeDispatchRefinement() { this.getFunction() instanceof RouteHandler }
+
+    override predicate mayResumeDispatch() { this.getAParameter().getName() = "next" }
+
+    override predicate definitelyResumesDispatch() { this.getAParameter().getName() = "next" }
+  }
+
+  private class ExpressStaticResumeDispatchRefinement extends Routing::Node {
+    ExpressStaticResumeDispatchRefinement() {
+      this = Routing::getNode(DataFlow::moduleMember("express", "static").getACall())
+    }
+
+    override predicate mayResumeDispatch() { none() }
+
+    override predicate definitelyResumesDispatch() { none() }
+  }
 }

javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.qhelp

@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Using a case-sensitive regular expression path in a middleware route enables an attacker to bypass that middleware
+when accessing an endpoint with a case-insensitive path.
+Paths specified using a string are case-insensitive, whereas regular expressions are case-sensitive by default.
+</p>
+</overview>
+
+<recommendation>
+<p>
+When using a regular expression as a middleware path, make sure the regular expression is
+case-insensitive by adding the <code>i</code> flag.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example restricts access to paths in the <code>/admin</code> path to users logged in as
+administrators:
+</p>
+<sample src="examples/CaseSensitiveMiddlewarePath.js" />
+<p>
+A path such as <code>/admin/users/45</code> can only be accessed by an administrator. However, the path
+<code>/ADMIN/USERS/45</code> can be accessed by anyone because the upper-case path doesn't match the case-sensitive regular expression, whereas
+Express considers it to match the path string <code>/admin/users</code>.
+</p>
+<p>
+The issue can be fixed by adding the <code>i</code> flag to the regular expression:
+</p>
+<sample src="examples/CaseSensitiveMiddlewarePathGood.js" />
+</example>
+
+<references>
+<li>
+MDN
+<a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#advanced_searching_with_flags">Regular Expression Flags</a>.
+</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql

@@ -0,0 +1,121 @@
+/**
+ * @name Case-sensitive middleware path
+ * @description Middleware with case-sensitive paths do not protect endpoints with case-insensitive paths.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 7.3
+ * @precision high
+ * @id js/case-sensitive-middleware-path
+ * @tags security
+ *       external/cwe/cwe-178
+ */
+
+import javascript
+
+/**
+ * Converts `s` to upper case, or to lower-case if it was already upper case.
+ */
+bindingset[s]
+string toOtherCase(string s) {
+  if s.regexpMatch(".*[a-z].*") then result = s.toUpperCase() else result = s.toLowerCase()
+}
+
+RegExpCharacterClass getEnclosingClass(RegExpTerm term) {
+  term = result.getAChild()
+  or
+  term = result.getAChild().(RegExpRange).getAChild()
+}
+
+/**
+ * Holds if `term` seems to distinguish between upper and lower case letters, assuming the `i` flag is not present.
+ */
+pragma[inline]
+predicate isLikelyCaseSensitiveRegExp(RegExpTerm term) {
+  exists(RegExpConstant const |
+    const = term.getAChild*() and
+    const.getValue().regexpMatch(".*[a-zA-Z].*") and
+    not getEnclosingClass(const).getAChild().(RegExpConstant).getValue() =
+      toOtherCase(const.getValue()) and
+    not const.getParent*() instanceof RegExpNegativeLookahead and
+    not const.getParent*() instanceof RegExpNegativeLookbehind
+  )
+}
+
+/**
+ * Gets a string matched by `term`, or part of such a string.
+ */
+string getExampleString(RegExpTerm term) {
+  result = term.getAMatchedString()
+  or
+  // getAMatchedString does not recurse into sequences. Perform one step manually.
+  exists(RegExpSequence seq | seq = term |
+    result =
+      strictconcat(RegExpTerm child, int i, string text |
+        child = seq.getChild(i) and
+        (
+          text = child.getAMatchedString()
+          or
+          not exists(child.getAMatchedString()) and
+          text = ""
+        )
+      |
+        text order by i
+      )
+  )
+}
+
+string getCaseSensitiveBypassExample(RegExpTerm term) {
+  exists(string example |
+    example = getExampleString(term) and
+    result = toOtherCase(example) and
+    result != example // getting an example string is approximate; ensure we got a proper case-change example
+  )
+}
+
+/**
+ * Holds if `setup` has a path-argument `arg` referring to the given case-sensitive `regexp`.
+ */
+predicate isCaseSensitiveMiddleware(
+  Routing::RouteSetup setup, DataFlow::RegExpCreationNode regexp, DataFlow::Node arg
+) {
+  exists(DataFlow::MethodCallNode call |
+    setup = Routing::getRouteSetupNode(call) and
+    (
+      setup.definitelyResumesDispatch()
+      or
+      // If applied to all HTTP methods, be a bit more lenient in detecting middleware
+      setup.mayResumeDispatch() and
+      not exists(setup.getOwnHttpMethod())
+    ) and
+    arg = call.getArgument(0) and
+    regexp.getAReference().flowsTo(arg) and
+    isLikelyCaseSensitiveRegExp(regexp.getRoot()) and
+    exists(string flags |
+      flags = regexp.getFlags() and
+      not RegExp::isIgnoreCase(flags)
+    )
+  )
+}
+
+predicate isGuardedCaseInsensitiveEndpoint(
+  Routing::RouteSetup endpoint, Routing::RouteSetup middleware
+) {
+  isCaseSensitiveMiddleware(middleware, _, _) and
+  exists(DataFlow::MethodCallNode call |
+    endpoint = Routing::getRouteSetupNode(call) and
+    endpoint.isGuardedByNode(middleware) and
+    call.getArgument(0).mayHaveStringValue(_)
+  )
+}
+
+from
+  DataFlow::RegExpCreationNode regexp, Routing::RouteSetup middleware, Routing::RouteSetup endpoint,
+  DataFlow::Node arg, string example
+where
+  isCaseSensitiveMiddleware(middleware, regexp, arg) and
+  example = getCaseSensitiveBypassExample(regexp.getRoot()) and
+  isGuardedCaseInsensitiveEndpoint(endpoint, middleware) and
+  exists(endpoint.getRelativePath().toLowerCase().indexOf(example.toLowerCase()))
+select arg,
+  "This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '"
+    + example + "' will bypass the middleware.", regexp, "pattern", endpoint, "here"

javascript/ql/src/Security/CWE-178/examples/CaseSensitiveMiddlewarePath.js

@@ -0,0 +1,13 @@
+const app = require('express')();
+
+app.use(/\/admin\/.*/, (req, res, next) => {
+    if (!req.user.isAdmin) {
+        res.status(401).send('Unauthorized');
+    } else {
+        next();
+    }
+});
+
+app.get('/admin/users/:id', (req, res) => {
+    res.send(app.database.users[req.params.id]);
+});

javascript/ql/src/Security/CWE-178/examples/CaseSensitiveMiddlewarePathGood.js

@@ -0,0 +1,13 @@
+const app = require('express')();
+
+app.use(/\/admin\/.*/i, (req, res, next) => {
+    if (!req.user.isAdmin) {
+        res.status(401).send('Unauthorized');
+    } else {
+        next();
+    }
+});
+
+app.get('/admin/users/:id', (req, res) => {
+    res.send(app.database.users[req.params.id]);
+});

javascript/ql/src/change-notes/2022-06-27-case-sensitive-middleware.md

@@ -0,0 +1,6 @@
+---
+category: newQuery
+---
+
+- A new query "Case-sensitive middleware path" (`js/case-sensitive-middleware-path`) has been added.
+  It highlights middleware routes that can be bypassed due to having a case-sensitive regular expression path.

javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected

@@ -0,0 +1,3 @@
+| tst.js:8:9:8:19 | /\\/foo\\/.*/ | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO/' will bypass the middleware. | tst.js:8:9:8:19 | /\\/foo\\/.*/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |
+| tst.js:14:5:14:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO' will bypass the middleware. | tst.js:14:5:14:28 | new Reg ... (.*)?') | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |
+| tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | This route uses a case-sensitive path $@, but is guarding a case-insensitive path $@. A path such as '/FOO/' will bypass the middleware. | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | here |

javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.qlref

@@ -0,0 +1 @@
+Security/CWE-178/CaseSensitiveMiddlewarePath.ql

javascript/ql/test/query-tests/Security/CWE-178/charclass.js

@@ -0,0 +1,9 @@
+const express = require('express');
+const app = express();
+
+app.get(/\/[a-zA-Z]+/, (req, res, next) => { // OK - regexp term is case insensitive
+    next();
+});
+
+app.get('/foo', (req, res) => {
+});

javascript/ql/test/query-tests/Security/CWE-178/tst.js

@@ -0,0 +1,61 @@
+const express = require('express');
+const app = express();
+const unknown = require('~something/blah');
+
+app.all(/\/.*/, unknown()); // OK - does not contain letters
+app.all(/\/.*/i, unknown()); // OK
+
+app.all(/\/foo\/.*/, unknown()); // NOT OK
+app.all(/\/foo\/.*/i, unknown()); // OK - case insensitive
+
+app.use(/\/x\/#\d{6}/, express.static('images/')); // OK - not a middleware
+
+app.get(
+    new RegExp('^/foo(.*)?'), // NOT OK - case sensitive
+    unknown(),
+    function(req, res, next) {
+        if (req.params.blah) {
+            next();
+        }
+    }
+);
+
+app.get(
+    new RegExp('^/foo(.*)?', 'i'), // OK - case insensitive
+    unknown(),
+    function(req, res, next) {
+        if (req.params.blah) {
+            next();
+        }
+    }
+);
+
+app.get(
+    new RegExp('^/foo(.*)?'), // OK - not a middleware
+    unknown(),
+    function(req,res) {
+        res.send('Hello World!');
+    }
+);
+
+app.use(/\/foo\/([0-9]+)/, (req, res, next) => { // NOT OK - case sensitive
+    unknown(req);
+    next();
+});
+
+app.use(/\/foo\/([0-9]+)/i, (req, res, next) => { // OK - case insensitive
+    unknown(req);
+    next();
+});
+
+
+app.use(/\/foo\/([0-9]+)/, (req, res) => { // OK - not middleware
+    unknown(req, res);
+});
+
+app.use(/\/foo\/([0-9]+)/i, (req, res) => { // OK - not middleware (also case insensitive)
+    unknown(req, res);
+});
+
+app.get('/foo/:param', (req, res) => { // OK - not a middleware
+});