Skip to content

Commit dbd57e3

Browse files
committed
Fixed issue where TaintTracking was not catching matchAll vulnerability
1 parent a4fe728 commit dbd57e3

File tree

3 files changed

+26
-2
lines changed

3 files changed

+26
-2
lines changed

javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -716,7 +716,7 @@ module TaintTracking {
716716

717717
pragma[nomagic]
718718
private DataFlow::MethodCallNode matchMethodCall() {
719-
result.getMethodName() = "match" and
719+
result.getMethodName() = ["match", "matchAll"] and
720720
exists(DataFlow::AnalyzedNode analyzed |
721721
pragma[only_bind_into](analyzed) = result.getArgument(0).analyze() and
722722
analyzed.getAType() = TTRegExp()

javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,18 @@ nodes
9494
| logInjectionBad.js:99:26:99:33 | username |
9595
| logInjectionBad.js:113:37:113:44 | username |
9696
| logInjectionBad.js:113:37:113:44 | username |
97+
| logInjectionBad.js:122:9:122:58 | username |
98+
| logInjectionBad.js:122:20:122:43 | url.par ... , true) |
99+
| logInjectionBad.js:122:20:122:49 | url.par ... ).query |
100+
| logInjectionBad.js:122:20:122:58 | url.par ... sername |
101+
| logInjectionBad.js:122:30:122:36 | req.url |
102+
| logInjectionBad.js:122:30:122:36 | req.url |
103+
| logInjectionBad.js:123:9:123:46 | otherStr |
104+
| logInjectionBad.js:123:20:123:27 | username |
105+
| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) |
106+
| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
107+
| logInjectionBad.js:124:17:124:24 | otherStr |
108+
| logInjectionBad.js:124:17:124:24 | otherStr |
97109
edges
98110
| logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
99111
| logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
@@ -186,6 +198,17 @@ edges
186198
| logInjectionBad.js:73:20:73:20 | q | logInjectionBad.js:73:20:73:26 | q.query |
187199
| logInjectionBad.js:73:20:73:26 | q.query | logInjectionBad.js:73:20:73:35 | q.query.username |
188200
| logInjectionBad.js:73:20:73:35 | q.query.username | logInjectionBad.js:73:9:73:35 | username |
201+
| logInjectionBad.js:122:9:122:58 | username | logInjectionBad.js:123:20:123:27 | username |
202+
| logInjectionBad.js:122:20:122:43 | url.par ... , true) | logInjectionBad.js:122:20:122:49 | url.par ... ).query |
203+
| logInjectionBad.js:122:20:122:49 | url.par ... ).query | logInjectionBad.js:122:20:122:58 | url.par ... sername |
204+
| logInjectionBad.js:122:20:122:58 | url.par ... sername | logInjectionBad.js:122:9:122:58 | username |
205+
| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) |
206+
| logInjectionBad.js:122:30:122:36 | req.url | logInjectionBad.js:122:20:122:43 | url.par ... , true) |
207+
| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr |
208+
| logInjectionBad.js:123:9:123:46 | otherStr | logInjectionBad.js:124:17:124:24 | otherStr |
209+
| logInjectionBad.js:123:20:123:27 | username | logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) |
210+
| logInjectionBad.js:123:20:123:43 | usernam ... (/.*/g) | logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] |
211+
| logInjectionBad.js:123:20:123:46 | usernam ... */g)[0] | logInjectionBad.js:123:9:123:46 | otherStr |
189212
#select
190213
| 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 |
191214
| 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 +231,4 @@ edges
208231
| 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 |
209232
| 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 |
210233
| 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 |
234+
| 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 |

javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,6 @@ const server4 = http.createServer((req, res) => {
120120

121121
const serverMatchAll = http.createServer((req, res) => {
122122
let username = url.parse(req.url, true).query.username;
123-
let otherStr = username.matchAll(/.*/g)[0]; // BAD - this is suppose to be cought by Taint Tracking, works for match but not matchAll
123+
let otherStr = username.matchAll(/.*/g)[0]; // BAD
124124
console.log(otherStr);
125125
});

0 commit comments

Comments
 (0)