Skip to content

Commit 0bd9e9f

Browse files
author
Stephan Brandauer
committed
add handlebars taint step
1 parent 5b97458 commit 0bd9e9f

File tree

4 files changed

+144
-1
lines changed

4 files changed

+144
-1
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/Handlebars.qll

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,69 @@ module Handlebars {
2727
SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
2828
}
2929
}
30+
31+
/** Provides logic for taint steps for the handlebars library. */
32+
module TaintStep {
33+
/**
34+
* Gets a `SourceNode` tracked from a compilation of a Handlebars template.
35+
*/
36+
private DataFlow::SourceNode compiledHandlebarsTemplate(DataFlow::Node originalCall) {
37+
result = compiledHandlebarsTemplate(DataFlow::TypeTracker::end(), originalCall)
38+
}
39+
40+
private DataFlow::SourceNode compiledHandlebarsTemplate(
41+
DataFlow::TypeTracker t, DataFlow::Node originalCall
42+
) {
43+
t.start() and
44+
result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
45+
result = originalCall
46+
or
47+
exists(DataFlow::TypeTracker t2 |
48+
result = compiledHandlebarsTemplate(t2, originalCall).track(t2, t)
49+
)
50+
}
51+
52+
/**
53+
* Holds if there's a step from `pred` to `succ` due to templating data being
54+
* passed from a templating call to a registered helper via a parameter.
55+
*/
56+
private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
57+
exists(string helperName |
58+
exists(DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall |
59+
templatingCall = compiledHandlebarsTemplate(compileCall).getACall() and
60+
pred = templatingCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs() and
61+
(
62+
compileCall
63+
.getArgument(0)
64+
.mayHaveStringValue(any(string templateText |
65+
// an approximation: if the template contains the
66+
// helper name, it's probably a helper call
67+
templateText.matches("%" + helperName + "%")
68+
))
69+
or
70+
// When we don't have a string value, we can't be sure
71+
// and will assume a step.
72+
not exists(string s | compileCall.getArgument(0).mayHaveStringValue(s))
73+
)
74+
) and
75+
exists(DataFlow::CallNode registerHelperCall, DataFlow::FunctionNode helperFunc |
76+
registerHelperCall = any(Handlebars::Handlebars hb).getAMethodCall("registerHelper") and
77+
registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
78+
helperFunc = registerHelperCall.getArgument(1).getAFunctionValue() and
79+
helperFunc.getAParameter() = succ
80+
)
81+
)
82+
}
83+
84+
/**
85+
* A shared flow step from passing data to a handlebars template with
86+
* helpers registered.
87+
*/
88+
class HandlebarsStep extends DataFlow::SharedFlowStep {
89+
DataFlow::CallNode templatingCall;
90+
91+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
92+
isHandlebarsArgStep(node1, node2)
93+
}
94+
}
95+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -850,7 +850,7 @@ module TaintedPath {
850850

851851
/**
852852
* Holds if we should include a step from `src -> dst` with labels `srclabel -> dstlabel`, and the
853-
* standard taint step `src -> dst` should be suppresesd.
853+
* standard taint step `src -> dst` should be suppressed.
854854
*/
855855
private predicate isPosixPathStep(
856856
DataFlow::Node src, DataFlow::Node dst, Label::PosixPath srclabel, Label::PosixPath dstlabel

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,6 +1541,25 @@ nodes
15411541
| express.js:8:20:8:32 | req.query.bar |
15421542
| express.js:8:20:8:32 | req.query.bar |
15431543
| express.js:8:20:8:32 | req.query.bar |
1544+
| handlebars.js:19:46:19:60 | req.params.path |
1545+
| handlebars.js:19:46:19:60 | req.params.path |
1546+
| handlebars.js:19:46:19:60 | req.params.path |
1547+
| handlebars.js:19:46:19:60 | req.params.path |
1548+
| handlebars.js:19:46:19:60 | req.params.path |
1549+
| handlebars.js:22:18:22:25 | filePath |
1550+
| handlebars.js:22:18:22:25 | filePath |
1551+
| handlebars.js:22:18:22:25 | filePath |
1552+
| handlebars.js:22:18:22:25 | filePath |
1553+
| handlebars.js:23:28:23:35 | filePath |
1554+
| handlebars.js:23:28:23:35 | filePath |
1555+
| handlebars.js:23:28:23:35 | filePath |
1556+
| handlebars.js:23:28:23:35 | filePath |
1557+
| handlebars.js:23:28:23:35 | filePath |
1558+
| handlebars.js:31:43:31:57 | req.params.name |
1559+
| handlebars.js:31:43:31:57 | req.params.name |
1560+
| handlebars.js:31:43:31:57 | req.params.name |
1561+
| handlebars.js:31:43:31:57 | req.params.name |
1562+
| handlebars.js:31:43:31:57 | req.params.name |
15441563
| normalizedPaths.js:11:7:11:27 | path |
15451564
| normalizedPaths.js:11:7:11:27 | path |
15461565
| normalizedPaths.js:11:7:11:27 | path |
@@ -6414,6 +6433,30 @@ edges
64146433
| TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
64156434
| TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
64166435
| express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
6436+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6437+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6438+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6439+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6440+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6441+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6442+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6443+
| handlebars.js:19:46:19:60 | req.params.path | handlebars.js:22:18:22:25 | filePath |
6444+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6445+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6446+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6447+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6448+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6449+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6450+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6451+
| handlebars.js:22:18:22:25 | filePath | handlebars.js:23:28:23:35 | filePath |
6452+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6453+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6454+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6455+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6456+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6457+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6458+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
6459+
| handlebars.js:31:43:31:57 | req.params.name | handlebars.js:22:18:22:25 | filePath |
64176460
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
64186461
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
64196462
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9844,6 +9887,8 @@ edges
98449887
| TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
98459888
| TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on $@. | TaintedPath.js:211:24:211:30 | req.url | a user-provided value |
98469889
| express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on $@. | express.js:8:20:8:32 | req.query.bar | a user-provided value |
9890+
| handlebars.js:23:28:23:35 | filePath | handlebars.js:19:46:19:60 | req.params.path | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:19:46:19:60 | req.params.path | a user-provided value |
9891+
| handlebars.js:23:28:23:35 | filePath | handlebars.js:31:43:31:57 | req.params.name | handlebars.js:23:28:23:35 | filePath | This path depends on $@. | handlebars.js:31:43:31:57 | req.params.name | a user-provided value |
98479892
| 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 |
98489893
| 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 |
98499894
| 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/handlebars.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
const express = require('express');
2+
const hb = require("handlebars");
3+
const fs = require("fs");
4+
5+
const app = express();
6+
7+
const data = {};
8+
9+
function init() {
10+
data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
11+
data.compiledBenign = hb.compile("hello, {{name}}");
12+
data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
13+
hb.registerHelper("catFile", catFile);
14+
}
15+
16+
init();
17+
18+
app.get('/some/path1', function (req, res) {
19+
res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
20+
});
21+
22+
function catFile(filePath) {
23+
return fs.readFileSync(filePath); // SINK (reads file)
24+
}
25+
26+
app.get('/some/path2', function (req, res) {
27+
res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
28+
});
29+
30+
app.get('/some/path3', function (req, res) {
31+
res.send(data.compiledUnknown({ name: req.params.name })); // NOT ALLOWED (could be using vulnerable catFile)
32+
});

0 commit comments

Comments
 (0)