From 533f1a93e2d500bd63dd5915c0408526c0ebb97f Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 2 Apr 2025 14:50:44 +0200 Subject: [PATCH 1/4] JS: Added test cases for `mkdirp`. --- .../CWE-022/TaintedPath/TaintedPath.expected | 13 +++++++++++ .../Security/CWE-022/TaintedPath/mkdirp.js | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js 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 99be2545b8e3..28cbc6908d60 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,8 @@ | 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 | +| mkdirp.js:11:12:11:18 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:11:12:11:18 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:12:17:12:23 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:12:17:12:23 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | 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 | @@ -390,6 +392,11 @@ 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 | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:11:12:11:18 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:12:17:12:23 | dirPath | provenance | | +| mkdirp.js:9:21:9:76 | path.jo ... ltDir') | mkdirp.js:9:11:9:76 | dirPath | provenance | | +| mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:9:42:9:75 | req.que ... ultDir' | provenance | | +| mkdirp.js:9:42:9:75 | req.que ... ultDir' | mkdirp.js:9:21:9:76 | path.jo ... ltDir') | provenance | Config | | 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 | | @@ -919,6 +926,12 @@ 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 | +| mkdirp.js:9:11:9:76 | dirPath | semmle.label | dirPath | +| mkdirp.js:9:21:9:76 | path.jo ... ltDir') | semmle.label | path.jo ... ltDir') | +| mkdirp.js:9:42:9:59 | req.query.filename | semmle.label | req.query.filename | +| mkdirp.js:9:42:9:75 | req.que ... ultDir' | semmle.label | req.que ... ultDir' | +| mkdirp.js:11:12:11:18 | dirPath | semmle.label | dirPath | +| mkdirp.js:12:17:12:23 | dirPath | semmle.label | dirPath | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js new file mode 100644 index 000000000000..06518acbc0c2 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js @@ -0,0 +1,22 @@ +const express = require('express'); +const mkdirp = require('mkdirp'); +const path = require('path'); + +const app = express(); +app.use(express.json()); + +app.post('/foo', async (req, res) => { + const dirPath = path.join(__dirname, req.query.filename || 'defaultDir'); // $ Source + + mkdirp(dirPath); // $ Alert + mkdirp.sync(dirPath); // $ Alert + mkdirp.nativeSync(dirPath); // $ MISSING: Alert + mkdirp.native(dirPath); // $ MISSING: Alert + mkdirp.manual(dirPath); // $ MISSING: Alert + mkdirp.manualSync(dirPath); // $ MISSING: Alert + mkdirp.mkdirpNative(dirPath); // $ MISSING: Alert + mkdirp.mkdirpManual(dirPath); // $ MISSING: Alert + mkdirp.mkdirpManualSync(dirPath); // $ MISSING: Alert + mkdirp.mkdirpNativeSync(dirPath); // $ MISSING: Alert + mkdirp.mkdirpSync(dirPath); // $ MISSING: Alert +}); From 3fa24d60261f4ee0945d66ac889ba19f182aac10 Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 2 Apr 2025 16:29:18 +0200 Subject: [PATCH 2/4] Add sink model for `mkdirp` and update tests for path injection alerts. --- javascript/ql/lib/ext/mkdirp.model.yml | 6 +++++ .../CWE-022/TaintedPath/TaintedPath.expected | 27 +++++++++++++++++++ .../Security/CWE-022/TaintedPath/mkdirp.js | 18 ++++++------- 3 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 javascript/ql/lib/ext/mkdirp.model.yml diff --git a/javascript/ql/lib/ext/mkdirp.model.yml b/javascript/ql/lib/ext/mkdirp.model.yml new file mode 100644 index 000000000000..488b46e2ee2e --- /dev/null +++ b/javascript/ql/lib/ext/mkdirp.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: sinkModel + data: + - ["mkdirp", "Member[nativeSync,native,manual,manualSync,mkdirpNative,mkdirpManual,mkdirpManualSync,mkdirpNativeSync,mkdirpSync].Argument[0]", "path-injection"] 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 28cbc6908d60..b486ca5e6638 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 @@ -54,6 +54,15 @@ | 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 | | mkdirp.js:11:12:11:18 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:11:12:11:18 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | | mkdirp.js:12:17:12:23 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:12:17:12:23 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:13:23:13:29 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:13:23:13:29 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:14:19:14:25 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:14:19:14:25 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:15:19:15:25 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:15:19:15:25 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:16:23:16:29 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:16:23:16:29 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:17:25:17:31 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:17:25:17:31 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:18:25:18:31 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:18:25:18:31 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:19:29:19:35 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:19:29:19:35 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:20:29:20:35 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:20:29:20:35 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | user-provided value | +| mkdirp.js:21:23:21:29 | dirPath | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:21:23:21:29 | dirPath | This path depends on a $@. | mkdirp.js:9:42:9:59 | req.query.filename | 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 | @@ -394,6 +403,15 @@ edges | hapi.js:14:30:14:51 | request ... ilepath | hapi.js:14:19:14:51 | filepath | provenance | | | mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:11:12:11:18 | dirPath | provenance | | | mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:12:17:12:23 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:13:23:13:29 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:14:19:14:25 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:15:19:15:25 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:16:23:16:29 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:17:25:17:31 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:18:25:18:31 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:19:29:19:35 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:20:29:20:35 | dirPath | provenance | | +| mkdirp.js:9:11:9:76 | dirPath | mkdirp.js:21:23:21:29 | dirPath | provenance | | | mkdirp.js:9:21:9:76 | path.jo ... ltDir') | mkdirp.js:9:11:9:76 | dirPath | provenance | | | mkdirp.js:9:42:9:59 | req.query.filename | mkdirp.js:9:42:9:75 | req.que ... ultDir' | provenance | | | mkdirp.js:9:42:9:75 | req.que ... ultDir' | mkdirp.js:9:21:9:76 | path.jo ... ltDir') | provenance | Config | @@ -932,6 +950,15 @@ nodes | mkdirp.js:9:42:9:75 | req.que ... ultDir' | semmle.label | req.que ... ultDir' | | mkdirp.js:11:12:11:18 | dirPath | semmle.label | dirPath | | mkdirp.js:12:17:12:23 | dirPath | semmle.label | dirPath | +| mkdirp.js:13:23:13:29 | dirPath | semmle.label | dirPath | +| mkdirp.js:14:19:14:25 | dirPath | semmle.label | dirPath | +| mkdirp.js:15:19:15:25 | dirPath | semmle.label | dirPath | +| mkdirp.js:16:23:16:29 | dirPath | semmle.label | dirPath | +| mkdirp.js:17:25:17:31 | dirPath | semmle.label | dirPath | +| mkdirp.js:18:25:18:31 | dirPath | semmle.label | dirPath | +| mkdirp.js:19:29:19:35 | dirPath | semmle.label | dirPath | +| mkdirp.js:20:29:20:35 | dirPath | semmle.label | dirPath | +| mkdirp.js:21:23:21:29 | dirPath | semmle.label | dirPath | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js index 06518acbc0c2..975c46df2cce 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/mkdirp.js @@ -10,13 +10,13 @@ app.post('/foo', async (req, res) => { mkdirp(dirPath); // $ Alert mkdirp.sync(dirPath); // $ Alert - mkdirp.nativeSync(dirPath); // $ MISSING: Alert - mkdirp.native(dirPath); // $ MISSING: Alert - mkdirp.manual(dirPath); // $ MISSING: Alert - mkdirp.manualSync(dirPath); // $ MISSING: Alert - mkdirp.mkdirpNative(dirPath); // $ MISSING: Alert - mkdirp.mkdirpManual(dirPath); // $ MISSING: Alert - mkdirp.mkdirpManualSync(dirPath); // $ MISSING: Alert - mkdirp.mkdirpNativeSync(dirPath); // $ MISSING: Alert - mkdirp.mkdirpSync(dirPath); // $ MISSING: Alert + mkdirp.nativeSync(dirPath); // $ Alert + mkdirp.native(dirPath); // $ Alert + mkdirp.manual(dirPath); // $ Alert + mkdirp.manualSync(dirPath); // $ Alert + mkdirp.mkdirpNative(dirPath); // $ Alert + mkdirp.mkdirpManual(dirPath); // $ Alert + mkdirp.mkdirpManualSync(dirPath); // $ Alert + mkdirp.mkdirpNativeSync(dirPath); // $ Alert + mkdirp.mkdirpSync(dirPath); // $ Alert }); From 04a39eb735f7e97288f647453a29890e56bd149e Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 2 Apr 2025 16:33:02 +0200 Subject: [PATCH 3/4] Removed old `mkdirp` modeling and replaced it with `MaD`. --- javascript/ql/lib/ext/mkdirp.model.yml | 3 ++- .../ql/lib/semmle/javascript/frameworks/Files.qll | 13 ------------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/javascript/ql/lib/ext/mkdirp.model.yml b/javascript/ql/lib/ext/mkdirp.model.yml index 488b46e2ee2e..1a3bcce767c5 100644 --- a/javascript/ql/lib/ext/mkdirp.model.yml +++ b/javascript/ql/lib/ext/mkdirp.model.yml @@ -3,4 +3,5 @@ extensions: pack: codeql/javascript-all extensible: sinkModel data: - - ["mkdirp", "Member[nativeSync,native,manual,manualSync,mkdirpNative,mkdirpManual,mkdirpManualSync,mkdirpNativeSync,mkdirpSync].Argument[0]", "path-injection"] + - ["mkdirp", "Member[nativeSync,native,manual,manualSync,mkdirpNative,mkdirpManual,mkdirpManualSync,mkdirpNativeSync,mkdirpSync,sync].Argument[0]", "path-injection"] + - ["mkdirp", "Argument[0]", "path-injection"] diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Files.qll b/javascript/ql/lib/semmle/javascript/frameworks/Files.qll index 853d122b3019..30f07396dccd 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Files.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Files.qll @@ -427,16 +427,3 @@ class Chokidar extends FileNameProducer, FileSystemAccess, API::CallNode { ) } } - -/** - * A call to the [`mkdirp`](https://www.npmjs.com/package/mkdirp) library. - */ -private class Mkdirp extends FileSystemAccess, API::CallNode { - Mkdirp() { - this = API::moduleImport("mkdirp").getACall() - or - this = API::moduleImport("mkdirp").getMember("sync").getACall() - } - - override DataFlow::Node getAPathArgument() { result = this.getArgument(0) } -} From 0e7bff0f81bfc2321023d6f09b62582aeae2319f Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 2 Apr 2025 16:34:06 +0200 Subject: [PATCH 4/4] Added change note. --- javascript/ql/lib/change-notes/2025-04-02-mkdirp.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2025-04-02-mkdirp.md diff --git a/javascript/ql/lib/change-notes/2025-04-02-mkdirp.md b/javascript/ql/lib/change-notes/2025-04-02-mkdirp.md new file mode 100644 index 000000000000..132bbf0cbe4f --- /dev/null +++ b/javascript/ql/lib/change-notes/2025-04-02-mkdirp.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added support for additional `mkdirp` methods as sinks in path-injection queries.