Skip to content

Commit 57b3e6a

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2958 from erik-krogh/InnerPrefix
Approved by asgerf
2 parents 7f3f629 + d2d5af4 commit 57b3e6a

File tree

4 files changed

+186
-0
lines changed

4 files changed

+186
-0
lines changed

‎javascript/ql/src/semmle/javascript/InclusionTests.qll‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,40 @@ module InclusionTest {
5858
boolean getPolarity() { result = true }
5959
}
6060

61+
/**
62+
* A call to a utility function (`callee`) that performs an InclusionTest (`inner`).
63+
*/
64+
private class IndirectInclusionTest extends Range, DataFlow::CallNode {
65+
InclusionTest inner;
66+
Function callee;
67+
68+
IndirectInclusionTest() {
69+
inner.getEnclosingExpr() = callee.getAReturnedExpr() and
70+
this.getACallee() = callee and
71+
count(this.getACallee()) = 1 and
72+
count(callee.getAReturnedExpr()) = 1 and
73+
not this.isImprecise() and
74+
inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getAParameter() and
75+
inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getAParameter()
76+
}
77+
78+
override DataFlow::Node getContainerNode() {
79+
exists(int arg |
80+
inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
81+
result = this.getArgument(arg)
82+
)
83+
}
84+
85+
override DataFlow::Node getContainedNode() {
86+
exists(int arg |
87+
inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
88+
result = this.getArgument(arg)
89+
)
90+
}
91+
92+
override boolean getPolarity() { result = inner.getPolarity() }
93+
}
94+
6195
/**
6296
* A call to a method named `includes`, assumed to refer to `String.prototype.includes`
6397
* or `Array.prototype.includes`.

‎javascript/ql/src/semmle/javascript/StringOps.qll‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,40 @@ module StringOps {
5656
boolean getPolarity() { result = true }
5757
}
5858

59+
/**
60+
* A call to a utility function (`callee`) that performs a StartsWith check (`inner`).
61+
*/
62+
private class IndirectStartsWith extends Range, DataFlow::CallNode {
63+
StartsWith inner;
64+
Function callee;
65+
66+
IndirectStartsWith() {
67+
inner.getEnclosingExpr() = callee.getAReturnedExpr() and
68+
this.getACallee() = callee and
69+
count(this.getACallee()) = 1 and
70+
count(callee.getAReturnedExpr()) = 1 and
71+
not this.isImprecise() and
72+
inner.getBaseString().getALocalSource().getEnclosingExpr() = callee.getAParameter() and
73+
inner.getSubstring().getALocalSource().getEnclosingExpr() = callee.getAParameter()
74+
}
75+
76+
override DataFlow::Node getBaseString() {
77+
exists(int arg |
78+
inner.getBaseString().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
79+
result = this.getArgument(arg)
80+
)
81+
}
82+
83+
override DataFlow::Node getSubstring() {
84+
exists(int arg |
85+
inner.getSubstring().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
86+
result = this.getArgument(arg)
87+
)
88+
}
89+
90+
override boolean getPolarity() { result = inner.getPolarity() }
91+
}
92+
5993
/**
6094
* An expression of form `A.startsWith(B)`.
6195
*/
@@ -253,6 +287,41 @@ module StringOps {
253287
boolean getPolarity() { result = true }
254288
}
255289

290+
/**
291+
* A call to a utility function (`callee`) that performs an EndsWith check (`inner`).
292+
*/
293+
private class IndirectEndsWith extends Range, DataFlow::CallNode {
294+
EndsWith inner;
295+
Function callee;
296+
297+
IndirectEndsWith() {
298+
inner.getEnclosingExpr() = callee.getAReturnedExpr() and
299+
this.getACallee() = callee and
300+
count(this.getACallee()) = 1 and
301+
count(callee.getAReturnedExpr()) = 1 and
302+
not this.isImprecise() and
303+
inner.getBaseString().getALocalSource().getEnclosingExpr() = callee.getAParameter() and
304+
inner.getSubstring().getALocalSource().getEnclosingExpr() = callee.getAParameter()
305+
}
306+
307+
override DataFlow::Node getBaseString() {
308+
exists(int arg |
309+
inner.getBaseString().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
310+
result = this.getArgument(arg)
311+
)
312+
}
313+
314+
override DataFlow::Node getSubstring() {
315+
exists(int arg |
316+
inner.getSubstring().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
317+
result = this.getArgument(arg)
318+
)
319+
}
320+
321+
override boolean getPolarity() { result = inner.getPolarity() }
322+
}
323+
324+
256325
/**
257326
* A call of form `A.endsWith(B)`.
258327
*/

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1654,6 +1654,33 @@ nodes
16541654
| normalizedPaths.js:346:19:346:22 | path |
16551655
| normalizedPaths.js:346:19:346:22 | path |
16561656
| normalizedPaths.js:346:19:346:22 | path |
1657+
| normalizedPaths.js:354:7:354:27 | path |
1658+
| normalizedPaths.js:354:7:354:27 | path |
1659+
| normalizedPaths.js:354:7:354:27 | path |
1660+
| normalizedPaths.js:354:7:354:27 | path |
1661+
| normalizedPaths.js:354:14:354:27 | req.query.path |
1662+
| normalizedPaths.js:354:14:354:27 | req.query.path |
1663+
| normalizedPaths.js:354:14:354:27 | req.query.path |
1664+
| normalizedPaths.js:354:14:354:27 | req.query.path |
1665+
| normalizedPaths.js:354:14:354:27 | req.query.path |
1666+
| normalizedPaths.js:356:19:356:22 | path |
1667+
| normalizedPaths.js:356:19:356:22 | path |
1668+
| normalizedPaths.js:356:19:356:22 | path |
1669+
| normalizedPaths.js:356:19:356:22 | path |
1670+
| normalizedPaths.js:356:19:356:22 | path |
1671+
| normalizedPaths.js:358:7:358:51 | requestPath |
1672+
| normalizedPaths.js:358:7:358:51 | requestPath |
1673+
| normalizedPaths.js:358:7:358:51 | requestPath |
1674+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
1675+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
1676+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
1677+
| normalizedPaths.js:358:47:358:50 | path |
1678+
| normalizedPaths.js:358:47:358:50 | path |
1679+
| normalizedPaths.js:358:47:358:50 | path |
1680+
| normalizedPaths.js:363:21:363:31 | requestPath |
1681+
| normalizedPaths.js:363:21:363:31 | requestPath |
1682+
| normalizedPaths.js:363:21:363:31 | requestPath |
1683+
| normalizedPaths.js:363:21:363:31 | requestPath |
16571684
| tainted-require.js:7:19:7:37 | req.param("module") |
16581685
| tainted-require.js:7:19:7:37 | req.param("module") |
16591686
| tainted-require.js:7:19:7:37 | req.param("module") |
@@ -4582,6 +4609,37 @@ edges
45824609
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
45834610
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
45844611
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4612+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4613+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4614+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4615+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4616+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4617+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4618+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4619+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:356:19:356:22 | path |
4620+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:358:47:358:50 | path |
4621+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:358:47:358:50 | path |
4622+
| normalizedPaths.js:354:7:354:27 | path | normalizedPaths.js:358:47:358:50 | path |
4623+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4624+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4625+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4626+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4627+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4628+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4629+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4630+
| normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:354:7:354:27 | path |
4631+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4632+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4633+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4634+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4635+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4636+
| normalizedPaths.js:358:7:358:51 | requestPath | normalizedPaths.js:363:21:363:31 | requestPath |
4637+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) | normalizedPaths.js:358:7:358:51 | requestPath |
4638+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) | normalizedPaths.js:358:7:358:51 | requestPath |
4639+
| normalizedPaths.js:358:21:358:51 | pathMod ... , path) | normalizedPaths.js:358:7:358:51 | requestPath |
4640+
| normalizedPaths.js:358:47:358:50 | path | normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
4641+
| normalizedPaths.js:358:47:358:50 | path | normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
4642+
| normalizedPaths.js:358:47:358:50 | path | normalizedPaths.js:358:21:358:51 | pathMod ... , path) |
45854643
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
45864644
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
45874645
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -5464,6 +5522,8 @@ edges
54645522
| normalizedPaths.js:332:19:332:32 | normalizedPath | normalizedPaths.js:303:13:303:26 | req.query.path | normalizedPaths.js:332:19:332:32 | normalizedPath | This path depends on $@. | normalizedPaths.js:303:13:303:26 | req.query.path | a user-provided value |
54655523
| normalizedPaths.js:341:18:341:21 | path | normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:341:18:341:21 | path | This path depends on $@. | normalizedPaths.js:339:32:339:45 | req.query.path | a user-provided value |
54665524
| normalizedPaths.js:346:19:346:22 | path | normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:346:19:346:22 | path | This path depends on $@. | normalizedPaths.js:339:32:339:45 | req.query.path | a user-provided value |
5525+
| normalizedPaths.js:356:19:356:22 | path | normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:356:19:356:22 | path | This path depends on $@. | normalizedPaths.js:354:14:354:27 | req.query.path | a user-provided value |
5526+
| normalizedPaths.js:363:21:363:31 | requestPath | normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:363:21:363:31 | requestPath | This path depends on $@. | normalizedPaths.js:354:14:354:27 | req.query.path | a user-provided value |
54675527
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | This path depends on $@. | tainted-require.js:7:19:7:37 | req.param("module") | a user-provided value |
54685528
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
54695529
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,4 +347,27 @@ app.get('/yet-another-prefix', (req, res) => {
347347
return;
348348
}
349349
fs.readFileSync(path); // OK
350+
});
351+
352+
var rootPath = process.cwd();
353+
app.get('/yet-another-prefix2', (req, res) => {
354+
let path = req.query.path;
355+
356+
fs.readFileSync(path); // NOT OK
357+
358+
var requestPath = pathModule.join(rootPath, path);
359+
360+
var targetPath;
361+
if (!allowPath(requestPath, rootPath)) {
362+
targetPath = rootPath;
363+
fs.readFileSync(requestPath); // NOT OK
364+
} else {
365+
targetPath = requestPath;
366+
fs.readFileSync(requestPath); // OK
367+
}
368+
fs.readFileSync(targetPath); // OK
369+
370+
function allowPath(requestPath, rootPath) {
371+
return requestPath.indexOf(rootPath) === 0;
372+
}
350373
});

0 commit comments

Comments
 (0)