Code execution after prototype pollution about dot HOT 15 CLOSED

Thanks for fixing the issue @epoberezkin, The Snyk advisory that was published will be updated later today https://snyk.io/vuln/SNYK-JS-DOT-174124

from dot.

I was planning on submitting the PR, but happy to see it fixed!! Thanks :)

from dot.

I'm sorry, I'm trying really hard to understand the underlying security issue, but I can't wrap my head around it.

Shouldn't both the attacker's code (Object.prototype.templateSettings = {varname:"x=alert(1+1)"}) and doT reside in the same context?
I mean, how is that:

/*elsewhere*/
Object.prototype.templateSettings = {varname:"x=alert(1+1)"};
/* in app code */
const templates = require("dot").process({path: "./my_trusted_templates_that_I_wrote"});
templates.mytemplate(data); // Executes code specified by prototype

Different from that?

x=alert(1+1)

I feel like I'm missing something here.

from dot.

Oh, sorry, I could've been more explicit about that. This is a way to combine two classes of security exploits each of which would be less serious on their own:

First, a prototype pollution vulnerability, in which some vulnerable code allows data to be added to a prototype. For example, https://snyk.io/vuln/npm:lodash:20180130

Then, a vulnerability like this one, which takes "data" (i.e. a string value) on a prototype (i.e., not directly specified by the calling code) and executes it.

from dot.

doT templates should be considered code, as some parts of them are indeed executed - therefore user input cannot be used to construct templates - it can only be passed to compiled template function.

This is by design and it does not make doT any more vulnerable than node.js or any other system that executes code.

from dot.

@fabiocbinbutter the fact that any property access lookups down the prototype chain is clear, it's how JS works. As @zlumer wrote though if the attacker can execute code to pollute prototype, it means they can execute any code, period - why bother delegating it to doT or any other library that happens to access any property (and you can also use compromised getters btw in this case, no need to do it via executed templates)?

Prototypes are context specific though, so if an isolated context (a window or node.js process) pollutes prototype it won't affect other contexts.

Also with doT it is recommended to run .process as part of the build step, not in runtime.

from dot.

if the attacker can execute code to pollute prototype, it means they can execute any code, period

That's not true - there's a whole name for this kind of vulnerability, and it is "prototype pollution", not "code execution". I shared one example earlier, to a snyk report for lodash:

And you can't use compromised getters in the same way because you would have to somehow make it a function. doT is the thing that takes the string in varname and makes it a function/code

from dot.

Interesting - missed that. From what you are writing just parsing could be enough? I will test.

Either way, doT indeed converts strings to code, in the same way node.js does as I wrote in the first comment. That’s why it is strongly recommended pre-compiling templates to code files at build time.

From what you are saying though, the fact that prototype of the configuration parameters also may be converted to code elevates this risk - even if templates themselves are safe, in case some developer violates this recommendation and uses dot as prod rather than as dev dependency, it may indeed be used for code injection at runtime via api payload.

I believe that given that majority of developers use some library for IO, rejecting proto property could/should be done on the level of those libraries. And JS itself could patch it for built in IO/parse methods - is it filed as an issue for browsers/DOM/node spec? Expecting that every single JS library should independently patch this vulnerability seems wrong. It can also be used by supplying invalid data or manipulating otherwise private data by defining their default properties via prototype - code injection is just one scenario.

from dot.

And yes, I would appreciate PR to fix it, with a test that would fail before the change (to show proto injection works) and pass after. I will still add docs to comment on doT security model.

from dot.

@fabiocbinbutter I was not able to reproduce prototype pollution without direct assignment to object prototype. The code in the picture didn't work as described in node 12. Possibly, this was patched. Did you try it yourself?

from dot.

I tried with Object.assign and with _.merge (lodash) - prototype did not change.

from dot.

Yes, the article says it's lodash issue affecting versions < 4.17.5, so I am not quite sure why this issue should be addressed in any other library - it's a lodash issue.

from dot.

Also your code would execute the injected code only if data passed to the templated function is undefined.

from dot.

I think it was more a theoretic possibility as in order for injected code to be executed you had to call template function without parameter (or pass undefined). But anyway - it’s patched now. Btw, npm security advisory about doT is now unpublished as well thanks to their helpful team.

from dot.

thank you @snoopysecurity - as you can see from the note I added to readme I am quite fond of this little library :) - exonerating it is much appreciated :)

from dot.