Skip to content

Commit 01c7957

Browse files
authored
feat: add code injection via template (#961)
1 parent 6fa7510 commit 01c7957

File tree

8 files changed

+221
-40
lines changed

8 files changed

+221
-40
lines changed

README.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,39 @@ Here are the exploitable vulnerable packages:
5959

6060
* Open Redirect
6161
* NoSQL Injection
62+
* Code Injection
6263
* Command execution
6364
* Cross-site Scripting (XSS)
6465
* Security misconfiguration exposes server information
6566
* Insecure protocol (HTTP) communication
6667

68+
#### Code injection
69+
70+
The page at `/account_details` is rendered as an Handlebars view.
71+
72+
The same view is used for both the GET request which shows the account details, as well as the form itself for a POST request which updates the account details. A so-called Server-side Rendering.
73+
74+
The form is completely functional. The way it works is, it receives the profile information from the `req.body` and passes it, as-is to the template. This however means, that the attacker is able to control a variable that flows directly from the request into the view template library.
75+
76+
You'd think that what's the worst that can happen because we use a validation to confirm the expected input, however the validation doesn't take into account a new field that can be added to the object, such as `layout`, which when passed to a template language, could lead to Local File Inclusion (Path Traversal) vulnerabilities. Here is a proof-of-concept showing it:
77+
78+
```sh
79+
curl -X 'POST' -H 'Content-Type: application/json' --data-binary $'{"layout": "./../package.json"}' 'http://localhost:3001/account_details'
80+
```
81+
82+
Actually, there's even another vulnerability in this code.
83+
The `validator` library that we use has several known regular expression denial of service vulnerabilities. One of them, is associated with the email regex, which if validated with the `{allow_display_name: true}` option then we can trigger a denial of service for this route:
84+
85+
```sh
86+
curl -X 'POST' -H 'Content-Type: application/json' --data-binary "{\"email\": \"`seq -s "" -f "<" 100000`\"}" 'http://localhost:3001/account_details'
87+
```
88+
89+
The `validator.rtrim()` sanitizer is also vulnerable, and we can use this to create a similar denial of service attack:
90+
91+
```sh
92+
curl -X 'POST' -H 'Content-Type: application/json' --data-binary "{\"email\": \"[email protected]\", \"country\": \"nop\", \"phone\": \"0501234123\", \"lastname\": \"nop\", \"firstname\": \"`node -e 'console.log(" ".repeat(100000) + "!")'`\"}" 'http://localhost:3001/account_details'
93+
```
94+
6795
#### Open redirect
6896

6997
The `/admin` view introduces a `redirectPage` query path, as follows in the admin view:

app.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ var fileUpload = require('express-fileupload');
2323
var dust = require('dustjs-linkedin');
2424
var dustHelpers = require('dustjs-helpers');
2525
var cons = require('consolidate');
26+
const hbs = require('hbs')
2627

2728
var app = express();
2829
var routes = require('./routes');
@@ -32,6 +33,7 @@ var routesUsers = require('./routes/users.js')
3233
app.set('port', process.env.PORT || 3001);
3334
app.engine('ejs', ejsEngine);
3435
app.engine('dust', cons.dust);
36+
app.engine('hbs', hbs.__express);
3537
cons.dust.helpers = dustHelpers;
3638
app.set('views', path.join(__dirname, 'views'));
3739
app.set('view engine', 'ejs');
@@ -48,7 +50,8 @@ app.get('/', routes.index);
4850
app.get('/login', routes.login);
4951
app.post('/login', routes.loginHandler);
5052
app.get('/admin', routes.admin);
51-
app.get('/account_details', routes.account_details);
53+
app.get('/account_details', routes.get_account_details);
54+
app.post('/account_details', routes.save_account_details);
5255
app.post('/create', routes.create);
5356
app.get('/destroy/:id', routes.destroy);
5457
app.get('/edit/:id', routes.edit);

package-lock.json

Lines changed: 85 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
"express": "4.12.4",
2828
"express-fileupload": "0.0.5",
2929
"file-type": "^8.1.0",
30+
"hbs": "^4.0.4",
3031
"humanize-ms": "1.0.1",
3132
"jquery": "^2.2.4",
3233
"lodash": "4.17.4",
@@ -43,7 +44,8 @@
4344
"st": "0.2.4",
4445
"stream-buffers": "^3.0.1",
4546
"tap": "^11.1.3",
46-
"typeorm": "^0.2.24"
47+
"typeorm": "^0.2.24",
48+
"validator": "^13.5.2"
4749
},
4850
"devDependencies": {
4951
"browserify": "^13.1.1",

routes/index.js

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ var streamBuffers = require('stream-buffers');
99
var readline = require('readline');
1010
var moment = require('moment');
1111
var exec = require('child_process').exec;
12+
var validator = require('validator');
1213

1314
// zip-slip
1415
var fileType = require('file-type');
@@ -59,12 +60,38 @@ exports.admin = function (req, res, next) {
5960
});
6061
};
6162

62-
exports.account_details = function (req, res, next) {
63-
return res.render('account_details', {
64-
title: 'Account details',
65-
granted: true,
66-
});
67-
};
63+
exports.get_account_details = function(req, res, next) {
64+
// @TODO need to add a database call to get the profile from the database
65+
// and provide it to the view to display
66+
const profile = {}
67+
return res.render('account.hbs', profile)
68+
}
69+
70+
exports.save_account_details = function(req, res, next) {
71+
// get the profile details from the JSON
72+
const profile = req.body
73+
// validate the input
74+
if (validator.isEmail(profile.email, { allow_display_name: true })
75+
// allow_display_name allows us to receive input as:
76+
// Display Name <email-address>
77+
// which we consider valid too
78+
&& validator.isMobilePhone(profile.phone, 'he-IL')
79+
&& validator.isAscii(profile.firstname)
80+
&& validator.isAscii(profile.lastname)
81+
&& validator.isAscii(profile.country)
82+
) {
83+
// trim any extra spaces on the right of the name
84+
profile.firstname = validator.rtrim(profile.firstname)
85+
profile.lastname = validator.rtrim(profile.lastname)
86+
87+
// render the view
88+
return res.render('account.hbs', profile)
89+
} else {
90+
// if input validation fails, we just render the view as is
91+
console.log('error in form details')
92+
return res.render('account.hbs')
93+
}
94+
}
6895

6996
function adminLoginSuccess(redirectPage, res) {
7097
console.log({redirectPage})

views/account.hbs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
2+
<style>
3+
strong {font-weight: bold}
4+
</style>
5+
{{#if firstname}}
6+
<h1 id="page-title">Account details for: {{firstname}}</h1>
7+
<center>
8+
<h3 style="color: green">details saved</h2>
9+
</center>
10+
{{else}}
11+
<h1 id="page-title" style="color: red">Account details missing</h1>
12+
{{/if}}
13+
14+
<div id="list">
15+
<form action="/account_details" method="POST" accept-charset="utf-8">
16+
<div class="item-new">
17+
<center>First name</center>
18+
<input class="input" type="text" name="firstname" value="{{firstname}}" />
19+
<br/>
20+
21+
<center>Last name</center>
22+
<input class="input" type="text" name="lastname" value="{{lastname}}" />
23+
<br/>
24+
25+
<center>Country</center>
26+
<input class="input" type="text" name="country" value="{{country}}" />
27+
<br/>
28+
29+
<center>Phone number</center>
30+
<input class="input" type="text" name="phone" value="{{phone}}" />
31+
<br/>
32+
33+
<center>Email</center>
34+
<input class="input" type="text" name="email" value="{{email}}" />
35+
<br/>
36+
37+
</div>
38+
39+
<br/>
40+
<br/>
41+
<button type="submit">Save account details</button>
42+
43+
</form>
44+
</div>

views/account_details.ejs

Lines changed: 0 additions & 32 deletions
This file was deleted.

views/layout.hbs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title><%= title %></title>
5+
<link rel='stylesheet' href='/public/css/screen.css' />
6+
<!--[if lt IE 9]>
7+
<script src="http://html5shiv.googlecode.com/svn/trunk/html5.js"></script>
8+
<![endif]-->
9+
</head>
10+
<body>
11+
<div id="layout">
12+
{{{body}}}
13+
<div id="layout-footer"></div>
14+
</div>
15+
<div id="footer-wrap">
16+
17+
<div id="footer">
18+
<center>
19+
<a href="/public/about.html">about</a>
20+
</center>
21+
</div>
22+
</div>
23+
</body>
24+
</html>

0 commit comments

Comments
 (0)