Mitigate prototype pollution effects

[nicdumz]

I ran into #451 over the weekend. outputFunctionName is a somewhat fairly common way to get Remote Code Execution from a preexisting prototype pollution vulnerability in another library. (https://www.google.com/search?q=outputFunctionName+ctf has 1500+ hits). This hints to me that people might actually rely on this in the wild fairly often.

It is true that it would be preferrable for all libraries (or the language itself) to completely avoid/prevent Prototype Pollution so most people do not have to think of effects. At the same time the reality is that at any given time there are always a few live vulnerabilities. As a popular node it's a good idea for ejs to take a few extra actions to prevent widespread pollution->RCE escalation.

Hope this makes sense. I'm definitely flexible in how we should address this problem, e.g. we can absolutely tweak the contents of this PR as long as we achieve reasonable mitigation at the end of the day.

Cheers!

[mde]

The desire to increase security is laudable. However, this is not 'remote code execution,' or a vulnerability, in that you should never, ever, ever pass unsanitized input to EJS render. I don't believe I can be any clearer about this: you should think of EJS as essentially a JavaScript runtime, and be scrupulous about what you pass to it. Also worth noting that EJS is expected to run in very old runtimes, which may not have Object.create.

[nicdumz]

No one is willingly passing render(..., opts = {outputFunctionName: "x;process.mainModule.require('child_process').exec('touch 1');x"}). It is obviously a bad idea to pass unsanitized options and nobody is questioning that. On that you are correct. I would be on your side here: "you're holding it wrong" it opts is user-input controlled and your library does not need to care.

However this is not the use case I'm addressing. The problem is finding ways to mitigate the effect of Prototype Pollution.

So the starting hypothesis is that somewhere else in the code (not in EJS) there's an existing Prototype Pollution vulnerability, allowing attacker to set Object.prototype.outputFunctionName = ... (or the many variants to do this).

The way things stand, any server using EJS as a rendering engine is now basically known to have this documented escalation path. For instance https://blog.p6.is/Web-Security-CheatSheet/#javascript lists prototype-pollution to RCE as a very common recipe to get to RCE if the server is using EJS.

Prototype pollution is hard to completely eradicate, and there are still open vulnerabilities out there. What's happening in this context is that EJS + any other vulnerable library / piece of code automatically gives attackers remote code execution.

You're correct, completely eradicating Prototype Pollution would be ideal. But it's hard. So mitigating its effect in popular libraries is worth the work.

I want to be sure to explain things properly: this escalation is available even if the server code is never explicitly passing opts. For instance:

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

is vulnerable, regardless of the index.ejs contents.

EJS is expected to run in very old runtimes, which may not have Object.create.

The commit containing Object.create could be pruned, I don't overly care. What about the first commit, is there any reason to allow arbitrary names through? That commit alone would prevent escalating Object prototype vulnerabilities into an RCE.

[mde]

Sorry to be so abrupt. I am just constantly getting pings from security 'experts' badgering me to 'fix vulnerabilities' that are nothing of the sort. I understand now that this is more about being a good citizen of the ecosystem, and I'm all for that. If you'll prune the commit with the Object.create, or alternatively, write a wrapper function that is a no-op in runtimes that don't support it, I'll happily merge this. Thanks for your patience with my impatience. :)

[nicdumz]

Not a problem, I understand.

Would you be open to something like require('core-js/features/object/create') to get the missing Object.create?

[mde]

Depends on how it's implemented. If it adds an enumerable static on Object, that would be a problem. Simpler would be something like this, on source code eval:

var createObj = typeof Object.create == 'function' ?  function () { return Object.create(null); } : function () { return {} };

[nicdumz]

Your suggestion would not mitigate things on older runtimes, but... could be an OK tradeoff. Let me know what you think of the current implementation suggestion.

[mde]

I'm not sure what the core-js implementation looks like — any idea what it does? AFAIK null proto is not even possible in older runtimes. (And __proto__ doesn't exist either.) That's why my shim just does no-op pass-through. Also would prefer it not create an enumerable static on the Object built-in. Very old runtimes are less common, and present a target with a much smaller surface area to attack.

[mde]

Wait, I guess statics are enumerable anyhow. :) Not sure what I was thinking there. I think I still have PTSD from the old Prototype.js lib, which added a bunch of methods by fiat to the Object proto. :)

[mde]

I seem to remember Crockford had some sort of implementation, and yeah, here it is: https://www.crockford.com/javascript/prototypal.html You set the .prototype on the constructor to null.

[mde]

This might be easiest:

if (typeof Object.create !== 'function') {
    Object.create = function (o) {
        function F() {}
        F.prototype = o;
        return new F();
    };
}

[nicdumz]

Implementation was at https://github.com/zloirock/core-js/blob/b6bec027ef1ee337e34fb5d4ca6bd650ae47b4a3/packages/core-js/internals/object-create.js#L68 if you're curious.

Anyhow, made createObj a simple wrapper now.

[mde]

Excellent, thank you for doing this. Much appreciated.

[ExE-Boss]

new F() doesn’t support null prototypes.

---

A null‑supporting Object.create polyfill created by me can be seen in the proxy‑polyfill repository:

const $Object = Object;

// Closure assumes that `{__proto__: null} instanceof Object` is always true, hence why we check against a different name.
const canCreateNullProtoObjects = Boolean($Object.create) || !({ __proto__: null } instanceof $Object);
const objectCreate =
	$Object.create ||
	(canCreateNullProtoObjects
		? function create(proto) {
			validateProto(proto);
			return { __proto__: proto };
		}
		: function create(proto) {
			validateProto(proto);
			if (proto === null) {
				throw new SyntaxError('Native Object.create is required to create objects with null prototype');
			}

			// nb. cast to convince Closure compiler that this is a constructor
			var T = /** @type {!Function} */ (function T() {});
			T.prototype = proto;
			return new T();
		});

[mde]

What happens in old runtimes in the case where the ctor's prototype property is set to null? Is it just a no-op? Dos your impl throw if it can't set the proto to null? Maybe we'd be better off with simple pass-through in that case.

[mde]

Thanks a ton for chiming in on this!

[ExE-Boss]

What happens in old runtimes in the case where the ctor's prototype property is set to null? Is it just a no-op? Dos your impl throw if it can't set the proto to null?

It creates an object with its prototype set to %Object.prototype% of the realm that T comes from:

• https://www.ecma-international.org/wp-content/uploads/ECMA-262_3rd_edition_december_1999.pdf#sec-13.2.2
• https://tc39.es/ecma262/#sec-ecmascript-function-objects-construct-argumentslist-newtarget

[mde]

So effectively a no-op. Well, damn. We were better off with the simple pass-through. Although I guess there's the middle case where __proto__ is accessible. Since I've merged this, I'll just modify our createObj.

[mde]

Thanks again for chiming in, much appreciated!

[mde]

@ExE-Boss, can you take a quick look here? #603