Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for matchAll js #17910

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
}
Expand Down
4 changes: 2 additions & 2 deletions javascript/ql/lib/semmle/javascript/Regexp.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/StringOps.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ private module Impl implements
|
name = "replace"
or
name = "match" and exists(mcn.getAPropertyRead())
name = ["match", "matchAll"] and exists(mcn.getAPropertyRead())
)
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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); // SSRF
} else {
axios.get(baseURL + req.params.tainted); // OK
}
});

const isValidPathMatchAll = path => path.matchAll(/^[0-9a-z]+$/g);
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Original file line number Diff line number Diff line change
Expand Up @@ -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

});
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
});
Loading
Loading