Merge pull request #259 from asger-semmle/open-redirect-expr

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -27,5 +27,6 @@
 | Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. |
 | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
 | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
+| Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
 
 ## Changes to QL libraries

javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll

@@ -21,10 +21,10 @@ module ServerSideUrlRedirect {
      * Holds if this sink may redirect to a non-local URL.
      */
     predicate maybeNonLocal() {
-      exists (Expr prefix | prefix = getAPrefix(this) |
-        not exists(prefix.getStringValue())
+      exists (DataFlow::Node prefix | prefix = getAPrefix(this) |
+        not exists(prefix.asExpr().getStringValue())
         or
-        exists (string prefixVal | prefixVal = prefix.getStringValue() |
+        exists (string prefixVal | prefixVal = prefix.asExpr().getStringValue() |
           // local URLs (i.e., URLs that start with `/` not followed by `\` or `/`,
           // or that start with `~/`) are unproblematic
           not prefixVal.regexpMatch("/[^\\\\/].*|~/.*") and
@@ -47,12 +47,12 @@ module ServerSideUrlRedirect {
   /**
    * Gets an expression that may end up being a prefix of the string concatenation `nd`.
    */
-  private Expr getAPrefix(Sink sink) {
+  private DataFlow::Node getAPrefix(Sink sink) {
     exists (DataFlow::Node prefix |
       prefix = prefixCandidate(sink) and
       not exists(StringConcatenation::getFirstOperand(prefix)) and
       not exists(prefix.getAPredecessor()) and
-      result = prefix.asExpr()
+      result = prefix
     )
   }
 

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected

@@ -3,6 +3,7 @@
 | express.js:33:18:33:23 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value |
 | express.js:35:16:35:21 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value |
 | express.js:44:16:44:108 | (req.pa ... ntacts" | Untrusted URL redirection due to $@. | express.js:44:69:44:87 | req.param('action') | user-provided value |
+| express.js:53:26:53:28 | url | Untrusted URL redirection due to $@. | express.js:48:16:48:28 | req.params[0] | user-provided value |
 | express.js:78:16:78:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:78:19:78:37 | req.param("target") | user-provided value |
 | express.js:94:18:94:23 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |
 | express.js:101:16:101:21 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value |

javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js

@@ -121,3 +121,12 @@ app.get('/array/join', function(req, res) {
   // BAD: request input becomes before query string
   res.redirect([req.query.page, '?section=', req.query.section].join(''));
 });
+
+function sendUserToUrl(res, nextUrl) {
+  // BAD: value comes from query parameter
+  res.redrect(nextUrl);
+}
+
+app.get('/call', function(req, res) {
+  sendUserToUrl(res, req.query.nextUrl);
+});