Merge pull request #18749 from Kwstubbs/express

JS: Add result.download to Express as Path Traversal Sink

Author: asgerf

javascript/ql/lib/change-notes/2025-02-12-express-download.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `response.download()` function in `express` is now recognized as a sink for path traversal attacks.

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

@@ -960,6 +960,23 @@ module Express {
     }
   }
 
+  /** A call to `response.download`, considered as a file system access. */
+  private class ResponseDownloadAsFileSystemAccess extends FileSystemReadAccess,
+    DataFlow::MethodCallNode
+  {
+    ResponseDownloadAsFileSystemAccess() {
+      exists(string name | name = "download" | this.calls(any(ResponseNode res), name))
+    }
+
+    override DataFlow::Node getADataNode() { none() }
+
+    override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
+
+    override DataFlow::Node getRootPathArgument() {
+      result = this.(DataFlow::CallNode).getOptionArgument([1, 2], "root")
+    }
+  }
+
   /**
    * A function that flows to a route setup.
    */

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected

@@ -450,6 +450,12 @@ nodes
 | tainted-sendFile.js:24:37:24:48 | req.params.x | semmle.label | req.params.x |
 | tainted-sendFile.js:25:16:25:46 | path.jo ... rams.x) | semmle.label | path.jo ... rams.x) |
 | tainted-sendFile.js:25:34:25:45 | req.params.x | semmle.label | req.params.x |
+| tainted-sendFile.js:30:16:30:33 | req.param("gimme") | semmle.label | req.param("gimme") |
+| tainted-sendFile.js:33:16:33:48 | homeDir ... arams.x | semmle.label | homeDir ... arams.x |
+| tainted-sendFile.js:33:37:33:48 | req.params.x | semmle.label | req.params.x |
+| tainted-sendFile.js:35:16:35:46 | path.jo ... rams.x) | semmle.label | path.jo ... rams.x) |
+| tainted-sendFile.js:35:34:35:45 | req.params.x | semmle.label | req.params.x |
+| tainted-sendFile.js:38:43:38:58 | req.param("dir") | semmle.label | req.param("dir") |
 | tainted-string-steps.js:6:7:6:48 | path | semmle.label | path |
 | tainted-string-steps.js:6:14:6:37 | url.par ... , true) | semmle.label | url.par ... , true) |
 | tainted-string-steps.js:6:14:6:43 | url.par ... ).query | semmle.label | url.par ... ).query |
@@ -895,6 +901,8 @@ edges
 | tainted-promise-steps.js:12:20:12:23 | path | tainted-promise-steps.js:12:44:12:47 | path | provenance |  |
 | tainted-sendFile.js:24:37:24:48 | req.params.x | tainted-sendFile.js:24:16:24:49 | path.re ... rams.x) | provenance | Config |
 | tainted-sendFile.js:25:34:25:45 | req.params.x | tainted-sendFile.js:25:16:25:46 | path.jo ... rams.x) | provenance | Config |
+| tainted-sendFile.js:33:37:33:48 | req.params.x | tainted-sendFile.js:33:16:33:48 | homeDir ... arams.x | provenance | Config |
+| tainted-sendFile.js:35:34:35:45 | req.params.x | tainted-sendFile.js:35:16:35:46 | path.jo ... rams.x) | provenance | Config |
 | tainted-string-steps.js:6:7:6:48 | path | tainted-string-steps.js:8:18:8:21 | path | provenance |  |
 | tainted-string-steps.js:6:7:6:48 | path | tainted-string-steps.js:9:18:9:21 | path | provenance |  |
 | tainted-string-steps.js:6:7:6:48 | path | tainted-string-steps.js:10:18:10:21 | path | provenance |  |
@@ -1114,6 +1122,10 @@ subpaths
 | tainted-sendFile.js:18:43:18:58 | req.param("dir") | tainted-sendFile.js:18:43:18:58 | req.param("dir") | tainted-sendFile.js:18:43:18:58 | req.param("dir") | This path depends on a $@. | tainted-sendFile.js:18:43:18:58 | req.param("dir") | user-provided value |
 | tainted-sendFile.js:24:16:24:49 | path.re ... rams.x) | tainted-sendFile.js:24:37:24:48 | req.params.x | tainted-sendFile.js:24:16:24:49 | path.re ... rams.x) | This path depends on a $@. | tainted-sendFile.js:24:37:24:48 | req.params.x | user-provided value |
 | tainted-sendFile.js:25:16:25:46 | path.jo ... rams.x) | tainted-sendFile.js:25:34:25:45 | req.params.x | tainted-sendFile.js:25:16:25:46 | path.jo ... rams.x) | This path depends on a $@. | tainted-sendFile.js:25:34:25:45 | req.params.x | user-provided value |
+| tainted-sendFile.js:30:16:30:33 | req.param("gimme") | tainted-sendFile.js:30:16:30:33 | req.param("gimme") | tainted-sendFile.js:30:16:30:33 | req.param("gimme") | This path depends on a $@. | tainted-sendFile.js:30:16:30:33 | req.param("gimme") | user-provided value |
+| tainted-sendFile.js:33:16:33:48 | homeDir ... arams.x | tainted-sendFile.js:33:37:33:48 | req.params.x | tainted-sendFile.js:33:16:33:48 | homeDir ... arams.x | This path depends on a $@. | tainted-sendFile.js:33:37:33:48 | req.params.x | user-provided value |
+| tainted-sendFile.js:35:16:35:46 | path.jo ... rams.x) | tainted-sendFile.js:35:34:35:45 | req.params.x | tainted-sendFile.js:35:16:35:46 | path.jo ... rams.x) | This path depends on a $@. | tainted-sendFile.js:35:34:35:45 | req.params.x | user-provided value |
+| tainted-sendFile.js:38:43:38:58 | req.param("dir") | tainted-sendFile.js:38:43:38:58 | req.param("dir") | tainted-sendFile.js:38:43:38:58 | req.param("dir") | This path depends on a $@. | tainted-sendFile.js:38:43:38:58 | req.param("dir") | user-provided value |
 | tainted-string-steps.js:8:18:8:34 | path.substring(4) | tainted-string-steps.js:6:24:6:30 | req.url | tainted-string-steps.js:8:18:8:34 | path.substring(4) | This path depends on a $@. | tainted-string-steps.js:6:24:6:30 | req.url | user-provided value |
 | tainted-string-steps.js:9:18:9:37 | path.substring(0, i) | tainted-string-steps.js:6:24:6:30 | req.url | tainted-string-steps.js:9:18:9:37 | path.substring(0, i) | This path depends on a $@. | tainted-string-steps.js:6:24:6:30 | req.url | user-provided value |
 | tainted-string-steps.js:10:18:10:31 | path.substr(4) | tainted-string-steps.js:6:24:6:30 | req.url | tainted-string-steps.js:10:18:10:31 | path.substr(4) | This path depends on a $@. | tainted-string-steps.js:6:24:6:30 | req.url | user-provided value |

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js

@@ -25,4 +25,18 @@ app.get('/some/path/:x', function(req, res) {
   res.sendfile(path.join('data', req.params.x)); // NOT OK
 
   res.sendFile(homeDir + path.join('data', req.params.x)); // kinda OK - can only escape from 'data/'
+
+  // BAD: downloading a file based on un-sanitized query parameters
+  res.download(req.param("gimme"));
+
+  // BAD: download allows ../
+  res.download(homeDir + '/data/' + req.params.x);
+
+  res.download(path.join('data', req.params.x)); // NOT OK
+
+  // BAD: doesn't help if user controls root
+  res.download(req.param("file"), { root: req.param("dir") });
+
+  // GOOD: ensures files cannot be accessed outside of root folder
+  res.download(req.param("gimme"), { root: process.cwd() });
 });