Skip to content

Commit a8098a2

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3197 from erik-krogh/NormalPathSanitizer
Approved by asgerf
2 parents 676da02 + 9c20531 commit a8098a2

File tree

4 files changed

+192
-1
lines changed

4 files changed

+192
-1
lines changed

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,20 @@ 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()
213+
|
214+
// the 4 possible combinations of normalized + relative for `srclabel`, and the possible values for `dstlabel` in each case.
215+
srclabel.isNonNormalized() and srclabel.isRelative() // raw + relative -> any()
216+
or
217+
srclabel.isNormalized() and srclabel.isAbsolute() and srclabel = dstlabel // normalized + absolute -> normalized + absolute
218+
or
219+
srclabel.isNonNormalized() and srclabel.isAbsolute() and dstlabel.isAbsolute() // raw + absolute -> raw/normalized + absolute
220+
// normalized + relative -> none()
221+
)
222+
or
209223
// path.join()
210224
exists(DataFlow::CallNode join, int n |
211225
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: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,48 @@ 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:201:40:201:73 | path.re ... +/, '') |
1298+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1299+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1300+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1301+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1302+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1303+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1304+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
1305+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1306+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1307+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1308+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) |
1309+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1310+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1311+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1312+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
1313+
| TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
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 |
12801322
| normalizedPaths.js:11:7:11:27 | path |
12811323
| normalizedPaths.js:11:7:11:27 | path |
12821324
| normalizedPaths.js:11:7:11:27 | path |
@@ -4385,6 +4427,22 @@ edges
43854427
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
43864428
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
43874429
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path |
4430+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4431+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4432+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | path |
4433+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:201:40:201:43 | 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:202:50:202:53 | path |
4439+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4440+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | path |
4441+
| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:202:50:202:53 | 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 |
43884446
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
43894447
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
43904448
| TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query |
@@ -4561,6 +4619,62 @@ edges
45614619
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
45624620
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
45634621
| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') |
4622+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4623+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4624+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4625+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4626+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4627+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4628+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4629+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4630+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4631+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4632+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
4633+
| TaintedPath.js:201:40:201:43 | path | TaintedPath.js:201:40:201:73 | path.re ... +/, '') |
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:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4639+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4640+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4641+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
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:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4651+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4652+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4653+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4654+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4655+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4656+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4657+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4658+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4659+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4660+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
4661+
| TaintedPath.js:201:40:201:73 | path.re ... +/, '') | TaintedPath.js:201:29:201:73 | "prefix ... +/, '') |
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:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4667+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4668+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
4669+
| TaintedPath.js:202:29:202:54 | pathMod ... e(path) | TaintedPath.js:202:29:202:84 | pathMod ... +/, '') |
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) |
45644678
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
45654679
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
45664680
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -6391,6 +6505,8 @@ edges
63916505
| 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 |
63926506
| 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 |
63936507
| 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 |
6508+
| 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 |
6509+
| 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 |
63946510
| 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 |
63956511
| 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 |
63966512
| 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)