Code execution after prototype pollution

[fabiocbinbutter]

Suggested continuation from #281 and as was originally described in "Part B" of https://hackerone.com/reports/390929

If you agree, I can submit the suggested fix as a PR

Description

First, note that with the addition of default function parameters in ES2015, it is easy to evaluate arbitrary JS expressions if you have access to arg in new Function(arg, body), for example new Function("x=alert(2+2)","return x")

Next, note that calling doT.process({}) without specifying templateSettings (which is the intended way of using the function when not attempting to override defaults) will allow a templateSettings property to be taken from o's prototype:

So, with a previously exploited prototype pollution vulnerability, doT can be caused to run arbitrary code stored as a string in that prototype:

/*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

### Suggested fix

On line 45, highlighted above, replace `... o.templateSettings ? ...` with `... Object.prototype.hasOwnProperty.call(o,"templateSettings") ? ...`


### Aside - similar but unaffected property
![image](https://user-images.githubusercontent.com/5790026/69501734-c68a5980-0ed5-11ea-81ab-ea70d264cb1a.png)
^ Here, a similar issue could be present with `Object.prototype.varname` but upon closer inspection, I don't think it's a significant issue since doT users are prevented from omitting the varname property in normal doT usage

[zlumer]

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.

[fabiocbinbutter]

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.

[epoberezkin]

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.

[epoberezkin]

@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.

[fabiocbinbutter]

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

[epoberezkin]

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.

[epoberezkin]

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.

[epoberezkin]

@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?

[epoberezkin]

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

[epoberezkin]

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.

[epoberezkin]

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

[fabiocbinbutter]

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

[epoberezkin]

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.

[snoopysecurity]

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

[epoberezkin]

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 :)