Skip to content

Commit 3a14651

Browse files
erik-krogherik-krogh
committed
add sanitizer for relative ".." in js/path-injection
1 parent da566a4 commit 3a14651

File tree

4 files changed

+126
-10
lines changed

4 files changed

+126
-10
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 RelativePathContainsDotDotGuard
3940
}
4041

4142
override predicate isAdditionalFlowStep(

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

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,19 +12,17 @@ module TaintedPath {
1212
*/
1313
abstract class Source extends DataFlow::Node {
1414
/** Gets a flow label denoting the type of value for which this is a source. */
15-
DataFlow::FlowLabel getAFlowLabel() {
16-
result instanceof Label::PosixPath
17-
}
15+
DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::PosixPath }
1816
}
1917

2018
/**
2119
* A data flow sink for tainted-path vulnerabilities.
2220
*/
2321
abstract class Sink extends DataFlow::Node {
22+
Sink() { not this instanceof Sanitizer }
23+
2424
/** Gets a flow label denoting the type of value for which this is a sink. */
25-
DataFlow::FlowLabel getAFlowLabel() {
26-
result instanceof Label::PosixPath
27-
}
25+
DataFlow::FlowLabel getAFlowLabel() { result instanceof Label::PosixPath }
2826
}
2927

3028
/**
@@ -355,6 +353,35 @@ module TaintedPath {
355353
}
356354
}
357355

356+
/**
357+
* A sanitizer that recognizes the following pattern:
358+
* var relative = path.relative(webroot, pathname);
359+
* if(relative.startsWith(".." + path.sep) || relative == "..") {
360+
* // pathname is unsafe
361+
* } else {
362+
* // pathname is safe
363+
* }
364+
*/
365+
class RelativePathContainsDotDotGuard extends DataFlow::BarrierGuardNode {
366+
StringOps::StartsWith startsWith;
367+
DataFlow::CallNode relativeCall;
368+
369+
RelativePathContainsDotDotGuard() {
370+
this = startsWith and
371+
relativeCall = DataFlow::moduleImport("path").getAMemberCall("relative") and
372+
startsWith.getBaseString().getALocalSource() = relativeCall and
373+
exists(DataFlow::Node subString | subString = startsWith.getSubstring() |
374+
subString.mayHaveStringValue("..")
375+
or
376+
subString.(StringOps::ConcatenationRoot).getFirstLeaf().mayHaveStringValue("..")
377+
)
378+
}
379+
380+
override predicate blocks(boolean outcome, Expr e) {
381+
e = relativeCall.getArgument(1).asExpr() and outcome = false
382+
}
383+
}
384+
358385
/**
359386
* A source of remote user input, considered as a flow source for
360387
* tainted-path vulnerabilities.
@@ -396,9 +423,7 @@ module TaintedPath {
396423
* A path argument to a file system access, which disallows upward navigation.
397424
*/
398425
private class FsPathSinkWithoutUpwardNavigation extends FsPathSink {
399-
FsPathSinkWithoutUpwardNavigation() {
400-
fileSystemAccess.isUpwardNavigationRejected(this)
401-
}
426+
FsPathSinkWithoutUpwardNavigation() { fileSystemAccess.isUpwardNavigationRejected(this) }
402427

403428
override DataFlow::FlowLabel getAFlowLabel() {
404429
// The protection is ineffective if the ../ segments have already

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1259,6 +1259,34 @@ nodes
12591259
| normalizedPaths.js:250:21:250:24 | path |
12601260
| normalizedPaths.js:250:21:250:24 | path |
12611261
| normalizedPaths.js:250:21:250:24 | path |
1262+
| normalizedPaths.js:254:7:254:47 | path |
1263+
| normalizedPaths.js:254:7:254:47 | path |
1264+
| normalizedPaths.js:254:7:254:47 | path |
1265+
| normalizedPaths.js:254:7:254:47 | path |
1266+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
1267+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
1268+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
1269+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
1270+
| normalizedPaths.js:254:33:254:46 | req.query.path |
1271+
| normalizedPaths.js:254:33:254:46 | req.query.path |
1272+
| normalizedPaths.js:254:33:254:46 | req.query.path |
1273+
| normalizedPaths.js:254:33:254:46 | req.query.path |
1274+
| normalizedPaths.js:254:33:254:46 | req.query.path |
1275+
| normalizedPaths.js:256:19:256:22 | path |
1276+
| normalizedPaths.js:256:19:256:22 | path |
1277+
| normalizedPaths.js:256:19:256:22 | path |
1278+
| normalizedPaths.js:256:19:256:22 | path |
1279+
| normalizedPaths.js:256:19:256:22 | path |
1280+
| normalizedPaths.js:262:21:262:24 | path |
1281+
| normalizedPaths.js:262:21:262:24 | path |
1282+
| normalizedPaths.js:262:21:262:24 | path |
1283+
| normalizedPaths.js:262:21:262:24 | path |
1284+
| normalizedPaths.js:262:21:262:24 | path |
1285+
| normalizedPaths.js:270:21:270:24 | path |
1286+
| normalizedPaths.js:270:21:270:24 | path |
1287+
| normalizedPaths.js:270:21:270:24 | path |
1288+
| normalizedPaths.js:270:21:270:24 | path |
1289+
| normalizedPaths.js:270:21:270:24 | path |
12621290
| tainted-require.js:7:19:7:37 | req.param("module") |
12631291
| tainted-require.js:7:19:7:37 | req.param("module") |
12641292
| tainted-require.js:7:19:7:37 | req.param("module") |
@@ -3630,6 +3658,42 @@ edges
36303658
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
36313659
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
36323660
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
3661+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3662+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3663+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3664+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3665+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3666+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3667+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3668+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:256:19:256:22 | path |
3669+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3670+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3671+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3672+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3673+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3674+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3675+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3676+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:262:21:262:24 | path |
3677+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3678+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3679+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3680+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3681+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3682+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3683+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3684+
| normalizedPaths.js:254:7:254:47 | path | normalizedPaths.js:270:21:270:24 | path |
3685+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
3686+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
3687+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
3688+
| normalizedPaths.js:254:14:254:47 | pathMod ... y.path) | normalizedPaths.js:254:7:254:47 | path |
3689+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3690+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3691+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3692+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3693+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3694+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3695+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
3696+
| normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:254:14:254:47 | pathMod ... y.path) |
36333697
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
36343698
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
36353699
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -4411,6 +4475,9 @@ edges
44114475
| 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 |
44124476
| 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 |
44134477
| 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 |
4478+
| normalizedPaths.js:256:19:256:22 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:256:19:256:22 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
4479+
| normalizedPaths.js:262:21:262:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:262:21:262:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
4480+
| normalizedPaths.js:270:21:270:24 | path | normalizedPaths.js:254:33:254:46 | req.query.path | normalizedPaths.js:270:21:270:24 | path | This path depends on $@. | normalizedPaths.js:254:33:254:46 | req.query.path | a user-provided value |
44144481
| 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 |
44154482
| 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 |
44164483
| 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
@@ -249,3 +249,26 @@ app.get('/resolve-path', (req, res) => {
249249
else
250250
fs.readFileSync(path); // NOT OK - wrong polarity
251251
});
252+
253+
app.get('/relative-startswith', (req, res) => {
254+
let path = pathModule.resolve(req.query.path);
255+
256+
fs.readFileSync(path); // NOT OK
257+
258+
var self = something();
259+
260+
var relative = pathModule.relative(self.webroot, path);
261+
if(relative.startsWith(".." + pathModule.sep) || relative == "..") {
262+
fs.readFileSync(path); // NOT OK!
263+
} else {
264+
fs.readFileSync(path); // OK!
265+
}
266+
267+
let newpath = pathModule.normalize(p);
268+
var relativePath = path.relative(path.normalize(workspaceDir), newpath);
269+
if (relativePath.indexOf('..' + pathModule.sep) === 0) {
270+
fs.readFileSync(path); // NOT OK!
271+
} else {
272+
fs.readFileSync(newpath); // OK!
273+
}
274+
});

0 commit comments

Comments
 (0)