diff --git a/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md b/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md new file mode 100644 index 000000000000..74d7c3f34c2d --- /dev/null +++ b/javascript/ql/lib/change-notes/2024-11-11-matchAll-support.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +Added support for `String.prototype.matchAll`. diff --git a/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll b/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll index 21f4cc1b1c5c..da9e90744ef0 100644 --- a/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll +++ b/javascript/ql/lib/semmle/javascript/MembershipCandidates.qll @@ -193,7 +193,7 @@ module MembershipCandidate { or // u.match(/re/) or u.match("re") base = this and - m = "match" and + m = ["match", "matchAll"] and enumeration = RegExp::getRegExpFromNode(firstArg) ) } diff --git a/javascript/ql/lib/semmle/javascript/Regexp.qll b/javascript/ql/lib/semmle/javascript/Regexp.qll index 27ad339c733e..df0f76e7cda6 100644 --- a/javascript/ql/lib/semmle/javascript/Regexp.qll +++ b/javascript/ql/lib/semmle/javascript/Regexp.qll @@ -938,7 +938,7 @@ private predicate isMatchObjectProperty(string name) { /** Holds if `call` is a call to `match` whose result is used in a way that is incompatible with Match objects. */ private predicate isUsedAsNonMatchObject(DataFlow::MethodCallNode call) { - call.getMethodName() = "match" and + call.getMethodName() = ["match", "matchAll"] and call.getNumArgument() = 1 and ( // Accessing a property that is absent on Match objects @@ -996,7 +996,7 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) { not isNativeStringMethod(func, methodName) ) | - methodName = "match" and + methodName = ["match", "matchAll"] and source = mce.getArgument(0) and mce.getNumArgument() = 1 and not isUsedAsNonMatchObject(mce) diff --git a/javascript/ql/lib/semmle/javascript/StringOps.qll b/javascript/ql/lib/semmle/javascript/StringOps.qll index 6b7820e964dd..f2b7f3eb9aa7 100644 --- a/javascript/ql/lib/semmle/javascript/StringOps.qll +++ b/javascript/ql/lib/semmle/javascript/StringOps.qll @@ -722,7 +722,7 @@ module StringOps { } private class MatchCall extends DataFlow::MethodCallNode { - MatchCall() { this.getMethodName() = "match" } + MatchCall() { this.getMethodName() = ["match", "matchAll"] } } private class ExecCall extends DataFlow::MethodCallNode { diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll index 11ce802ac720..989c551ab2fa 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll @@ -716,7 +716,7 @@ module TaintTracking { pragma[nomagic] private DataFlow::MethodCallNode matchMethodCall() { - result.getMethodName() = "match" and + result.getMethodName() = ["match", "matchAll"] and exists(DataFlow::AnalyzedNode analyzed | pragma[only_bind_into](analyzed) = result.getArgument(0).analyze() and analyzed.getAType() = TTRegExp() @@ -917,7 +917,7 @@ module TaintTracking { */ private ControlFlowNode getACaptureSetter(DataFlow::Node input) { exists(DataFlow::MethodCallNode call | result = call.asExpr() | - call.getMethodName() = ["search", "replace", "replaceAll", "match"] and + call.getMethodName() = ["search", "replace", "replaceAll", "match", "matchAll"] and input = call.getReceiver() or call.getMethodName() = ["test", "exec"] and input = call.getArgument(0) @@ -998,7 +998,7 @@ module TaintTracking { or // u.match(/re/) or u.match("re") base = expr and - m = "match" and + m = ["match", "matchAll"] and RegExp::isGenericRegExpSanitizer(RegExp::getRegExpFromNode(firstArg.flow()), sanitizedOutcome) ) diff --git a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql index 858dedc603e5..1057f9ccca50 100644 --- a/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql +++ b/javascript/ql/src/Security/CWE-020/MissingRegExpAnchor.ql @@ -35,7 +35,7 @@ private module Impl implements | name = "replace" or - name = "match" and exists(mcn.getAPropertyRead()) + name = ["match", "matchAll"] and exists(mcn.getAPropertyRead()) ) ) } diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected index 848264b661b1..b8f58cb4c785 100644 --- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected +++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected @@ -51,6 +51,10 @@ nodes | check-regex.js:41:13:41:43 | "test.c ... tainted | | check-regex.js:41:27:41:43 | req.query.tainted | | check-regex.js:41:27:41:43 | req.query.tainted | +| check-regex.js:61:15:61:42 | baseURL ... tainted | +| check-regex.js:61:15:61:42 | baseURL ... tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | | check-validator.js:15:15:15:45 | "test.c ... tainted | | check-validator.js:15:15:15:45 | "test.c ... tainted | | check-validator.js:15:29:15:45 | req.query.tainted | @@ -127,6 +131,10 @@ edges | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | +| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | @@ -166,6 +174,7 @@ edges | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. | | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. | | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. | +| check-regex.js:61:15:61:42 | baseURL ... tainted | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. | | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. | | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. | | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. | diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js index a05c1ae7ddfa..238aa9068432 100644 --- a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js +++ b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js @@ -25,7 +25,7 @@ app.get('/check-with-axios', req => { } else { axios.get(baseURL + req.params.tainted); // OK } - + // Blacklists are not safe if (!req.query.tainted.match(/^[/\.%]+$/)) { axios.get("test.com/" + req.query.tainted); // SSRF @@ -39,8 +39,29 @@ app.get('/check-with-axios', req => { } axios.get("test.com/" + req.query.tainted); // OK - False Positive + + if (req.query.tainted.matchAll(/^[0-9a-z]+$/g)) { // letters and numbers + axios.get("test.com/" + req.query.tainted); // OK + } + if (req.query.tainted.matchAll(/^[0-9a-z\-_]+$/g)) { // letters, numbers, - and _ + axios.get("test.com/" + req.query.tainted); // OK + } }); const isValidPath = path => path.match(/^[0-9a-z]+$/); const isInBlackList = path => path.match(/^[/\.%]+$/); + +app.get('/check-with-axios', req => { + const baseURL = "test.com/" + if (isValidPathMatchAll(req.params.tainted) ) { + axios.get(baseURL + req.params.tainted); // OK + } + if (!isValidPathMatchAll(req.params.tainted) ) { + axios.get(baseURL + req.params.tainted); // NOT OK - SSRF + } else { + axios.get(baseURL + req.params.tainted); // OK + } +}); + +const isValidPathMatchAll = path => path.matchAll(/^[0-9a-z]+$/g); diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected index d0fb91322fe1..8341636994e4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected @@ -25,3 +25,4 @@ | tst-IncompleteHostnameRegExp.js:53:14:53:35 | test.example.com$ | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:53:13:53:36 | 'test.' ... e.com$' | here | | tst-IncompleteHostnameRegExp.js:55:14:55:38 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:55:13:55:39 | '^http: ... le.com' | here | | tst-IncompleteHostnameRegExp.js:59:5:59:20 | foo.example\\.com | This regular expression has an unescaped '.' before 'example\\.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:59:2:59:32 | /^(foo. ... ever)$/ | here | +| tst-IncompleteHostnameRegExp.js:61:18:61:41 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:61:17:61:42 | "^http: ... le.com" | here | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js index ddc267ebdd71..51c2c9914209 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js @@ -57,4 +57,6 @@ /^http:\/\/(..|...)\.example\.com\/index\.html/; // OK, wildcards are intentional /^http:\/\/.\.example\.com\/index\.html/; // OK, the wildcard is intentional /^(foo.example\.com|whatever)$/; // kinda OK - one disjunction doesn't even look like a hostname + + if (s.matchAll("^http://test.example.com")) {} // NOT OK }); diff --git a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected index a7933f2926ec..ebff0b583c02 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected +++ b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected @@ -59,3 +59,12 @@ | tst-UnanchoredUrlRegExp.js:26:3:26:22 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. | | tst-UnanchoredUrlRegExp.js:35:2:35:32 | /https? ... 0-9]+)/ | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | | tst-UnanchoredUrlRegExp.js:77:11:77:32 | /vimeo\\ ... 0-9]+)/ | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | +| tst-UnanchoredUrlRegExp.js:111:50:111:68 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | +| tst-UnanchoredUrlRegExp.js:112:61:112:79 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | +| tst-UnanchoredUrlRegExp.js:113:50:113:69 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. | +| tst-UnanchoredUrlRegExp.js:114:50:114:72 | /^https ... d.com/g | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. | +| tst-UnanchoredUrlRegExp.js:115:50:115:94 | "(^http ... 2.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. | +| tst-UnanchoredUrlRegExp.js:116:50:116:93 | "(https ... e.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. | +| tst-UnanchoredUrlRegExp.js:117:50:117:59 | "good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | +| tst-UnanchoredUrlRegExp.js:118:50:118:68 | "https?://good.com" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | +| tst-UnanchoredUrlRegExp.js:119:50:119:73 | "https? ... m:8080" | When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it. | diff --git a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js index 24815ebe59e7..c0c5ecb3e3e0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js +++ b/javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js @@ -105,4 +105,27 @@ /\.com|\.org/; // OK, has no domain name /example\.com|whatever/; // OK, the other disjunction doesn't match a hostname + + // MatchAll test cases: + // Vulnerable patterns + if ("http://evil.com/?http://good.com".matchAll("https?://good.com")) {} // NOT OK + if ("http://evil.com/?http://good.com".matchAll(new RegExp("https?://good.com"))) {} // NOT OK + if ("http://evil.com/?http://good.com".matchAll("^https?://good.com")) {} // NOT OK - missing post-anchor + if ("http://evil.com/?http://good.com".matchAll(/^https?:\/\/good.com/g)) {} // NOT OK - missing post-anchor + if ("http://evil.com/?http://good.com".matchAll("(^https?://good1.com)|(^https?://good2.com)")) {} // NOT OK - missing post-anchor + if ("http://evil.com/?http://good.com".matchAll("(https?://good.com)|(^https?://goodie.com)")) {} // NOT OK - missing post-anchor + if ("http://evil.com/?http://good.com".matchAll("good.com")) {} // NOT OK - missing protocol + if ("http://evil.com/?http://good.com".matchAll("https?://good.com")) {} // NOT OK + if ("http://evil.com/?http://good.com".matchAll("https?://good.com:8080")) {} // NOT OK + + // Non-vulnerable patterns + if ("something".matchAll("other")) {} // OK + if ("something".matchAll("x.commissary")) {} // OK + if ("http://evil.com/?http://good.com".matchAll("^https?://good.com$")) {} // OK + if ("http://evil.com/?http://good.com".matchAll(new RegExp("^https?://good.com$"))) {} // OK + if ("http://evil.com/?http://good.com".matchAll("^https?://good.com/$")) {} // OK + if ("http://evil.com/?http://good.com".matchAll(/^https?:\/\/good.com\/$/)) {} // OK + if ("http://evil.com/?http://good.com".matchAll("(^https?://good1.com$)|(^https?://good2.com$)")) {} // OK + if ("http://evil.com/?http://good.com".matchAll("(https?://good.com$)|(^https?://goodie.com$)")) {} // OK + }); 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 2a3784617e75..0022ca69c6ba 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 @@ -2237,6 +2237,19 @@ nodes | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | +| normalizedPaths.js:412:7:412:46 | path | +| normalizedPaths.js:412:7:412:46 | path | +| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | +| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | +| normalizedPaths.js:412:35:412:45 | req.query.x | +| normalizedPaths.js:412:35:412:45 | req.query.x | +| normalizedPaths.js:412:35:412:45 | req.query.x | +| normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:426:21:426:24 | path | | other-fs-libraries.js:9:7:9:48 | path | | other-fs-libraries.js:9:7:9:48 | path | | other-fs-libraries.js:9:7:9:48 | path | @@ -7524,6 +7537,20 @@ edges | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | | normalizedPaths.js:408:38:408:59 | req.que ... it('/') | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:415:19:415:22 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:412:7:412:46 | path | normalizedPaths.js:426:21:426:24 | path | +| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | normalizedPaths.js:412:7:412:46 | path | +| normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | normalizedPaths.js:412:7:412:46 | path | +| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | +| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | +| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | +| normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:412:14:412:46 | pathMod ... uery.x) | | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path | | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path | | other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path | @@ -10539,6 +10566,8 @@ edges | normalizedPaths.js:399:21:399:24 | path | normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:399:21:399:24 | path | This path depends on a $@. | normalizedPaths.js:385:35:385:45 | req.query.x | user-provided value | | normalizedPaths.js:407:19:407:67 | pathMod ... t('/')) | normalizedPaths.js:407:45:407:55 | req.query.x | normalizedPaths.js:407:19:407:67 | pathMod ... t('/')) | This path depends on a $@. | normalizedPaths.js:407:45:407:55 | req.query.x | user-provided value | | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | normalizedPaths.js:408:38:408:48 | req.query.x | normalizedPaths.js:408:19:408:60 | pathMod ... t('/')) | This path depends on a $@. | normalizedPaths.js:408:38:408:48 | req.query.x | user-provided value | +| normalizedPaths.js:415:19:415:22 | path | normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:415:19:415:22 | path | This path depends on a $@. | normalizedPaths.js:412:35:412:45 | req.query.x | user-provided value | +| normalizedPaths.js:426:21:426:24 | path | normalizedPaths.js:412:35:412:45 | req.query.x | normalizedPaths.js:426:21:426:24 | path | This path depends on a $@. | normalizedPaths.js:412:35:412:45 | req.query.x | user-provided value | | other-fs-libraries.js:11:19:11:22 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:11:19:11:22 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value | | other-fs-libraries.js:12:27:12:30 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:12:27:12:30 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value | | other-fs-libraries.js:13:24:13:27 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:13:24:13:27 | path | This path depends on a $@. | other-fs-libraries.js:9:24:9:30 | req.url | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js index a5453a728d3f..4fa6b3f50d54 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js @@ -407,3 +407,25 @@ app.get('/join-spread', (req, res) => { fs.readFileSync(pathModule.join('foo', ...req.query.x.split('/'))); // NOT OK fs.readFileSync(pathModule.join(...req.query.x.split('/'))); // NOT OK }); + +app.get('/dotdot-matchAll-regexp', (req, res) => { + let path = pathModule.normalize(req.query.x); + if (pathModule.isAbsolute(path)) + return; + fs.readFileSync(path); // NOT OK + if (!path.matchAll(/\./)) { + fs.readFileSync(path); // OK + } + if (!path.matchAll(/\.\./)) { + fs.readFileSync(path); // OK + } + if (!path.matchAll(/\.\.\//)) { + fs.readFileSync(path); // OK + } + if (!path.matchAll(/\.\.\/foo/)) { + fs.readFileSync(path); // NOT OK + } + if (!path.matchAll(/(\.\.\/|\.\.\\)/)) { + fs.readFileSync(path); // OK + } +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected index db473a17d2c3..1f810f1beea8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected @@ -94,6 +94,25 @@ nodes | logInjectionBad.js:99:26:99:33 | username | | logInjectionBad.js:113:37:113:44 | username | | logInjectionBad.js:113:37:113:44 | username | +| logInjectionBad.js:122:9:122:58 | username | +| logInjectionBad.js:122:20:122:43 | url.par ... , true) | +| logInjectionBad.js:122:20:122:49 | url.par ... ).query | +| logInjectionBad.js:122:20:122:58 | url.par ... sername | +| logInjectionBad.js:122:30:122:36 | req.url | +| logInjectionBad.js:122:30:122:36 | req.url | +| logInjectionBad.js:123:9:123:46 | otherStr | +| logInjectionBad.js:123:20:123:27 | username | +| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | +| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | +| logInjectionBad.js:124:17:124:24 | otherStr | +| logInjectionBad.js:124:17:124:24 | otherStr | +| logInjectionBad.js:128:20:128:43 | url.par ... , true) | +| logInjectionBad.js:128:20:128:49 | url.par ... ).query | +| logInjectionBad.js:128:20:128:58 | url.par ... sername | +| logInjectionBad.js:128:30:128:36 | req.url | +| logInjectionBad.js:128:30:128:36 | req.url | +| logInjectionBad.js:129:42:129:50 | RegExp.$1 | +| logInjectionBad.js:129:42:129:50 | RegExp.$1 | edges | logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q | | logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q | @@ -186,6 +205,23 @@ edges | logInjectionBad.js:73:20:73:20 | q | logInjectionBad.js:73:20:73:26 | q.query | | logInjectionBad.js:73:20:73:26 | q.query | logInjectionBad.js:73:20:73:35 | q.query.username | | logInjectionBad.js:73:20:73:35 | q.query.username | logInjectionBad.js:73:9:73:35 | username | +| logInjectionBad.js:122:9:122:58 | username | logInjectionBad.js:123:20:123:27 | username | +| logInjectionBad.js:122:20:122:43 | url.par ... , true) | logInjectionBad.js:122:20:122:49 | url.par ... ).query | +| logInjectionBad.js:122:20:122:49 | url.par ... ).query | logInjectionBad.js:122:20:122:58 | url.par ... sername | +| logInjectionBad.js:122:20:122:58 | url.par ... sername | logInjectionBad.js:122:9:122:58 | username | +| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) | +| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) | +| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr | +| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr | +| logInjectionBad.js:123:20:123:27 | username | logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | +| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | +| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | logInjectionBad.js:123:9:123:46 | otherStr | +| logInjectionBad.js:128:20:128:43 | url.par ... , true) | logInjectionBad.js:128:20:128:49 | url.par ... ).query | +| logInjectionBad.js:128:20:128:49 | url.par ... ).query | logInjectionBad.js:128:20:128:58 | url.par ... sername | +| logInjectionBad.js:128:20:128:58 | url.par ... sername | logInjectionBad.js:129:42:129:50 | RegExp.$1 | +| logInjectionBad.js:128:20:128:58 | url.par ... sername | logInjectionBad.js:129:42:129:50 | RegExp.$1 | +| logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:128:20:128:43 | url.par ... , true) | +| logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:128:20:128:43 | url.par ... , true) | #select | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value | | logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | Log entry depends on a $@. | logInjectionBad.js:19:23:19:29 | req.url | user-provided value | @@ -208,3 +244,5 @@ edges | logInjectionBad.js:91:26:91:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:91:26:91:33 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value | | logInjectionBad.js:99:26:99:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:99:26:99:33 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value | | logInjectionBad.js:113:37:113:44 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:113:37:113:44 | username | Log entry depends on a $@. | logInjectionBad.js:72:23:72:29 | req.url | user-provided value | +| logInjectionBad.js:124:17:124:24 | otherStr | logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:124:17:124:24 | otherStr | Log entry depends on a $@. | logInjectionBad.js:122:30:122:36 | req.url | user-provided value | +| logInjectionBad.js:129:42:129:50 | RegExp.$1 | logInjectionBad.js:128:30:128:36 | req.url | logInjectionBad.js:129:42:129:50 | RegExp.$1 | Log entry depends on a $@. | logInjectionBad.js:128:30:128:36 | req.url | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js index c6c86363164e..028412c346b0 100644 --- a/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js +++ b/javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js @@ -116,4 +116,15 @@ const server4 = http.createServer((req, res) => { }); server.start(); } -}); \ No newline at end of file +}); + +const serverMatchAll = http.createServer((req, res) => { + let username = url.parse(req.url, true).query.username; + let otherStr = username.matchAll(/.*/g)[0]; // BAD + console.log(otherStr); +}); + +const serverMatchAl2l = http.createServer((req, res) => { + const result = url.parse(req.url, true).query.username.matchAll(/(\d+)/g); // BAD + console.log("First captured group:", RegExp.$1); +});