Skip to content

Commit 2fb3147

Browse files
author
Stephan Brandauer
authored
Merge pull request github#8430 from kaeluka/js/CVE-2022-24718
JS: Add taint step for handlebars model
2 parents 91324d4 + fb66ccf commit 2fb3147

File tree

4 files changed

+245
-1
lines changed

4 files changed

+245
-1
lines changed

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,133 @@ module Handlebars {
2727
SafeString() { this = any(Handlebars h).getAConstructorInvocation("SafeString") }
2828
}
2929
}
30+
31+
/** Provides logic for taint steps for the handlebars library. */
32+
private module HandlebarsTaintSteps {
33+
/**
34+
* Gets a reference to a compiled Handlebars template.
35+
*/
36+
private DataFlow::SourceNode compiledTemplate(DataFlow::CallNode compileCall) {
37+
result = compiledTemplate(DataFlow::TypeTracker::end(), compileCall)
38+
}
39+
40+
private DataFlow::SourceNode compiledTemplate(
41+
DataFlow::TypeTracker t, DataFlow::CallNode compileCall
42+
) {
43+
t.start() and
44+
result = any(Handlebars::Handlebars hb).getAMethodCall(["compile", "template"]) and
45+
result = compileCall
46+
or
47+
exists(DataFlow::TypeTracker t2 | result = compiledTemplate(t2, compileCall).track(t2, t))
48+
}
49+
50+
/**
51+
* Gets a reference to a parameter of a registered Handlebars helper.
52+
*
53+
* ```javascript
54+
* function loudHelper(text) {
55+
* return text.toUpperCase();
56+
* }
57+
*
58+
* hb.registerHelper("loud", loudHelper);
59+
* ```
60+
* In this example, `getRegisteredHelperParameter("loud", func, 0)` will bind `func` to
61+
* the `FunctionNode` representing `function loudHelper`, and return its parameter `text`.
62+
*/
63+
private DataFlow::ParameterNode getRegisteredHelperParam(
64+
string helperName, DataFlow::FunctionNode helperFunction, int paramIndex
65+
) {
66+
exists(DataFlow::CallNode registerHelperCall |
67+
registerHelperCall = any(Handlebars::Handlebars hb).getAMemberCall("registerHelper") and
68+
registerHelperCall.getArgument(0).mayHaveStringValue(helperName) and
69+
helperFunction = registerHelperCall.getArgument(1).getAFunctionValue() and
70+
result = helperFunction.getParameter(paramIndex)
71+
)
72+
}
73+
74+
/**
75+
* Gets a `call` (which is a block wrapped inside curly braces inside the template) from `templateText`.
76+
*
77+
* For example, `getAHelperCallFromTemplate("Hello {{loud customer}}")` will return `"loud customer"`.
78+
*/
79+
bindingset[templateText]
80+
private string getAHelperCallFromTemplate(string templateText) {
81+
result = templateText.regexpFind("\\{\\{[^}]+\\}\\}", _, _).regexpReplaceAll("[{}]", "").trim() and
82+
result.regexpMatch(".*\\s.*")
83+
}
84+
85+
/**
86+
* Holds for calls to helpers from handlebars templates.
87+
*
88+
* ```javascript
89+
* hb.compile("contents of file {{path}} are: {{catFile path}} {{echo p1 p2}}");
90+
* ```
91+
*
92+
* In the example, the predicate will hold for:
93+
*
94+
* * helperName="catFile", argIdx=1, arg="path"
95+
* * helperName="echo", argIdx=1, arg="p1"
96+
* * helperName="echo", argIdx=2, arg="p2"
97+
*
98+
* The initial `{{path}}` will not be considered, as it has no arguments.
99+
*/
100+
bindingset[templateText]
101+
private predicate isTemplateHelperCallArg(
102+
string templateText, string helperName, int argIdx, string argVal
103+
) {
104+
exists(string call | call = getAHelperCallFromTemplate(templateText) |
105+
helperName = call.regexpFind("[^\\s]+", 0, _) and
106+
argIdx >= 0 and
107+
argVal = call.regexpFind("[^\\s]+", argIdx + 1, _)
108+
)
109+
}
110+
111+
/**
112+
* Holds if there's a step from `pred` to `succ` due to templating data being
113+
* passed from a templating call to a registered helper via a parameter.
114+
*
115+
* To establish the step, we look at the template passed to `compile`, and will
116+
* only track steps from templates to helpers they actually reference.
117+
*
118+
* ```javascript
119+
* function loudHelper(text) {
120+
* // ^^^^ succ
121+
* return text.toUpperCase();
122+
* }
123+
*
124+
* hb.registerHelper("loud", loudHelper);
125+
*
126+
* const template = hb.compile("Hello, {{loud name}}!");
127+
*
128+
* template({name: "user"});
129+
* // ^^^^^^ pred
130+
* ```
131+
*/
132+
private predicate isHandlebarsArgStep(DataFlow::Node pred, DataFlow::Node succ) {
133+
exists(
134+
string helperName, DataFlow::CallNode templatingCall, DataFlow::CallNode compileCall,
135+
DataFlow::FunctionNode helperFunction
136+
|
137+
templatingCall = compiledTemplate(compileCall).getACall() and
138+
exists(string templateText, string paramName, int argIdx |
139+
compileCall.getArgument(0).mayHaveStringValue(templateText)
140+
|
141+
pred = templatingCall.getArgument(0).getALocalSource().getAPropertyWrite(paramName).getRhs() and
142+
isTemplateHelperCallArg(templateText, helperName, argIdx, paramName) and
143+
succ = getRegisteredHelperParam(helperName, helperFunction, argIdx)
144+
)
145+
)
146+
}
147+
148+
/**
149+
* A shared flow step from passing data to a handlebars template with
150+
* helpers registered.
151+
*/
152+
class HandlebarsStep extends DataFlow::SharedFlowStep {
153+
DataFlow::CallNode templatingCall;
154+
155+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
156+
isHandlebarsArgStep(pred, succ)
157+
}
158+
}
159+
}

‎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: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,6 +1541,34 @@ 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:10:51:10:58 | filePath |
1545+
| handlebars.js:10:51:10:58 | filePath |
1546+
| handlebars.js:10:51:10:58 | filePath |
1547+
| handlebars.js:10:51:10:58 | filePath |
1548+
| handlebars.js:11:32:11:39 | filePath |
1549+
| handlebars.js:11:32:11:39 | filePath |
1550+
| handlebars.js:11:32:11:39 | filePath |
1551+
| handlebars.js:11:32:11:39 | filePath |
1552+
| handlebars.js:11:32:11:39 | filePath |
1553+
| handlebars.js:13:73:13:80 | filePath |
1554+
| handlebars.js:13:73:13:80 | filePath |
1555+
| handlebars.js:13:73:13:80 | filePath |
1556+
| handlebars.js:13:73:13:80 | filePath |
1557+
| handlebars.js:15:25:15:32 | filePath |
1558+
| handlebars.js:15:25:15:32 | filePath |
1559+
| handlebars.js:15:25:15:32 | filePath |
1560+
| handlebars.js:15:25:15:32 | filePath |
1561+
| handlebars.js:15:25:15:32 | filePath |
1562+
| handlebars.js:29:46:29:60 | req.params.path |
1563+
| handlebars.js:29:46:29:60 | req.params.path |
1564+
| handlebars.js:29:46:29:60 | req.params.path |
1565+
| handlebars.js:29:46:29:60 | req.params.path |
1566+
| handlebars.js:29:46:29:60 | req.params.path |
1567+
| handlebars.js:43:15:43:29 | req.params.path |
1568+
| handlebars.js:43:15:43:29 | req.params.path |
1569+
| handlebars.js:43:15:43:29 | req.params.path |
1570+
| handlebars.js:43:15:43:29 | req.params.path |
1571+
| handlebars.js:43:15:43:29 | req.params.path |
15441572
| normalizedPaths.js:11:7:11:27 | path |
15451573
| normalizedPaths.js:11:7:11:27 | path |
15461574
| normalizedPaths.js:11:7:11:27 | path |
@@ -6414,6 +6442,38 @@ edges
64146442
| TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
64156443
| TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
64166444
| express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
6445+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6446+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6447+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6448+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6449+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6450+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6451+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6452+
| handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
6453+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6454+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6455+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6456+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6457+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6458+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6459+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6460+
| handlebars.js:13:73:13:80 | filePath | handlebars.js:15:25:15:32 | filePath |
6461+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6462+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6463+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6464+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6465+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6466+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6467+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6468+
| handlebars.js:29:46:29:60 | req.params.path | handlebars.js:10:51:10:58 | filePath |
6469+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6470+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6471+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6472+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6473+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6474+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6475+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
6476+
| handlebars.js:43:15:43:29 | req.params.path | handlebars.js:13:73:13:80 | filePath |
64176477
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
64186478
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
64196479
| normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path |
@@ -9844,6 +9904,8 @@ edges
98449904
| 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 |
98459905
| 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 |
98469906
| 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 |
9907+
| handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on $@. | handlebars.js:29:46:29:60 | req.params.path | a user-provided value |
9908+
| handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on $@. | handlebars.js:43:15:43:29 | req.params.path | a user-provided value |
98479909
| 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 |
98489910
| 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 |
98499911
| 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: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
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+
hb.registerHelper("catFile", function catFile(filePath) {
11+
return fs.readFileSync(filePath); // SINK (reads file)
12+
});
13+
hb.registerHelper("prependToLines", function prependToLines(prefix, filePath) {
14+
return fs
15+
.readFileSync(filePath)
16+
.split("\n")
17+
.map((line) => prefix + line)
18+
.join("\n");
19+
});
20+
data.compiledFileAccess = hb.compile("contents of file {{path}} are: {{catFile path}}")
21+
data.compiledBenign = hb.compile("hello, {{name}}");
22+
data.compiledUnknown = hb.compile(fs.readFileSync("greeting.template"));
23+
data.compiledMixed = hb.compile("helpers may have several args, like here: {{prependToLines prefix path}}");
24+
}
25+
26+
init();
27+
28+
app.get('/some/path1', function (req, res) {
29+
res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
30+
});
31+
32+
app.get('/some/path2', function (req, res) {
33+
res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
34+
});
35+
36+
app.get('/some/path3', function (req, res) {
37+
res.send(data.compiledUnknown({ name: req.params.name })); // ALLOWED (could be using a vulnerable helper, but we'll assume it's ok)
38+
});
39+
40+
app.get('/some/path4', function (req, res) {
41+
res.send(data.compiledMixed({
42+
prefix: ">>> ",
43+
path: req.params.path // NOT ALLOWED (template uses vulnerable helper)
44+
}));
45+
});
46+
47+
app.get('/some/path5', function (req, res) {
48+
res.send(data.compiledMixed({
49+
prefix: req.params.prefix, // ALLOWED (this parameter is safe)
50+
path: "data/path-5.txt"
51+
}));
52+
});

0 commit comments

Comments
 (0)