Skip to content

Commit 0d8bef7

Browse files
erik-krogherik-krogh
authored
Merge pull request github#6736 from erik-krogh/polyReplace
JS: track flow through string replace calls that just replace single chars for js/polynomial-redos
2 parents 8425eaf + a082ed9 commit 0d8bef7

File tree

4 files changed

+46
-3
lines changed

4 files changed

+46
-3
lines changed

‎javascript/ql/lib/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,28 @@ module PolynomialReDoS {
8686
this.(StringReplaceCall).isGlobal() and
8787
// not lone char classes - they don't remove any repeated pattern.
8888
not exists(RegExpTerm root | root = this.(StringReplaceCall).getRegExp().getRoot() |
89-
root instanceof RegExpCharacterClass
90-
or
91-
root instanceof RegExpCharacterClassEscape
89+
isCharClassLike(root)
9290
)
9391
or
9492
this.(DataFlow::MethodCallNode).getMethodName() = StringOps::substringMethodName()
9593
}
9694
}
9795

96+
/**
97+
* Holds if `term` matches a set of strings of length 1.
98+
*/
99+
predicate isCharClassLike(RegExpTerm term) {
100+
term instanceof RegExpCharacterClass
101+
or
102+
term instanceof RegExpCharacterClassEscape
103+
or
104+
term.(RegExpConstant).getValue().length() = 1
105+
or
106+
exists(RegExpAlt alt | term = alt |
107+
forall(RegExpTerm choice | choice = alt.getAlternative() | isCharClassLike(choice))
108+
)
109+
}
110+
98111
/**
99112
* An check on the length of a string, seen as a sanitizer guard.
100113
*/

‎javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,8 @@
119119
| polynomial-redos.js:116:21:116:28 | [\\d\\D]*? | Strings starting with '/*' and with many repetitions of 'a/*' can start matching anywhere after the start of the preceeding \\/\\*[\\d\\D]*?\\*\\/ |
120120
| polynomial-redos.js:118:17:118:23 | (#\\d+)+ | Strings with many repetitions of '9' can start matching anywhere after the start of the preceeding \\d+ |
121121
| polynomial-redos.js:124:33:124:35 | \\s+ | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding \\s+$ |
122+
| polynomial-redos.js:130:21:130:22 | c+ | Strings starting with 'c' and with many repetitions of 'c' can start matching anywhere after the start of the preceeding cc+D |
123+
| polynomial-redos.js:133:22:133:23 | f+ | Strings starting with 'f' and with many repetitions of 'f' can start matching anywhere after the start of the preceeding ff+G |
122124
| regexplib/address.js:27:3:27:5 | \\s* | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding (\\s*\\(?0\\d{4}\\)?(\\s*\|-)\\d{3}(\\s*\|-)\\d{3}\\s*) |
123125
| regexplib/address.js:27:48:27:50 | \\s* | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding (\\s*\\(?0\\d{3}\\)?(\\s*\|-)\\d{3}(\\s*\|-)\\d{4}\\s*) |
124126
| regexplib/address.js:27:93:27:95 | \\s* | Strings with many repetitions of ' ' can start matching anywhere after the start of the preceeding (\\s*(7\|8)(\\d{7}\|\\d{3}(\\-\|\\s{1})\\d{4})\\s*) |

‎javascript/ql/test/query-tests/Performance/ReDoS/PolynomialReDoS.expected

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,16 @@ nodes
197197
| polynomial-redos.js:123:13:123:20 | replaced |
198198
| polynomial-redos.js:124:12:124:17 | result |
199199
| polynomial-redos.js:124:12:124:17 | result |
200+
| polynomial-redos.js:129:6:129:42 | modified |
201+
| polynomial-redos.js:129:17:129:23 | tainted |
202+
| polynomial-redos.js:129:17:129:42 | tainted ... g, "b") |
203+
| polynomial-redos.js:130:2:130:9 | modified |
204+
| polynomial-redos.js:130:2:130:9 | modified |
205+
| polynomial-redos.js:132:6:132:50 | modified2 |
206+
| polynomial-redos.js:132:18:132:24 | tainted |
207+
| polynomial-redos.js:132:18:132:50 | tainted ... g, "e") |
208+
| polynomial-redos.js:133:2:133:10 | modified2 |
209+
| polynomial-redos.js:133:2:133:10 | modified2 |
200210
edges
201211
| lib/closure.js:3:21:3:21 | x | lib/closure.js:4:16:4:16 | x |
202212
| lib/closure.js:3:21:3:21 | x | lib/closure.js:4:16:4:16 | x |
@@ -377,6 +387,8 @@ edges
377387
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:118:2:118:8 | tainted |
378388
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:118:2:118:8 | tainted |
379389
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:121:18:121:24 | tainted |
390+
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:129:17:129:23 | tainted |
391+
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:132:18:132:24 | tainted |
380392
| polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:5:6:5:32 | tainted |
381393
| polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:5:6:5:32 | tainted |
382394
| polynomial-redos.js:68:18:68:24 | req.url | polynomial-redos.js:68:18:68:24 | req.url |
@@ -387,6 +399,14 @@ edges
387399
| polynomial-redos.js:123:3:123:20 | result | polynomial-redos.js:124:12:124:17 | result |
388400
| polynomial-redos.js:123:3:123:20 | result | polynomial-redos.js:124:12:124:17 | result |
389401
| polynomial-redos.js:123:13:123:20 | replaced | polynomial-redos.js:123:3:123:20 | result |
402+
| polynomial-redos.js:129:6:129:42 | modified | polynomial-redos.js:130:2:130:9 | modified |
403+
| polynomial-redos.js:129:6:129:42 | modified | polynomial-redos.js:130:2:130:9 | modified |
404+
| polynomial-redos.js:129:17:129:23 | tainted | polynomial-redos.js:129:17:129:42 | tainted ... g, "b") |
405+
| polynomial-redos.js:129:17:129:42 | tainted ... g, "b") | polynomial-redos.js:129:6:129:42 | modified |
406+
| polynomial-redos.js:132:6:132:50 | modified2 | polynomial-redos.js:133:2:133:10 | modified2 |
407+
| polynomial-redos.js:132:6:132:50 | modified2 | polynomial-redos.js:133:2:133:10 | modified2 |
408+
| polynomial-redos.js:132:18:132:24 | tainted | polynomial-redos.js:132:18:132:50 | tainted ... g, "e") |
409+
| polynomial-redos.js:132:18:132:50 | tainted ... g, "e") | polynomial-redos.js:132:6:132:50 | modified2 |
390410
#select
391411
| lib/closure.js:4:5:4:17 | /u*o/.test(x) | lib/closure.js:3:21:3:21 | x | lib/closure.js:4:16:4:16 | x | This $@ that depends on $@ may run slow on strings with many repetitions of 'u'. | lib/closure.js:4:6:4:7 | u* | regular expression | lib/closure.js:3:21:3:21 | x | library input |
392412
| lib/indirect.js:2:5:2:17 | /k*h/.test(x) | lib/indirect.js:1:32:1:32 | x | lib/indirect.js:2:16:2:16 | x | This $@ that depends on $@ may run slow on strings with many repetitions of 'k'. | lib/indirect.js:2:6:2:7 | k* | regular expression | lib/indirect.js:1:32:1:32 | x | library input |
@@ -474,3 +494,5 @@ edges
474494
| polynomial-redos.js:116:2:116:35 | tainted ... \\*\\//g) | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:116:2:116:8 | tainted | This $@ that depends on $@ may run slow on strings starting with '/*' and with many repetitions of 'a/*'. | polynomial-redos.js:116:21:116:28 | [\\d\\D]*? | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
475495
| polynomial-redos.js:118:2:118:25 | tainted ... \\d+)+/) | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:118:2:118:8 | tainted | This $@ that depends on $@ may run slow on strings with many repetitions of '9'. | polynomial-redos.js:118:17:118:23 | (#\\d+)+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
476496
| polynomial-redos.js:124:12:124:43 | result. ... /g, '') | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:124:12:124:17 | result | This $@ that depends on $@ may run slow on strings with many repetitions of ' '. | polynomial-redos.js:124:33:124:35 | \\s+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
497+
| polynomial-redos.js:130:2:130:31 | modifie ... g, "b") | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:130:2:130:9 | modified | This $@ that depends on $@ may run slow on strings starting with 'c' and with many repetitions of 'c'. | polynomial-redos.js:130:21:130:22 | c+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
498+
| polynomial-redos.js:133:2:133:32 | modifie ... g, "b") | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:133:2:133:10 | modified2 | This $@ that depends on $@ may run slow on strings starting with 'f' and with many repetitions of 'f'. | polynomial-redos.js:133:22:133:23 | f+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |

‎javascript/ql/test/query-tests/Performance/ReDoS/polynomial-redos.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,4 +125,10 @@ app.use(function(req, res) {
125125
})();
126126

127127
tainted.match(/(https?:\/\/[^\s]+)/gm); // OK
128+
129+
var modified = tainted.replace(/a/g, "b");
130+
modified.replace(/cc+D/g, "b"); // NOT OK
131+
132+
var modified2 = tainted.replace(/a|b|c|\d/g, "e");
133+
modified2.replace(/ff+G/g, "b"); // NOT OK
128134
});

0 commit comments

Comments
 (0)