mde / ejs

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

Potential Security Issue #604

Closed JamieSlome closed 3 years ago

JamieSlome commented 3 years ago

Hello,

We recently received a vulnerability disclosure against your repository from @nicdumz. I couldn't find an e-mail to contact or a security process to follow, so created this issue instead.

If you would like me to e-mail over the details or put them on the GitHub Issue, I'm more than happy to facilitate this for you. Otherwise, you can access the advisory here.

It is private to you and the discloser of the report.

If you have any questions, let me know.

-- Jamie from huntr.dev

mde commented 3 years ago

I receive numerous messages about purported vulnerabilities in EJS that are, in fact, not vulnerabilities at all. EJS is effectively a JavaScript runtime, and the its render call should never, ever be called with unsanitized inputs, and every 'disclosure' I get depends on doing just that. I am closing this issue. Feel free to open a new issue if there is a real and verifiable problem with EJS.

nicdumz commented 3 years ago

But we've worked together through #601, merged it, and agreed there that mitigating the effects of (outside) prototype pollution in EJS would be worth it, right?

JamieSlome commented 3 years ago

As your intent is to fix the issue that @nicdumz has disclosed, you'll both be eligible to receive bounties via our platform.

If you have any questions, my door is open!

mde commented 3 years ago

If this is the same issue, then yes.

JamieSlome commented 3 years ago

Yep, exactly the issue!

Are you able to sign up, confirm the report and the fix, and the bounty is yours!

JamieSlome commented 3 years ago

Also, because we love that your GitHub ID is only three numbers long, we are giving you an exclusive badge on your profile.

Cheers! ❤️

nicdumz commented 3 years ago

@JamieSlome I'd generally appreciate a second look on the initial advisory and classification.

As you can see what's happening is that in the presence of prototype pollution in another library, the AST for EJS can be polluted and gets attackers RCE. "AST injection" is not yet an official CVE type so I've settled for Code Injection. Does that feel reasonable?

As a note, that vector is well-known so worth fixing, the actual severity / classification matters little to me :-)

JamieSlome commented 3 years ago

@nicdumz - we will await to hear thoughts from @mde.

Cheers!

mde commented 3 years ago

I'm not sure what y'all are asking me to weigh in on here. I think it made sense to merge this in the interest of being a good citizen of the ecosystem, but there's no EJS vulnerability here, and I get quite a number of similar "security issue reports" from self-described security experts, who fundamentally don't understand how EJS works.

nicdumz commented 3 years ago

We merged fixes in now, which is great.

Do you think you could cut a minor release out, which would contain those fixes?

For me my goal is essentially to get a patched version out, flag a minor vulnerability of sorts, and next leverage automatic package scanners (snyk, etc) so that they go around and force version bumps to get rid of that vector.

I understand how we could say that there is no "EJS vulnerability on itself". However EJS used to provide an easy, well-documented way to escalate prototype pollution into Remote Code Execution. Because of the number of users, this was a very yummy hop to target for attackers. Mitigating this pathway is worth it, and worth an advisory so that automated scanner can upgrade your users. Makes sense?