Skip to content

Commit e46cde1

Browse files
erik-krogherik-krogh
committed
add a "../" removing taint-step for js/path-injection
1 parent 604731b commit e46cde1

File tree

4 files changed

+190
-1
lines changed

4 files changed

+190
-1
lines changed

‎javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,14 @@ module TaintedPath {
206206
dstlabel.isNormalized()
207207
)
208208
or
209+
// foo.replace(/(\.\.\/)*/, "") and similar
210+
exists(DotDotSlashPrefixRemovingReplace call |
211+
src = call.getInput() and
212+
dst = call.getOutput() and
213+
dstlabel.isAbsolute() and // result can be absolute
214+
dstlabel.toAbsolute() = srclabel.toAbsolute() // preserves normalization status
215+
)
216+
or
209217
// path.join()
210218
exists(DataFlow::CallNode join, int n |
211219
join = NodeJSLib::Path::moduleMember("join").getACall()

‎javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,8 @@ module TaintedPath {
225225
term.getAMatchedString() = "/" or
226226
term.getAMatchedString() = "." or
227227
term.getAMatchedString() = ".."
228-
)
228+
) and
229+
not this instanceof DotDotSlashPrefixRemovingReplace
229230
}
230231

231232
/**
@@ -239,6 +240,57 @@ module TaintedPath {
239240
DataFlow::Node getOutput() { result = output }
240241
}
241242

243+
/**
244+
* A call that removes all instances of "../" in the prefix of the string.
245+
*/
246+
class DotDotSlashPrefixRemovingReplace extends DataFlow::CallNode {
247+
DataFlow::Node input;
248+
DataFlow::Node output;
249+
250+
DotDotSlashPrefixRemovingReplace() {
251+
this.getCalleeName() = "replace" and
252+
input = getReceiver() and
253+
output = this and
254+
exists(RegExpLiteral literal, RegExpTerm term |
255+
getArgument(0).getALocalSource().asExpr() = literal and
256+
(term instanceof RegExpStar or term instanceof RegExpPlus) and
257+
term.getChild(0) = getADotDotSlashMatcher()
258+
|
259+
literal.getRoot() = term
260+
or
261+
exists(RegExpSequence seq | seq.getNumChild() = 2 and literal.getRoot() = seq |
262+
seq.getChild(0) instanceof RegExpCaret and
263+
seq.getChild(1) = term
264+
)
265+
)
266+
}
267+
268+
/**
269+
* Gets the input path to be sanitized.
270+
*/
271+
DataFlow::Node getInput() { result = input }
272+
273+
/**
274+
* Gets the path where prefix "../" has been removed.
275+
*/
276+
DataFlow::Node getOutput() { result = output }
277+
}
278+
279+
/**
280+
* Gets a RegExpTerm that matches a variation of "../".
281+
*/
282+
private RegExpTerm getADotDotSlashMatcher() {
283+
result.getAMatchedString() = "../"
284+
or
285+
exists(RegExpSequence seq | seq = result |
286+
seq.getChild(0).getConstantValue() = "." and
287+
seq.getChild(1).getConstantValue() = "." and
288+
seq.getAChild().getAMatchedString() = "/"
289+
)
290+
or
291+
exists(RegExpGroup group | result = group | group.getChild(0) = getADotDotSlashMatcher())
292+
}
293+
242294
/**
243295
* A call that removes all "." or ".." from a path, without also removing all forward slashes.
244296
*/

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,52 @@ nodes
12771277
| TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
12781278
| TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
12791279
| TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
1280+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
1281+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
1282+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
1283+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
1284+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
1285+
| TaintedPath.js:201:40:201:43 | path |
1286+
| TaintedPath.js:201:40:201:43 | path |
1287+
| TaintedPath.js:201:40:201:43 | path |
1288+
| TaintedPath.js:201:40:201:43 | path |
1289+
| TaintedPath.js:201:40:201:43 | path |
1290+
| TaintedPath.js:201:40:201:43 | path |
1291+
| TaintedPath.js:201:40:201:43 | path |
1292+
| TaintedPath.js:201:40:201:43 | path |
1293+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1294+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1295+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1296+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1297+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1298+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1299+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1300+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1301+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1302+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1303+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1304+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1305+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1306+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1307+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1308+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1309+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1310+
| TaintedPath.js:202:50:202:53 | path |
1311+
| TaintedPath.js:202:50:202:53 | path |
1312+
| TaintedPath.js:202:50:202:53 | path |
1313+
| TaintedPath.js:202:50:202:53 | path |
1314+
| TaintedPath.js:202:50:202:53 | path |
1315+
| TaintedPath.js:202:50:202:53 | path |
1316+
| TaintedPath.js:202:50:202:53 | path |
1317+
| TaintedPath.js:202:50:202:53 | path |
1318+
| TaintedPath.js:202:50:202:53 | path |
1319+
| TaintedPath.js:202:50:202:53 | path |
1320+
| TaintedPath.js:202:50:202:53 | path |
1321+
| TaintedPath.js:202:50:202:53 | path |
1322+
| TaintedPath.js:202:50:202:53 | path |
1323+
| TaintedPath.js:202:50:202:53 | path |
1324+
| TaintedPath.js:202:50:202:53 | path |
1325+
| TaintedPath.js:202:50:202:53 | path |
12801326
| normalizedPaths.js:11:7:11:27 | path |
12811327
| normalizedPaths.js:11:7:11:27 | path |
12821328
| normalizedPaths.js:11:7:11:27 | path |
@@ -4385,6 +4431,30 @@ edges
43854431
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
43864432
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
43874433
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
4434+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4435+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4436+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4437+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4438+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4439+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4440+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4441+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4442+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4443+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4444+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4445+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4446+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4447+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4448+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4449+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4450+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4451+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4452+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4453+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4454+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4455+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4456+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4457+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
43884458
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
43894459
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
43904460
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
@@ -4561,6 +4631,54 @@ edges
45614631
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
45624632
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
45634633
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
4634+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4635+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4636+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4637+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4638+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4639+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4640+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4641+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4642+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4643+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4644+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4645+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4646+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4647+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4648+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4649+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4650+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4651+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4652+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4653+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4654+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4655+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4656+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4657+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4658+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4659+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4660+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4661+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4662+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4663+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4664+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4665+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4666+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4667+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4668+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4669+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4670+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4671+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4672+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4673+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4674+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4675+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4676+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4677+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4678+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4679+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4680+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
4681+
| TaintedPath.js:202:50:202:53 | path | TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
45644682
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
45654683
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
45664684
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -6391,6 +6509,8 @@ edges
63916509
| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value |
63926510
| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value |
63936511
| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value |
6512+
| TaintedPath.js:201:29:201:73 | "prefix ... +/, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value |
6513+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value |
63946514
| normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
63956515
| normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |
63966516
| normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,4 +191,13 @@ var server = http.createServer(function(req, res) {
191191
res.write(fs.readFileSync(path.replace(/\./g, ''))); // OK
192192
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // OK
193193
}
194+
195+
// removing of "../" from prefix.
196+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // OK
197+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.[\/\\])+/, ''))); // OK
198+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)+/, ''))); // OK
199+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)*/, ''))); // OK
200+
201+
res.write(fs.readFileSync("prefix" + path.replace(/^(\.\.[\/\\])+/, ''))); // NOT OK - not normalized
202+
res.write(fs.readFileSync(pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // NOT OK (can be absolute)
194203
});

0 commit comments

Comments
 (0)