From d0ed0fdeb368209abdbdfad48c4708d7470c1a5a Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 12 Feb 2025 00:10:09 -0800 Subject: [PATCH 1/3] Add download to Express --- .../2025-02-12-express-download.md | 4 ++++ .../semmle/javascript/frameworks/Express.qll | 19 +++++++++++++++++++ .../CWE-022/TaintedPath/TaintedPath.expected | 12 ++++++++++++ .../CWE-022/TaintedPath/tainted-sendFile.js | 14 ++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-02-12-express-download.md 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..b47c7a843996 --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-02-12-express-download.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added result.download() function to ResponseDownloadAsFileSystemAccess to FileSystemReadAccess \ 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..7b8036671e29 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll @@ -960,6 +960,25 @@ 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() }); }); From f5521ca1b84ee683177520699f311d9d9c294987 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 12 Feb 2025 00:15:27 -0800 Subject: [PATCH 2/3] Formatting --- javascript/ql/lib/semmle/javascript/frameworks/Express.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll index 7b8036671e29..ffeee7df73ee 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll @@ -965,9 +965,7 @@ module Express { DataFlow::MethodCallNode { ResponseDownloadAsFileSystemAccess() { - exists(string name | name = "download" | - this.calls(any(ResponseNode res), name) - ) + exists(string name | name = "download" | this.calls(any(ResponseNode res), name)) } override DataFlow::Node getADataNode() { none() } From 253882c3d1a32e4d720cde739a02e7e1f5447865 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 12 Feb 2025 11:01:29 -0800 Subject: [PATCH 3/3] Update javascript/ql/lib/change-notes/2025-02-12-express-download.md Co-authored-by: Asger F --- javascript/ql/lib/change-notes/2025-02-12-express-download.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index b47c7a843996..7d9cf337234e 100644 --- a/javascript/ql/lib/change-notes/2025-02-12-express-download.md +++ b/javascript/ql/lib/change-notes/2025-02-12-express-download.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Added result.download() function to ResponseDownloadAsFileSystemAccess to FileSystemReadAccess \ No newline at end of file +* The `response.download()` function in `express` is now recognized as a sink for path traversal attacks. \ No newline at end of file