Skip to content

JS: sharpen recognition of match calls #6117

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 23, 2021
Merged

JS: sharpen recognition of match calls #6117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5838e54
JS: Sharpen recognition of string 'match' calls
asgerf Jun 15, 2021
0754ed2
JS: Change note
asgerf Jun 21, 2021
16e3681
JS: Update RegExpInjection test case
asgerf Jun 22, 2021
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
3 changes: 3 additions & 0 deletions javascript/change-notes/2021-06-21-sharpen-match-calls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
lgtm,codescanning
* The regular expression queries now recognize calls to the String `match` method more precisely,
resulting in fewer false-positive results when a string is passed to a method named `match`.
35 changes: 34 additions & 1 deletion javascript/ql/src/semmle/javascript/Regexp.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,36 @@ private predicate isNativeStringMethod(Function func, string name) {
)
}

/**
* Holds if `name` is the name of a property on a Match object returned by `String.prototype.match`,
* not including array indices.
*/
private predicate isMatchObjectProperty(string name) {
any(ExternalInstanceMemberDecl decl).hasQualifiedName("Array", name)
or
name in ["length", "index", "input", "groups"]
}

/** 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.getNumArgument() = 1 and
(
// Accessing a property that is absent on Match objects
exists(string propName |
exists(call.getAPropertyRead(propName)) and
not isMatchObjectProperty(propName) and
not exists(propName.toInt())
)
or
// Awaiting the result
call.flowsToExpr(any(AwaitExpr await).getOperand())
or
// Result is obviously unused
call.asExpr() = any(ExprStmt stmt).getExpr()
)
}

/**
* Holds if `source` may be interpreted as a regular expression.
*/
Expand All @@ -939,7 +969,10 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
not isNativeStringMethod(func, methodName)
)
|
methodName = "match" and source = mce.getArgument(0) and mce.getNumArgument() = 1
methodName = "match" and
source = mce.getArgument(0) and
mce.getNumArgument() = 1 and
not isUsedAsNonMatchObject(mce)
or
methodName = "search" and
source = mce.getArgument(0) and
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
| tst-IncompleteHostnameRegExp.js:7:3:7:30 | ^http:\\/\\/(.+).example.com\\/ | This regular expression has an unrestricted wildcard '.+' which may cause 'example.com' to be matched anywhere in the URL, outside the hostname. | tst-IncompleteHostnameRegExp.js:7:2:7:31 | /^http: ... .com\\// | here |
| tst-IncompleteHostnameRegExp.js:10:3:10:36 | ^http:\\/\\/test.example.com\\/(?:.*) | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:10:2:10:37 | /^http: ... (?:.*)/ | here |
| tst-IncompleteHostnameRegExp.js:11:14:11:37 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:11:13:11:38 | "^http: ... le.com" | here |
| tst-IncompleteHostnameRegExp.js:12:11:12:34 | ^http://test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:12:10:12:35 | "^http: ... le.com" | here |
| tst-IncompleteHostnameRegExp.js:12:15:12: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:12:14:12:39 | "^http: ... le.com" | here |
| tst-IncompleteHostnameRegExp.js:15:23:15:46 | ^http://test.example.com | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:15:13:15:50 | id(id(i ... com"))) | here |
| tst-IncompleteHostnameRegExp.js:19:18:19:34 | ^test.example.com | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:20:13:20:26 | `${hostname}$` | here |
| tst-IncompleteHostnameRegExp.js:22:28:22:44 | test.example.com$ | This string, which is used as a regular expression $@, has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:23:13:23:27 | domain.hostname | here |
Expand Down
18 changes: 9 additions & 9 deletions javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
| tst-IncompleteHostnameRegExp.js:5:2:5:29 | /^http: ... le.net/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:6:2:6:43 | /^http: ... b).com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:11:13:11:38 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:12:10:12:35 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:12:14:12:39 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:15:22:15:47 | "^http: ... le.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:19:17:19:35 | '^test.example.com' | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-IncompleteHostnameRegExp.js:40:2:40:30 | /^https ... le.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
Expand Down Expand Up @@ -51,18 +51,18 @@
| tst-SemiAnchoredRegExp.js:96:2:96:55 | /^mouse ... ragend/ | Misleading operator precedence. The subexpression '^mouse' is anchored at the beginning, but the other parts of this regular expression are not |
| tst-SemiAnchoredRegExp.js:97:2:97:14 | /^xxx:\|yyy:/i | Misleading operator precedence. The subexpression '^xxx:' is anchored at the beginning, but the other parts of this regular expression are not |
| tst-SemiAnchoredRegExp.js:98:2:98:18 | /_xxx\|_yyy\|_zzz$/ | Misleading operator precedence. The subexpression '_zzz$' is anchored at the end, but the other parts of this regular expression are not |
| tst-UnanchoredUrlRegExp.js:3:43:3:61 | "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:4:54:4:72 | "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:5:43:5:62 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:6:43:6:64 | /^https ... od.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:7:43:7:87 | "(^http ... 2.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:8:43:8:86 | "(https ... e.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:3:47:3:65 | "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:4:58:4:76 | "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:5:47:5:66 | "^https?://good.com" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:6:47:6:68 | /^https ... od.com/ | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:7:47:7:91 | "(^http ... 2.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:8:47:8:90 | "(https ... e.com)" | This hostname pattern may match any domain name, as it is missing a '$' or '/' at the end. |
| tst-UnanchoredUrlRegExp.js:10:2:10:22 | /https? ... od.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:11:13:11:31 | "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:13:44:13:62 | "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:15:13:15:31 | "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:19:43:19:61 | "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:20:43:20:66 | "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. |
| tst-UnanchoredUrlRegExp.js:19:47:19:65 | "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:20:47:20:70 | "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. |
| tst-UnanchoredUrlRegExp.js:23:3:23:21 | "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:24:3:24:23 | /https? ... od.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:25:14:25:32 | "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. |
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/test/query-tests/Security/CWE-020/tst-IncompleteHostnameRegExp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
/^http:\/\/(?:.+)\\.test\\.example.com\//; // NOT OK
/^http:\/\/test.example.com\/(?:.*)/; // OK
new RegExp("^http://test.example.com"); // NOT OK
s.match("^http://test.example.com"); // NOT OK
if (s.match("^http://test.example.com")) {} // NOT OK

function id(e) { return e; }
new RegExp(id(id(id("^http://test.example.com")))); // NOT OK
Expand Down
20 changes: 10 additions & 10 deletions javascript/ql/test/query-tests/Security/CWE-020/tst-UnanchoredUrlRegExp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
(function(x){

"http://evil.com/?http://good.com".match("https?://good.com"); // NOT OK
"http://evil.com/?http://good.com".match(new RegExp("https?://good.com")); // NOT OK
"http://evil.com/?http://good.com".match("^https?://good.com"); // NOT OK - missing post-anchor
"http://evil.com/?http://good.com".match(/^https?:\/\/good.com/); // NOT OK - missing post-anchor
"http://evil.com/?http://good.com".match("(^https?://good1.com)|(^https?://good2.com)"); // NOT OK - missing post-anchor
"http://evil.com/?http://good.com".match("(https?://good.com)|(^https?://goodie.com)"); // NOT OK - missing post-anchor
if ("http://evil.com/?http://good.com".match("https?://good.com")) {} // NOT OK
if ("http://evil.com/?http://good.com".match(new RegExp("https?://good.com"))) {} // NOT OK
if ("http://evil.com/?http://good.com".match("^https?://good.com")) {} // NOT OK - missing post-anchor
if ("http://evil.com/?http://good.com".match(/^https?:\/\/good.com/)) {} // NOT OK - missing post-anchor
if ("http://evil.com/?http://good.com".match("(^https?://good1.com)|(^https?://good2.com)")) {} // NOT OK - missing post-anchor
if ("http://evil.com/?http://good.com".match("(https?://good.com)|(^https?://goodie.com)")) {} // NOT OK - missing post-anchor

/https?:\/\/good.com/.exec("http://evil.com/?http://good.com"); // NOT OK
new RegExp("https?://good.com").exec("http://evil.com/?http://good.com"); // NOT OK
Expand All @@ -14,10 +14,10 @@

new RegExp("https?://good.com").test("http://evil.com/?http://good.com"); // NOT OK

"something".match("other"); // OK
"something".match("x.commissary"); // OK
"http://evil.com/?http://good.com".match("https?://good.com"); // NOT OK
"http://evil.com/?http://good.com".match("https?://good.com:8080"); // NOT OK
if ("something".match("other")) {} // OK
if ("something".match("x.commissary")) {} // OK
if ("http://evil.com/?http://good.com".match("https?://good.com")) {} // NOT OK
if ("http://evil.com/?http://good.com".match("https?://good.com:8080")) {} // NOT OK

let trustedUrls = [
"https?://good.com", // NOT OK, referenced below
Expand Down
30 changes: 15 additions & 15 deletions javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ nodes
| RegExpInjection.js:31:23:31:23 | s |
| RegExpInjection.js:33:12:33:14 | key |
| RegExpInjection.js:34:12:34:19 | getKey() |
| RegExpInjection.js:40:19:40:23 | input |
| RegExpInjection.js:40:19:40:23 | input |
| RegExpInjection.js:41:22:41:26 | input |
| RegExpInjection.js:41:22:41:26 | input |
| RegExpInjection.js:42:21:42:25 | input |
| RegExpInjection.js:42:21:42:25 | input |
| RegExpInjection.js:40:23:40:27 | input |
| RegExpInjection.js:40:23:40:27 | input |
| RegExpInjection.js:41:26:41:30 | input |
| RegExpInjection.js:41:26:41:30 | input |
| RegExpInjection.js:42:25:42:29 | input |
| RegExpInjection.js:42:25:42:29 | input |
| RegExpInjection.js:45:20:45:24 | input |
| RegExpInjection.js:45:20:45:24 | input |
| RegExpInjection.js:46:23:46:27 | input |
Expand Down Expand Up @@ -73,12 +73,12 @@ edges
| RegExpInjection.js:5:7:5:28 | key | RegExpInjection.js:54:14:54:16 | key |
| RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:5:7:5:28 | key |
| RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:5:7:5:28 | key |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:40:19:40:23 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:40:19:40:23 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:41:22:41:26 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:41:22:41:26 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:21:42:25 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:21:42:25 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:40:23:40:27 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:40:23:40:27 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:41:26:41:30 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:41:26:41:30 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:25:42:29 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:25:42:29 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:20:45:24 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:20:45:24 | input |
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:46:23:46:27 | input |
Expand Down Expand Up @@ -136,9 +136,9 @@ edges
| RegExpInjection.js:27:14:27:21 | getKey() | RegExpInjection.js:24:12:24:27 | req.param("key") | RegExpInjection.js:27:14:27:21 | getKey() | This regular expression is constructed from a $@. | RegExpInjection.js:24:12:24:27 | req.param("key") | user-provided value |
| RegExpInjection.js:31:23:31:23 | s | RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:31:23:31:23 | s | This regular expression is constructed from a $@. | RegExpInjection.js:5:13:5:28 | req.param("key") | user-provided value |
| RegExpInjection.js:31:23:31:23 | s | RegExpInjection.js:24:12:24:27 | req.param("key") | RegExpInjection.js:31:23:31:23 | s | This regular expression is constructed from a $@. | RegExpInjection.js:24:12:24:27 | req.param("key") | user-provided value |
| RegExpInjection.js:40:19:40:23 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:40:19:40:23 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:41:22:41:26 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:41:22:41:26 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:42:21:42:25 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:42:21:42:25 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:40:23:40:27 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:40:23:40:27 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:41:26:41:30 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:41:26:41:30 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:42:25:42:29 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:42:25:42:29 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:45:20:45:24 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:45:20:45:24 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:46:23:46:27 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:46:23:46:27 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
| RegExpInjection.js:47:22:47:26 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:47:22:47:26 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
Expand Down
8 changes: 4 additions & 4 deletions javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ app.get('/findKey', function(req, res) {
var likelyString = x? defString: 42;
var notString = {};

defString.match(input); // NOT OK
likelyString.match(input); // NOT OK
maybeString.match(input); // NOT OK
notString.match(input); // OK
if (defString.match(input)) {} // NOT OK
if (likelyString.match(input)) {} // NOT OK
if (maybeString.match(input)) {} // NOT OK
if (notString.match(input)) {} // OK

defString.search(input); // NOT OK
likelyString.search(input); // NOT OK
Expand Down