Skip to content

Commit 03b8823

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#2723 from esbena/js/support-path-is-inside
Approved by asgerf
2 parents b453cf8 + 5baba62 commit 03b8823

File tree

4 files changed

+183
-1
lines changed

4 files changed

+183
-1
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ module TaintedPath {
3535
guard instanceof StartsWithDotDotSanitizer or
3636
guard instanceof StartsWithDirSanitizer or
3737
guard instanceof IsAbsoluteSanitizer or
38-
guard instanceof ContainsDotDotSanitizer
38+
guard instanceof ContainsDotDotSanitizer or
39+
guard instanceof IsInsideCheckSanitizer
3940
}
4041

4142
override predicate isAdditionalFlowStep(

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,39 @@ module TaintedPath {
369369
*/
370370
private class VarAccessBarrier extends Sanitizer, DataFlow::VarAccessBarrier { }
371371

372+
/**
373+
* An expression of form `isInside(x, y)` or similar, where `isInside` is
374+
* a library check for the relation between `x` and `y`.
375+
*/
376+
class IsInsideCheckSanitizer extends DataFlow::LabeledBarrierGuardNode {
377+
DataFlow::Node checked;
378+
boolean onlyNormalizedAbsolutePaths;
379+
380+
IsInsideCheckSanitizer() {
381+
exists(string name, DataFlow::CallNode check |
382+
name = "path-is-inside" and onlyNormalizedAbsolutePaths = true
383+
or
384+
name = "is-path-inside" and onlyNormalizedAbsolutePaths = false
385+
|
386+
check = DataFlow::moduleImport(name).getACall() and
387+
checked = check.getArgument(0) and
388+
check = this
389+
)
390+
}
391+
392+
override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
393+
(
394+
onlyNormalizedAbsolutePaths = true and
395+
label.(Label::PosixPath).isNormalized() and
396+
label.(Label::PosixPath).isAbsolute()
397+
or
398+
onlyNormalizedAbsolutePaths = false
399+
) and
400+
e = checked.asExpr() and
401+
outcome = true
402+
}
403+
}
404+
372405
/**
373406
* A source of remote user input, considered as a flow source for
374407
* tainted-path vulnerabilities.

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

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1495,6 +1495,51 @@ nodes
14951495
| normalizedPaths.js:250:21:250:24 | path |
14961496
| normalizedPaths.js:250:21:250:24 | path |
14971497
| normalizedPaths.js:250:21:250:24 | path |
1498+
| normalizedPaths.js:256:6:256:26 | path |
1499+
| normalizedPaths.js:256:6:256:26 | path |
1500+
| normalizedPaths.js:256:6:256:26 | path |
1501+
| normalizedPaths.js:256:6:256:26 | path |
1502+
| normalizedPaths.js:256:13:256:26 | req.query.path |
1503+
| normalizedPaths.js:256:13:256:26 | req.query.path |
1504+
| normalizedPaths.js:256:13:256:26 | req.query.path |
1505+
| normalizedPaths.js:256:13:256:26 | req.query.path |
1506+
| normalizedPaths.js:256:13:256:26 | req.query.path |
1507+
| normalizedPaths.js:257:18:257:21 | path |
1508+
| normalizedPaths.js:257:18:257:21 | path |
1509+
| normalizedPaths.js:257:18:257:21 | path |
1510+
| normalizedPaths.js:257:18:257:21 | path |
1511+
| normalizedPaths.js:257:18:257:21 | path |
1512+
| normalizedPaths.js:262:19:262:22 | path |
1513+
| normalizedPaths.js:262:19:262:22 | path |
1514+
| normalizedPaths.js:262:19:262:22 | path |
1515+
| normalizedPaths.js:262:19:262:22 | path |
1516+
| normalizedPaths.js:262:19:262:22 | path |
1517+
| normalizedPaths.js:266:19:266:22 | path |
1518+
| normalizedPaths.js:266:19:266:22 | path |
1519+
| normalizedPaths.js:266:19:266:22 | path |
1520+
| normalizedPaths.js:266:19:266:22 | path |
1521+
| normalizedPaths.js:269:19:269:22 | path |
1522+
| normalizedPaths.js:269:19:269:22 | path |
1523+
| normalizedPaths.js:269:19:269:22 | path |
1524+
| normalizedPaths.js:269:19:269:22 | path |
1525+
| normalizedPaths.js:269:19:269:22 | path |
1526+
| normalizedPaths.js:273:6:273:49 | normalizedPath |
1527+
| normalizedPaths.js:273:6:273:49 | normalizedPath |
1528+
| normalizedPaths.js:273:6:273:49 | normalizedPath |
1529+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
1530+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
1531+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
1532+
| normalizedPaths.js:273:45:273:48 | path |
1533+
| normalizedPaths.js:273:45:273:48 | path |
1534+
| normalizedPaths.js:273:45:273:48 | path |
1535+
| normalizedPaths.js:278:19:278:32 | normalizedPath |
1536+
| normalizedPaths.js:278:19:278:32 | normalizedPath |
1537+
| normalizedPaths.js:278:19:278:32 | normalizedPath |
1538+
| normalizedPaths.js:278:19:278:32 | normalizedPath |
1539+
| normalizedPaths.js:285:19:285:32 | normalizedPath |
1540+
| normalizedPaths.js:285:19:285:32 | normalizedPath |
1541+
| normalizedPaths.js:285:19:285:32 | normalizedPath |
1542+
| normalizedPaths.js:285:19:285:32 | normalizedPath |
14981543
| tainted-require.js:7:19:7:37 | req.param("module") |
14991544
| tainted-require.js:7:19:7:37 | req.param("module") |
15001545
| tainted-require.js:7:19:7:37 | req.param("module") |
@@ -4228,6 +4273,65 @@ edges
42284273
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
42294274
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
42304275
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
4276+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4277+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4278+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4279+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4280+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4281+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4282+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4283+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:257:18:257:21 | path |
4284+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4285+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4286+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4287+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4288+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4289+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4290+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4291+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:262:19:262:22 | path |
4292+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4293+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4294+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4295+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4296+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4297+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:266:19:266:22 | path |
4298+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4299+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4300+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4301+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4302+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4303+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4304+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4305+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:269:19:269:22 | path |
4306+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:273:45:273:48 | path |
4307+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:273:45:273:48 | path |
4308+
| normalizedPaths.js:256:6:256:26 | path | normalizedPaths.js:273:45:273:48 | path |
4309+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4310+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4311+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4312+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4313+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4314+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4315+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4316+
| normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:256:6:256:26 | path |
4317+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4318+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4319+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4320+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4321+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4322+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:278:19:278:32 | normalizedPath |
4323+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4324+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4325+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4326+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4327+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4328+
| normalizedPaths.js:273:6:273:49 | normalizedPath | normalizedPaths.js:285:19:285:32 | normalizedPath |
4329+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) | normalizedPaths.js:273:6:273:49 | normalizedPath |
4330+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) | normalizedPaths.js:273:6:273:49 | normalizedPath |
4331+
| normalizedPaths.js:273:23:273:49 | pathMod ... , path) | normalizedPaths.js:273:6:273:49 | normalizedPath |
4332+
| normalizedPaths.js:273:45:273:48 | path | normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
4333+
| normalizedPaths.js:273:45:273:48 | path | normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
4334+
| normalizedPaths.js:273:45:273:48 | path | normalizedPaths.js:273:23:273:49 | pathMod ... , path) |
42314335
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
42324336
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
42334337
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -5096,6 +5200,12 @@ edges
50965200
| normalizedPaths.js:238:19:238:22 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:238:19:238:22 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
50975201
| normalizedPaths.js:245:21:245:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:245:21:245:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
50985202
| normalizedPaths.js:250:21:250:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:250:21:250:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
5203+
| normalizedPaths.js:257:18:257:21 | path | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:257:18:257:21 | path | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
5204+
| normalizedPaths.js:262:19:262:22 | path | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:262:19:262:22 | path | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
5205+
| normalizedPaths.js:266:19:266:22 | path | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:266:19:266:22 | path | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
5206+
| normalizedPaths.js:269:19:269:22 | path | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:269:19:269:22 | path | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
5207+
| normalizedPaths.js:278:19:278:32 | normalizedPath | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:278:19:278:32 | normalizedPath | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
5208+
| normalizedPaths.js:285:19:285:32 | normalizedPath | normalizedPaths.js:256:13:256:26 | req.query.path | normalizedPaths.js:285:19:285:32 | normalizedPath | This path depends on $@. | normalizedPaths.js:256:13:256:26 | req.query.path | a user-provided value |
50995209
| 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 |
51005210
| 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 |
51015211
| 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: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,3 +249,41 @@ app.get('/resolve-path', (req, res) => {
249249
else
250250
fs.readFileSync(path); // NOT OK - wrong polarity
251251
});
252+
253+
var isPathInside = require("is-path-inside"),
254+
pathIsInside = require("path-is-inside");
255+
app.get('/pseudo-normalizations', (req, res) => {
256+
let path = req.query.path;
257+
fs.readFileSync(path); // NOT OK
258+
if (isPathInside(path, SAFE)) {
259+
fs.readFileSync(path); // OK
260+
return;
261+
} else {
262+
fs.readFileSync(path); // NOT OK
263+
264+
}
265+
if (pathIsInside(path, SAFE)) {
266+
fs.readFileSync(path); // NOT OK - can be of the form 'safe/directory/../../../etc/passwd'
267+
return;
268+
} else {
269+
fs.readFileSync(path); // NOT OK
270+
271+
}
272+
273+
let normalizedPath = pathModule.join(SAFE, path);
274+
if (pathIsInside(normalizedPath, SAFE)) {
275+
fs.readFileSync(normalizedPath); // OK
276+
return;
277+
} else {
278+
fs.readFileSync(normalizedPath); // NOT OK
279+
}
280+
281+
if (pathIsInside(normalizedPath, SAFE)) {
282+
fs.readFileSync(normalizedPath); // OK
283+
return;
284+
} else {
285+
fs.readFileSync(normalizedPath); // NOT OK
286+
287+
}
288+
289+
});

0 commit comments

Comments
 (0)