Skip to content

Commit 28951e9

Browse files
committed
add engine filter to js/template-object-injection
1 parent b30484d commit 28951e9

File tree

3 files changed

+150
-2
lines changed

3 files changed

+150
-2
lines changed

javascript/ql/src/semmle/javascript/security/dataflow/TemplateObjectInjectionCustomizations.qll

Lines changed: 78 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,83 @@ module TemplateObjectInjection {
4242
override DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
4343
}
4444

45-
private class TemplateSink extends Sink {
46-
TemplateSink() { this.asExpr() instanceof Express::TemplateObjectInput }
45+
/**
46+
* An argument given to the `render` method on an Express response object,
47+
* where the view engine used by the Express instance is vulnerable to template object injection.
48+
*/
49+
private class TemplateSink extends Sink, Express::TemplateObjectInput {
50+
TemplateSink() {
51+
exists(
52+
Express::RouteSetup setup, Express::RouterDefinition router, Express::RouterDefinition top
53+
|
54+
setup.getARouteHandler() = getRouteHandler() and
55+
setup.getRouter() = router and
56+
usesVulnerableTemplateEngine(router)
57+
)
58+
}
59+
}
60+
61+
/**
62+
* Gets the package name for a templating library that is vulnerable to template object injection.
63+
*/
64+
private string getAVulnerableTemplateEngine() {
65+
result =
66+
[
67+
"eta", "squirrelly", "haml-coffee", "express-hbs", "ejs", "hbs", "whiskers",
68+
"express-handlebars"
69+
]
70+
}
71+
72+
/**
73+
* Holds if the "view engine" of `router` is set to a vulnerable templating engine.
74+
*/
75+
predicate usesVulnerableTemplateEngine(Express::RouterDefinition router) {
76+
// option 1: `app.set("view engine", "theEngine")`.
77+
// Express will load the engine automatically.
78+
exists(MethodCallExpr call |
79+
router.flowsTo(call.getReceiver()) and
80+
call.getMethodName() = "set" and
81+
call.getArgument(0).getStringValue() = "view engine" and
82+
call.getArgument(1).getStringValue() = getAVulnerableTemplateEngine()
83+
)
84+
or
85+
// option 2: setup an engine.
86+
// ```app.engine("name", engine); app.set("view engine", "name");```
87+
// ^^^ `registerCall` ^^^ ^^^ `viewEngineCall` ^^^
88+
exists(
89+
DataFlow::MethodCallNode registerCall, DataFlow::SourceNode engine,
90+
DataFlow::MethodCallNode viewEngineCall
91+
|
92+
// `app.engine("name", engine)
93+
router.flowsTo(registerCall.getReceiver().asExpr()) and
94+
registerCall.getMethodName() = ["engine", "register"] and
95+
engine = registerCall.getArgument(1).getALocalSource() and
96+
// app.set("view engine", "name")
97+
router.flowsTo(viewEngineCall.getReceiver().asExpr()) and
98+
viewEngineCall.getMethodName() = "set" and
99+
viewEngineCall.getArgument(0).getStringValue() = "view engine" and
100+
// The name set by the `app.engine("name")` call matches `app.set("view engine", "name")`.
101+
viewEngineCall.getArgument(1).getStringValue() = registerCall.getArgument(0).getStringValue()
102+
|
103+
// Different ways of initializing vulnerable template engines.
104+
engine = DataFlow::moduleImport(getAVulnerableTemplateEngine())
105+
or
106+
engine = DataFlow::moduleImport(getAVulnerableTemplateEngine()).getAPropertyRead("__express")
107+
or
108+
engine = DataFlow::moduleImport("express-handlebars").getACall()
109+
or
110+
engine = DataFlow::moduleImport("express-handlebars").getAPropertyRead("engine")
111+
or
112+
exists(DataFlow::SourceNode hbs |
113+
hbs = DataFlow::moduleImport("express-hbs")
114+
or
115+
hbs = DataFlow::moduleImport("express-hbs").getAMemberCall("create")
116+
|
117+
engine = hbs.getAMemberCall(["express3", "express4"])
118+
)
119+
or
120+
engine =
121+
DataFlow::moduleImport("consolidate").getAPropertyRead(getAVulnerableTemplateEngine())
122+
)
47123
}
48124
}

javascript/ql/test/query-tests/Security/CWE-073/TemplateObjectInjection.expected

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,22 @@
11
nodes
2+
| tst2.js:6:9:6:46 | bodyParameter |
3+
| tst2.js:6:25:6:32 | req.body |
4+
| tst2.js:6:25:6:32 | req.body |
5+
| tst2.js:6:25:6:46 | req.bod ... rameter |
6+
| tst2.js:7:28:7:40 | bodyParameter |
7+
| tst2.js:7:28:7:40 | bodyParameter |
8+
| tst2.js:26:9:26:46 | bodyParameter |
9+
| tst2.js:26:25:26:32 | req.body |
10+
| tst2.js:26:25:26:32 | req.body |
11+
| tst2.js:26:25:26:46 | req.bod ... rameter |
12+
| tst2.js:27:28:27:40 | bodyParameter |
13+
| tst2.js:27:28:27:40 | bodyParameter |
14+
| tst2.js:34:9:34:46 | bodyParameter |
15+
| tst2.js:34:25:34:32 | req.body |
16+
| tst2.js:34:25:34:32 | req.body |
17+
| tst2.js:34:25:34:46 | req.bod ... rameter |
18+
| tst2.js:35:28:35:40 | bodyParameter |
19+
| tst2.js:35:28:35:40 | bodyParameter |
220
| tst.js:5:9:5:46 | bodyParameter |
321
| tst.js:5:25:5:32 | req.body |
422
| tst.js:5:25:5:32 | req.body |
@@ -25,6 +43,21 @@ nodes
2543
| tst.js:27:28:27:42 | JSON.parse(str) |
2644
| tst.js:27:39:27:41 | str |
2745
edges
46+
| tst2.js:6:9:6:46 | bodyParameter | tst2.js:7:28:7:40 | bodyParameter |
47+
| tst2.js:6:9:6:46 | bodyParameter | tst2.js:7:28:7:40 | bodyParameter |
48+
| tst2.js:6:25:6:32 | req.body | tst2.js:6:25:6:46 | req.bod ... rameter |
49+
| tst2.js:6:25:6:32 | req.body | tst2.js:6:25:6:46 | req.bod ... rameter |
50+
| tst2.js:6:25:6:46 | req.bod ... rameter | tst2.js:6:9:6:46 | bodyParameter |
51+
| tst2.js:26:9:26:46 | bodyParameter | tst2.js:27:28:27:40 | bodyParameter |
52+
| tst2.js:26:9:26:46 | bodyParameter | tst2.js:27:28:27:40 | bodyParameter |
53+
| tst2.js:26:25:26:32 | req.body | tst2.js:26:25:26:46 | req.bod ... rameter |
54+
| tst2.js:26:25:26:32 | req.body | tst2.js:26:25:26:46 | req.bod ... rameter |
55+
| tst2.js:26:25:26:46 | req.bod ... rameter | tst2.js:26:9:26:46 | bodyParameter |
56+
| tst2.js:34:9:34:46 | bodyParameter | tst2.js:35:28:35:40 | bodyParameter |
57+
| tst2.js:34:9:34:46 | bodyParameter | tst2.js:35:28:35:40 | bodyParameter |
58+
| tst2.js:34:25:34:32 | req.body | tst2.js:34:25:34:46 | req.bod ... rameter |
59+
| tst2.js:34:25:34:32 | req.body | tst2.js:34:25:34:46 | req.bod ... rameter |
60+
| tst2.js:34:25:34:46 | req.bod ... rameter | tst2.js:34:9:34:46 | bodyParameter |
2861
| tst.js:5:9:5:46 | bodyParameter | tst.js:8:28:8:40 | bodyParameter |
2962
| tst.js:5:9:5:46 | bodyParameter | tst.js:8:28:8:40 | bodyParameter |
3063
| tst.js:5:25:5:32 | req.body | tst.js:5:25:5:46 | req.bod ... rameter |
@@ -49,6 +82,9 @@ edges
4982
| tst.js:27:39:27:41 | str | tst.js:27:28:27:42 | JSON.parse(str) |
5083
| tst.js:27:39:27:41 | str | tst.js:27:28:27:42 | JSON.parse(str) |
5184
#select
85+
| tst2.js:7:28:7:40 | bodyParameter | tst2.js:6:25:6:32 | req.body | tst2.js:7:28:7:40 | bodyParameter | Template object injection due to $@. | tst2.js:6:25:6:32 | req.body | user-provided value |
86+
| tst2.js:27:28:27:40 | bodyParameter | tst2.js:26:25:26:32 | req.body | tst2.js:27:28:27:40 | bodyParameter | Template object injection due to $@. | tst2.js:26:25:26:32 | req.body | user-provided value |
87+
| tst2.js:35:28:35:40 | bodyParameter | tst2.js:34:25:34:32 | req.body | tst2.js:35:28:35:40 | bodyParameter | Template object injection due to $@. | tst2.js:34:25:34:32 | req.body | user-provided value |
5288
| tst.js:8:28:8:40 | bodyParameter | tst.js:5:25:5:32 | req.body | tst.js:8:28:8:40 | bodyParameter | Template object injection due to $@. | tst.js:5:25:5:32 | req.body | user-provided value |
5389
| tst.js:9:28:9:41 | queryParameter | tst.js:6:26:6:49 | req.que ... rameter | tst.js:9:28:9:41 | queryParameter | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |
5490
| tst.js:22:28:22:30 | obj | tst.js:6:26:6:49 | req.que ... rameter | tst.js:22:28:22:30 | obj | Template object injection due to $@. | tst.js:6:26:6:49 | req.que ... rameter | user-provided value |
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
const handlebars = require("express-handlebars");
2+
var app = require('express')();
3+
app.engine( '.hbs', handlebars({ defaultLayout: 'main', extname: '.hbs' }) );
4+
app.set('view engine', '.hbs')
5+
app.post('/path', function(req, res) {
6+
var bodyParameter = req.body.bodyParameter;
7+
res.render('template', bodyParameter); // NOT OK
8+
});
9+
10+
var app2 = require('express')();
11+
app2.post('/path', function(req, res) {
12+
var bodyParameter = req.body.bodyParameter;
13+
res.render('template', bodyParameter); // OK
14+
});
15+
16+
var app3 = require('express')();
17+
app3.set('view engine', 'pug');
18+
app3.post('/path', function(req, res) {
19+
var bodyParameter = req.body.bodyParameter;
20+
res.render('template', bodyParameter); // OK
21+
});
22+
23+
var app4 = require('express')();
24+
app4.set('view engine', 'ejs');
25+
app4.post('/path', function(req, res) {
26+
var bodyParameter = req.body.bodyParameter;
27+
res.render('template', bodyParameter); // NOT OK
28+
});
29+
30+
var app5 = require('express')();
31+
app5.engine("foobar", require("consolidate").whiskers);
32+
app5.set('view engine', 'foobar');
33+
app5.post('/path', function(req, res) {
34+
var bodyParameter = req.body.bodyParameter;
35+
res.render('template', bodyParameter); // NOT OK
36+
});

0 commit comments

Comments
 (0)