From 178da21fb8868280826e3d1dbec5a1e43f673717 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 25 Nov 2024 11:53:00 +0100 Subject: [PATCH 1/4] JS: Added test case for CWE-178 RegExp with unknown flags --- .../CWE-178/CaseSensitiveMiddlewarePath.expected | 1 + .../ql/test/query-tests/Security/CWE-178/tst.js | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected index 2195bbf00297..2be1780188a0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected @@ -2,6 +2,7 @@ | tst.js:14:5:14:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/FOO/1' will bypass the middleware. | tst.js:14:5:14:28 | new Reg ... (.*)?') | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/FOO/1' will bypass the middleware. | tst.js:41:9:41:25 | /\\/foo\\/([0-9]+)/ | pattern | tst.js:60:1:61:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:64:5:64:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:64:5:64:28 | new Reg ... (.*)?') | pattern | tst.js:73:1:74:2 | app.get ... ware\\n}) | case-insensitive path | +| tst.js:64:5:64:28 | new Reg ... (.*)?') | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:64:5:64:28 | new Reg ... (.*)?') | pattern | tst.js:107:1:108:2 | app.get ... ware\\n}) | case-insensitive path | | tst.js:76:9:76:20 | /\\/baz\\/bla/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ/BLA' will bypass the middleware. | tst.js:76:9:76:20 | /\\/baz\\/bla/ | pattern | tst.js:77:1:79:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ3/A' will bypass the middleware. | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | pattern | tst.js:87:1:89:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/CURRENTGAME' will bypass the middleware. | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | pattern | tst.js:93:1:95:2 | app.get ... O");\\n}) | case-insensitive path | diff --git a/javascript/ql/test/query-tests/Security/CWE-178/tst.js b/javascript/ql/test/query-tests/Security/CWE-178/tst.js index 5fcc3cc94a0f..3dd24029ebe0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-178/tst.js @@ -93,3 +93,16 @@ app.use(/\/summonerByName|\/currentGame/,apiLimit1, apiLimit2); app.get('/currentGame', function (req, res) { res.send("FOO"); }); + +app.get( + new RegExp('^/bar(.*)?', unknownFlag()), // NOT OK - Currently not flagged. + unknown(), + function(req, res, next) { + if (req.params.blah) { + next(); + } + } +); + +app.get('/bar/*', (req, res) => { // OK - not a middleware +}); From e38b63ebcda88f698ab728c8d2aabd12cef06c51 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 25 Nov 2024 11:56:06 +0100 Subject: [PATCH 2/4] JS: previously js/case-sensitive-middleware-path was not taking into consideration unknown flags --- .../ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql | 2 +- .../Security/CWE-178/CaseSensitiveMiddlewarePath.expected | 1 + javascript/ql/test/query-tests/Security/CWE-178/tst.js | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql index d16f72d4172a..2045d801c702 100644 --- a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql +++ b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql @@ -65,7 +65,7 @@ predicate isCaseSensitiveMiddleware( arg = call.getArgument(0) and regexp.getAReference().flowsTo(arg) and exists(string flags | - flags = regexp.getFlags() and + flags = regexp.tryGetFlags() and not RegExp::isIgnoreCase(flags) ) ) diff --git a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected index 2be1780188a0..cb1720260ef8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected @@ -6,3 +6,4 @@ | tst.js:76:9:76:20 | /\\/baz\\/bla/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ/BLA' will bypass the middleware. | tst.js:76:9:76:20 | /\\/baz\\/bla/ | pattern | tst.js:77:1:79:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ3/A' will bypass the middleware. | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | pattern | tst.js:87:1:89:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/CURRENTGAME' will bypass the middleware. | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | pattern | tst.js:93:1:95:2 | app.get ... O");\\n}) | case-insensitive path | +| tst.js:98:5:98:43 | new Reg ... Flag()) | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:98:5:98:43 | new Reg ... Flag()) | pattern | tst.js:107:1:108:2 | app.get ... ware\\n}) | case-insensitive path | diff --git a/javascript/ql/test/query-tests/Security/CWE-178/tst.js b/javascript/ql/test/query-tests/Security/CWE-178/tst.js index 3dd24029ebe0..69a203cb8df0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-178/tst.js @@ -95,7 +95,7 @@ app.get('/currentGame', function (req, res) { }); app.get( - new RegExp('^/bar(.*)?', unknownFlag()), // NOT OK - Currently not flagged. + new RegExp('^/bar(.*)?', unknownFlag()), // NOT OK - Might be OK if the unknown flag evaluates to case insensitive one unknown(), function(req, res, next) { if (req.params.blah) { From d6372aebc76cb196665f34e05671b0006bc6445c Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Mon, 25 Nov 2024 12:11:07 +0100 Subject: [PATCH 3/4] Update javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql Co-authored-by: Erik Krogh Kristensen --- .../ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql | 2 +- .../Security/CWE-178/CaseSensitiveMiddlewarePath.expected | 1 - javascript/ql/test/query-tests/Security/CWE-178/tst.js | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql index 2045d801c702..997ee8971a5a 100644 --- a/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql +++ b/javascript/ql/src/Security/CWE-178/CaseSensitiveMiddlewarePath.ql @@ -66,7 +66,7 @@ predicate isCaseSensitiveMiddleware( regexp.getAReference().flowsTo(arg) and exists(string flags | flags = regexp.tryGetFlags() and - not RegExp::isIgnoreCase(flags) + not RegExp::maybeIgnoreCase(flags) ) ) } diff --git a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected index cb1720260ef8..2be1780188a0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-178/CaseSensitiveMiddlewarePath.expected @@ -6,4 +6,3 @@ | tst.js:76:9:76:20 | /\\/baz\\/bla/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ/BLA' will bypass the middleware. | tst.js:76:9:76:20 | /\\/baz\\/bla/ | pattern | tst.js:77:1:79:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAZ3/A' will bypass the middleware. | tst.js:86:9:86:30 | /\\/[Bb] ... 3\\/[a]/ | pattern | tst.js:87:1:89:2 | app.get ... });\\n}) | case-insensitive path | | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/CURRENTGAME' will bypass the middleware. | tst.js:91:9:91:40 | /\\/summ ... ntGame/ | pattern | tst.js:93:1:95:2 | app.get ... O");\\n}) | case-insensitive path | -| tst.js:98:5:98:43 | new Reg ... Flag()) | This route uses a case-sensitive path $@, but is guarding a $@. A path such as '/BAR/1' will bypass the middleware. | tst.js:98:5:98:43 | new Reg ... Flag()) | pattern | tst.js:107:1:108:2 | app.get ... ware\\n}) | case-insensitive path | diff --git a/javascript/ql/test/query-tests/Security/CWE-178/tst.js b/javascript/ql/test/query-tests/Security/CWE-178/tst.js index 69a203cb8df0..81bbe993291a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-178/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-178/tst.js @@ -95,7 +95,7 @@ app.get('/currentGame', function (req, res) { }); app.get( - new RegExp('^/bar(.*)?', unknownFlag()), // NOT OK - Might be OK if the unknown flag evaluates to case insensitive one + new RegExp('^/bar(.*)?', unknownFlag()), // OK - Might be OK if the unknown flag evaluates to case insensitive one unknown(), function(req, res, next) { if (req.params.blah) { From b39a8fe3d16fa0e376253a7eff71e69397297b54 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 25 Nov 2024 12:36:21 +0100 Subject: [PATCH 4/4] JS: In oprder to keep code more consistent removed predicate isGlobal from RegExpCreationNode and reused RegExp::isGlobal in std --- javascript/ql/lib/semmle/javascript/StandardLibrary.qll | 4 +++- javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/StandardLibrary.qll b/javascript/ql/lib/semmle/javascript/StandardLibrary.qll index b40f10d93691..f33efd59f779 100644 --- a/javascript/ql/lib/semmle/javascript/StandardLibrary.qll +++ b/javascript/ql/lib/semmle/javascript/StandardLibrary.qll @@ -115,7 +115,9 @@ class StringReplaceCall extends DataFlow::MethodCallNode { * Holds if this is a global replacement, that is, the first argument is a regular expression * with the `g` flag, or this is a call to `.replaceAll()`. */ - predicate isGlobal() { this.getRegExp().isGlobal() or this.getMethodName() = "replaceAll" } + predicate isGlobal() { + RegExp::isGlobal(this.getRegExp().getFlags()) or this.getMethodName() = "replaceAll" + } /** * Holds if this call to `replace` replaces `old` with `new`. diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll index d88dab4d4317..59ed567e392c 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Nodes.qll @@ -1682,9 +1682,6 @@ class RegExpCreationNode extends DataFlow::SourceNode { result = this.(RegExpLiteralNode).getFlags() } - /** Holds if the constructed predicate has the `g` flag. */ - predicate isGlobal() { RegExp::isGlobal(this.getFlags()) } - /** Gets a data flow node referring to this regular expression. */ private DataFlow::SourceNode getAReference(DataFlow::TypeTracker t) { t.start() and