mde / ejs

Embedded JavaScript templates -- http://ejs.co
Apache License 2.0
7.72k stars 846 forks source link

vuln #571

Closed wadahkode closed 3 years ago

wadahkode commented 3 years ago

how to fix?

let ejs = require('ejs') ejs.render('./views/test.ejs',{ filename:'/etc/passwd\nfinally { this.global.process.mainModule.require(\'child_process\').execSync(\'touch EJS_HACKED\') }', compileDebug: true, message: 'test', client: true })

mde commented 3 years ago

This is not a problem with EJS.

EJS literally evaluates the JavaScript you pass it. It's what EJS is. EJS should never be used server-side with tainted/unsanitized input from an end-user. If you allow people to pass arbitrary code to it, it will run arbitrary code.

If a developer uses EJS this way, they are definitively using EJS wrong.

dominykas commented 3 years ago

While I agree with the sentiment that this is not exactly a security vulnerability in ejs as it is true that you should not be passing unsanitized user input to it, there still is a bug somewhere in there. Note the code - for whatever reason, the code inside options.filename string gets executed.

This only happens if options.compileDebug is set to true, which leads to me believe that there is a bug, likely here: https://github.com/mde/ejs/blob/9f69c0a1766dffd5e99db4931d45bfb90ab30e5c/lib/ejs.js#L639

Namely, if the filename contains a new line, then the second line escapes from the comment.

Not sure what the fix should be. A naive implementation should forbid \n in the filename, but probably this needs URI encoding or smth like that - not sure what the valid values are for this sourceURL comment, or how to escape these values exactly.

mde commented 3 years ago

Updated to prevent this in v3.1.6, now pushed to NPM.