[Vulnerability] Server side template injection leads to RCE

[netcode]

Hey team, Thank you for this awesome project.

I found a template injection leads to Remote code execution. Using this vulnerability an attacker can update the value of outputFunctionName to inject a JS code which will be executed with the template compiled function.

I didn't post the details here, waiting for your confirmation on the right place to report the full details of this vulnerability.

Thanks again, waiting for your response.

[mde]

If you are giving end-users unfettered access to the render method, then you are using EJS in an inherently non-secure way. EJS is not meant to be used in this way.

EJS is effectively a JavaScript runtime. That's what it does — execute JavaScript. Reporting this type of issue in EJS would be like reporting something similar in bash.

[netcode]

Thanks, @mde for the response.

template engines should have some safeguards against untrusted inputs (you already implemented some countermeasures to reduce the risk of unvalidated inputs (eg. escape* functions in utils)., you can't expect all the inputs used in the template to be static 🤷🏻‍♂️

I'm talking about vulnerable code like this

const express = require('express')
const app = express()
app.set('view engine', 'ejs');

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

If you allow me I can put the full attack payload and clarify what happens.

[mde]

Thank you for this example. It is precisely how you should NOT be using EJS. You should never, ever run the render method with untrusted inputs — any more than you should be passing raw user-input to be used in raw SQL calls to your DB.

This doesn't imply the inputs have to be static. It implies that the app-developer creating userland code should be escaping or otherwise checking those user inputs, before handing them to what is, as I have noted, is in fact, a JavaScript runtime.

This is not a templating library that does simple parameterized variable substitution. It's "simply JavaScript," in every sense of those words. You get all the power of running full-blown JavaScript in your templates, which means YOU as the developer have the responsibility of securing your code.

Early on, I indulged these 'vulnerability' reports, because I imagined I was being a good community citizen, and indeed, some of them involved making sure the old API of passing options along with the data was safe to use in earlier versions of Express. That was actually worth doing.

But it is not possible to secure EJS from unfettered access to the render method. It's a fool's errand, and it gives the consumers of the library a false sense of security. You, as a user of the library, are responsible for securing the inputs you handle to render. Full stop.

[netcode]

Thank you for your detailed response. While I'm not aligned with what you said but every library is free to declare how it should be used and what is risky and what is not.

Also, I found that there is a fix already to this kind of issue created by @nicdumz here: 15ee698 - so I highly recommend just create a release and that's it.

To avoid this in the future I've updated my PR here #664 to clarify that in the readme.md & in security.md

[netcode]

@mde are you planning to do a release anytime soon?
I believe the validation implemented in the main branch is good. You can do a release (the last ejs release was > 1 year ago).

Also if you want to avoid further security reports targeting the render options, the best way is to highlight that in the readme.md to ensure users are not using ejs the wrong way. and security.md to make it clear for security researchers.

[mde]

@netcode You are 100% correct about that. We should do a much better job of setting expectations in the docs. I am totally underwater with work this week, but I am hoping to have some time this weekend to push out a release. Maybe I can add some caveating to the docs at the same time. Thanks so much for your understanding!

[kokushkin]

So, what is the outcome of this conversation?

Option 1: The documentation was mostly clear. So, update the docs. Maybe, apply the fix... just in case. Delete the vulnerability report so it doesn't show up with npm audit. (if the report is still there, it creates additional noise and it's harder to see what's really matter)

Option 2: The documentation was misleading so people are likely to be affected by this vulnerability. Then, fix the vulnerability. Because it's gonna be a major update of the documentation (read interface of the library) , create a new major version where the docs will be changed. (if major version isn't bumped - nobody notices and continues to use the package according to the old docs)

It looks to me that you've chosen Option 1. So @netcode maybe you delete(/decrease the severity level) this report so I don't see 3 critical errors (somehow it's getting multiplied) in my console ?