diff --git a/javascript/ql/lib/change-notes/2025-02-12-express-download.md b/javascript/ql/lib/change-notes/2025-02-12-express-download.md new file mode 100644 index 000000000000..7d9cf337234e --- /dev/null +++ b/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. \ No newline at end of file diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll index 00e2600a8cc1..ffeee7df73ee 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll +++ b/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. */ diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected index c1985970e3b0..8f1786508a59 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected +++ b/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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js index 50e4152e5bf2..f4f289895a86 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/tainted-sendFile.js +++ b/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() }); });