Merge pull request github#4352 from erik-krogh/destructing-redirect

Approved by esbena
max-schaefer · Sep 28, 2020 · 060c19a · 060c19a
2 parents 8a76195 + 664342d
commit 060c19a
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 6 deletions.
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Express.qll b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -195,7 +195,7 @@ module Express {
 
     PassportRouteHandler() { this = any(PassportRouteSetup setup).getARouteHandler() }
 
-    override SimpleParameter getRouteHandlerParameter(string kind) {
+    override Parameter getRouteHandlerParameter(string kind) {
       kind = "request" and
       result = astNode.getParameter(0)
     }
@@ -329,17 +329,17 @@ module Express {
      *
      * `kind` is one of: "error", "request", "response", "next", or "parameter".
      */
-    abstract SimpleParameter getRouteHandlerParameter(string kind);
+    abstract Parameter getRouteHandlerParameter(string kind);
 
     /**
      * Gets the parameter of the route handler that contains the request object.
      */
-    SimpleParameter getRequestParameter() { result = getRouteHandlerParameter("request") }
+    Parameter getRequestParameter() { result = getRouteHandlerParameter("request") }
 
     /**
      * Gets the parameter of the route handler that contains the response object.
      */
-    SimpleParameter getResponseParameter() { result = getRouteHandlerParameter("response") }
+    Parameter getResponseParameter() { result = getRouteHandlerParameter("response") }
 
     /**
      * Gets a request body access of this handler.
@@ -357,7 +357,7 @@ module Express {
 
     StandardRouteHandler() { this = routeSetup.getARouteHandler() }
 
-    override SimpleParameter getRouteHandlerParameter(string kind) {
+    override Parameter getRouteHandlerParameter(string kind) {
       if routeSetup.isParameterHandler()
       then result = getRouteParameterHandlerParameter(astNode, kind)
       else result = getRouteHandlerParameter(astNode, kind)
@@ -890,7 +890,7 @@ module Express {
 
     TrackedRouteHandlerCandidateWithSetup() { this = routeSetup.getARouteHandler() }
 
-    override SimpleParameter getRouteHandlerParameter(string kind) {
+    override Parameter getRouteHandlerParameter(string kind) {
       if routeSetup.isParameterHandler()
       then result = getRouteParameterHandlerParameter(astNode, kind)
       else result = getRouteHandlerParameter(astNode, kind)

diff --git a/...ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected b/...ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected
@@ -44,6 +44,12 @@ nodes
 | express.js:136:16:136:36 | 'u' + r ... ms.user |
 | express.js:136:22:136:36 | req.params.user |
 | express.js:136:22:136:36 | req.params.user |
+| express.js:143:16:143:28 | req.query.foo |
+| express.js:143:16:143:28 | req.query.foo |
+| express.js:143:16:143:28 | req.query.foo |
+| express.js:146:16:146:24 | query.foo |
+| express.js:146:16:146:24 | query.foo |
+| express.js:146:16:146:24 | query.foo |
 | koa.js:6:6:6:27 | url |
 | koa.js:6:12:6:27 | ctx.query.target |
 | koa.js:6:12:6:27 | ctx.query.target |
@@ -128,6 +134,8 @@ edges
 | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
 | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
 | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
+| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo |
+| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo |
 | koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
 | koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
 | koa.js:6:6:6:27 | url | koa.js:8:18:8:20 | url |
@@ -181,6 +189,8 @@ edges
 | express.js:134:16:134:36 | '/' + r ... ms.user | express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:134:22:134:36 | req.params.user | user-provided value |
 | express.js:135:16:135:37 | '//' +  ... ms.user | express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' +  ... ms.user | Untrusted URL redirection due to $@. | express.js:135:23:135:37 | req.params.user | user-provided value |
 | express.js:136:16:136:36 | 'u' + r ... ms.user | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:136:22:136:36 | req.params.user | user-provided value |
+| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | Untrusted URL redirection due to $@. | express.js:143:16:143:28 | req.query.foo | user-provided value |
+| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | Untrusted URL redirection due to $@. | express.js:146:16:146:24 | query.foo | user-provided value |
 | koa.js:7:15:7:17 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:7:15:7:17 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
 | koa.js:8:15:8:26 | `${url}${x}` | koa.js:6:12:6:27 | ctx.query.target | koa.js:8:15:8:26 | `${url}${x}` | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
 | koa.js:14:16:14:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:14:16:14:18 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |

diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js
@@ -138,3 +138,10 @@ app.get('/redirect/:user', function(req, res) {
   res.redirect('/' + ('/u' + req.params.user)); // BAD - could go to //u.evil.com, but not flagged [INCONSISTENCY]
   res.redirect('/u' + req.params.user); // GOOD
 });
+
+app.get("foo", (req, res) => {
+  res.redirect(req.query.foo); // NOT OK
+});
+app.get("bar", ({query}, res) => {
+  res.redirect(query.foo); // NOT OK
+})