Skip to content

Commit 76a8e98

Browse files
asgerfasgerf
authored
Merge pull request #13283 from asgerf/js/restrict-regex-search-function
JS: Be more conservative about flagging "search" call arguments as regex
2 parents d59263a + 77d2799 commit 76a8e98

7 files changed

Lines changed: 58 additions & 36 deletions

File tree

‎javascript/ql/lib/semmle/javascript/Regexp.qll‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -958,6 +958,27 @@ private predicate isUsedAsNonMatchObject(DataFlow::MethodCallNode call) {
958958
)
959959
}
960960

961+
/**
962+
* Holds if `value` is used in a way that suggests it returns a number.
963+
*/
964+
pragma[inline]
965+
private predicate isUsedAsNumber(DataFlow::LocalSourceNode value) {
966+
any(Comparison compare)
967+
.hasOperands(value.getALocalUse().asExpr(), any(Expr e | e.analyze().getAType() = TTNumber()))
968+
or
969+
value.flowsToExpr(any(ArithmeticExpr e).getAnOperand())
970+
or
971+
value.flowsToExpr(any(UnaryExpr e | e.getOperator() = "-").getOperand())
972+
or
973+
value.flowsToExpr(any(IndexExpr expr).getPropertyNameExpr())
974+
or
975+
exists(DataFlow::CallNode call |
976+
call.getCalleeName() =
977+
["substring", "substr", "slice", "splice", "charAt", "charCodeAt", "codePointAt"] and
978+
value.flowsTo(call.getAnArgument())
979+
)
980+
}
981+
961982
/**
962983
* Holds if `source` may be interpreted as a regular expression.
963984
*/
@@ -985,9 +1006,9 @@ predicate isInterpretedAsRegExp(DataFlow::Node source) {
9851006
methodName = "search" and
9861007
source = mce.getArgument(0) and
9871008
mce.getNumArgument() = 1 and
988-
// "search" is a common method name, and so we exclude chained accesses
989-
// because `String.prototype.search` returns a number
990-
not exists(PropAccess p | p.getBase() = mce.getEnclosingExpr())
1009+
// "search" is a common method name, and the built-in "search" method is rarely used,
1010+
// so to reduce FPs we also require that the return value appears to be used as a number.
1011+
isUsedAsNumber(mce)
9911012
)
9921013
or
9931014
exists(DataFlow::SourceNode schema | schema = JsonSchema::getAPartOfJsonSchema() |

‎javascript/ql/src/change-notes/2023-06-01-restrict-regex-search-function.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Fixed an issue where calls to a method named `search` would lead to false positive alerts related to regular expressions.
5+
This happened when the call was incorrectly seen as a call to `String.prototype.search`, since this function converts its first argument
6+
to a regular expression. The analysis is now more restrictive about when to treat `search` calls as regular expression sinks.

‎javascript/ql/test/query-tests/RegExp/RegExpAlwaysMatches/tst.js‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,23 +55,23 @@ function emptyAlt3(x) {
5555
}
5656

5757
function search(x) {
58-
return x.search(/[a-z]*/); // NOT OK
58+
return x.search(/[a-z]*/) > -1; // NOT OK
5959
}
6060

6161
function search2(x) {
62-
return x.search(/[a-z]/); // OK
62+
return x.search(/[a-z]/) > -1; // OK
6363
}
6464

6565
function lookahead(x) {
66-
return x.search(/(?!x)/); // OK
66+
return x.search(/(?!x)/) > -1; // OK
6767
}
6868

6969
function searchPrefix(x) {
70-
return x.search(/^(foo)?/); // NOT OK - `foo?` does not affect the returned index
70+
return x.search(/^(foo)?/) > -1; // NOT OK - `foo?` does not affect the returned index
7171
}
7272

7373
function searchSuffix(x) {
74-
return x.search(/(foo)?$/); // OK - `foo?` affects the returned index
74+
return x.search(/(foo)?$/) > -1; // OK - `foo?` affects the returned index
7575
}
7676

7777
function wordBoundary(x) {

‎javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/MissingRegExpAnchor.expected‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
| 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. |
5050
| 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. |
5151
| 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. |
52-
| 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. |
52+
| tst-UnanchoredUrlRegExp.js:13:48:13:66 | "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. |
5353
| 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. |
5454
| 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. |
5555
| 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. |

‎javascript/ql/test/query-tests/Security/CWE-020/MissingRegExpAnchor/tst-UnanchoredUrlRegExp.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
/https?:\/\/good.com/.exec("http://evil.com/?http://good.com"); // NOT OK
1111
new RegExp("https?://good.com").exec("http://evil.com/?http://good.com"); // NOT OK
1212

13-
"http://evil.com/?http://good.com".search("https?://good.com"); // NOT OK
13+
if ("http://evil.com/?http://good.com".search("https?://good.com") > -1) {} // NOT OK
1414

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

‎javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,12 @@ nodes
3131
| RegExpInjection.js:41:26:41:30 | input |
3232
| RegExpInjection.js:42:25:42:29 | input |
3333
| RegExpInjection.js:42:25:42:29 | input |
34-
| RegExpInjection.js:45:20:45:24 | input |
35-
| RegExpInjection.js:45:20:45:24 | input |
36-
| RegExpInjection.js:46:23:46:27 | input |
37-
| RegExpInjection.js:46:23:46:27 | input |
38-
| RegExpInjection.js:47:22:47:26 | input |
39-
| RegExpInjection.js:47:22:47:26 | input |
40-
| RegExpInjection.js:50:46:50:50 | input |
41-
| RegExpInjection.js:50:46:50:50 | input |
34+
| RegExpInjection.js:45:24:45:28 | input |
35+
| RegExpInjection.js:45:24:45:28 | input |
36+
| RegExpInjection.js:46:27:46:31 | input |
37+
| RegExpInjection.js:46:27:46:31 | input |
38+
| RegExpInjection.js:47:26:47:30 | input |
39+
| RegExpInjection.js:47:26:47:30 | input |
4240
| RegExpInjection.js:54:14:54:16 | key |
4341
| RegExpInjection.js:54:14:54:27 | key.split(".") |
4442
| RegExpInjection.js:54:14:54:42 | key.spl ... x => x) |
@@ -89,14 +87,12 @@ edges
8987
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:41:26:41:30 | input |
9088
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:25:42:29 | input |
9189
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:42:25:42:29 | input |
92-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:20:45:24 | input |
93-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:20:45:24 | input |
94-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:46:23:46:27 | input |
95-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:46:23:46:27 | input |
96-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:47:22:47:26 | input |
97-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:47:22:47:26 | input |
98-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:50:46:50:50 | input |
99-
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:50:46:50:50 | input |
90+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:24:45:28 | input |
91+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:45:24:45:28 | input |
92+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:46:27:46:31 | input |
93+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:46:27:46:31 | input |
94+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:47:26:47:30 | input |
95+
| RegExpInjection.js:5:31:5:56 | input | RegExpInjection.js:47:26:47:30 | input |
10096
| RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:5:31:5:56 | input |
10197
| RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:5:31:5:56 | input |
10298
| RegExpInjection.js:8:31:8:33 | key | RegExpInjection.js:8:23:8:45 | "\\\\b" + ... (.*)\\n" |
@@ -157,10 +153,9 @@ edges
157153
| 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 |
158154
| 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 |
159155
| 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 |
160-
| 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 |
161-
| 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 |
162-
| 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 |
163-
| RegExpInjection.js:50:46:50:50 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:50:46:50:50 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
156+
| RegExpInjection.js:45:24:45:28 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:45:24:45:28 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
157+
| RegExpInjection.js:46:27:46:31 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:46:27:46:31 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
158+
| RegExpInjection.js:47:26:47:30 | input | RegExpInjection.js:5:39:5:56 | req.param("input") | RegExpInjection.js:47:26:47:30 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
164159
| RegExpInjection.js:54:14:54:52 | key.spl ... in("-") | RegExpInjection.js:5:13:5:28 | req.param("key") | RegExpInjection.js:54:14:54:52 | key.spl ... in("-") | This regular expression is constructed from a $@. | RegExpInjection.js:5:13:5:28 | req.param("key") | user-provided value |
165160
| RegExpInjection.js:64:14:64:18 | input | RegExpInjection.js:60:39:60:56 | req.param("input") | RegExpInjection.js:64:14:64:18 | input | This regular expression is constructed from a $@. | RegExpInjection.js:60:39:60:56 | req.param("input") | user-provided value |
166161
| RegExpInjection.js:87:14:87:55 | "^.*\\.( ... + ")$" | RegExpInjection.js:82:15:82:32 | req.param("input") | RegExpInjection.js:87:14:87:55 | "^.*\\.( ... + ")$" | This regular expression is constructed from a $@. | RegExpInjection.js:82:15:82:32 | req.param("input") | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,12 @@ app.get('/findKey', function(req, res) {
4242
if (maybeString.match(input)) {} // NOT OK
4343
if (notString.match(input)) {} // OK
4444

45-
defString.search(input); // NOT OK
46-
likelyString.search(input); // NOT OK
47-
maybeString.search(input); // NOT OK
48-
notString.search(input); // OK
45+
if (defString.search(input) > -1) {} // NOT OK
46+
if (likelyString.search(input) > -1) {} // NOT OK
47+
if (maybeString.search(input) > -1) {} // NOT OK
48+
if (notString.search(input) > -1) {} // OK
4949

50-
URI(`${protocol}://${host}${path}`).search(input); // OK, but still flagged [INCONSISTENCY]
50+
URI(`${protocol}://${host}${path}`).search(input); // OK
5151
URI(`${protocol}://${host}${path}`).search(input).href(); // OK
5252
unknown.search(input).unknown; // OK
5353

@@ -62,7 +62,7 @@ app.get('/findKey', function(req, res) {
6262
Search.search(input); // OK!
6363

6464
new RegExp(input); // NOT OK
65-
65+
6666
var sanitized = input.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, "\\$&");
6767
new RegExp(sanitized); // OK
6868
});

0 commit comments

Comments
 (0)