Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mitigate prototype pollution effects #601

Merged
merged 2 commits into from May 31, 2021
Merged

Mitigate prototype pollution effects #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15ee698
Sanitize option names.
nicdumz May 30, 2021
be9a9bb
Create Objects without prototypes.
nicdumz May 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 31 additions & 8 deletions lib/ejs.js
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@
* @public
*/


var fs = require('fs');
var path = require('path');
var utils = require('./utils');
Expand All @@ -64,6 +65,18 @@ var _OPTS_PASSABLE_WITH_DATA = ['delimiter', 'scope', 'context', 'debug', 'compi
// so we make an exception for `renderFile`
var _OPTS_PASSABLE_WITH_DATA_EXPRESS = _OPTS_PASSABLE_WITH_DATA.concat('cache');
var _BOM = /^\uFEFF/;
var _JS_IDENTIFIER = /^[a-zA-Z_$][0-9a-zA-Z_$]*$/;

var createObj = function() {
if (typeof Object.create !== 'function') {
return function (o) {
function F() {}
F.prototype = o;
return new F();
};
}
return Object.create;
}();
Comment on lines +70 to +79
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
		});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton for chiming in on this!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for chiming in, much appreciated!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* EJS template function cache. This can be a LRU object from lru-cache NPM
Expand Down Expand Up @@ -305,7 +318,7 @@ function fileLoader(filePath){
*/

function includeFile(path, options) {
var opts = utils.shallowCopy({}, options);
var opts = utils.shallowCopy(createObj(null), options);
opts.filename = getIncludePath(path, opts);
if (typeof options.includer === 'function') {
var includerResult = options.includer(path, opts.filename);
Expand Down Expand Up @@ -411,8 +424,8 @@ exports.compile = function compile(template, opts) {
*/

exports.render = function (template, d, o) {
var data = d || {};
var opts = o || {};
var data = d || createObj(null);
var opts = o || createObj(null);

// No options object -- if there are optiony names
// in the data, copy them to options
Expand Down Expand Up @@ -483,7 +496,7 @@ exports.renderFile = function () {
opts.filename = filename;
}
else {
data = {};
data = createObj(null);
}

return tryHandleCache(opts, data, cb);
Expand All @@ -505,8 +518,8 @@ exports.clearCache = function () {
};

function Template(text, opts) {
opts = opts || {};
var options = {};
opts = opts || createObj(null);
var options = createObj(null);
this.templateText = text;
/** @type {string | null} */
this.mode = null;
Expand Down Expand Up @@ -587,12 +600,21 @@ Template.prototype = {
' var __output = "";\n' +
' function __append(s) { if (s !== undefined && s !== null) __output += s }\n';
if (opts.outputFunctionName) {
if (!_JS_IDENTIFIER.test(opts.outputFunctionName)) {
throw new Error('outputFunctionName is not a valid JS identifier.');
}
prepended += ' var ' + opts.outputFunctionName + ' = __append;' + '\n';
}
if (opts.localsName && !_JS_IDENTIFIER.test(opts.localsName)) {
throw new Error('localsName is not a valid JS identifier.');
}
if (opts.destructuredLocals && opts.destructuredLocals.length) {
var destructuring = ' var __locals = (' + opts.localsName + ' || {}),\n';
for (var i = 0; i < opts.destructuredLocals.length; i++) {
var name = opts.destructuredLocals[i];
if (!_JS_IDENTIFIER.test(name)) {
throw new Error(`destructuredLocals[${i}] is not a valid JS identifier.`);
}
if (i > 0) {
destructuring += ',\n ';
}
Expand Down Expand Up @@ -683,13 +705,14 @@ Template.prototype = {
// Adds a local `include` function which allows full recursive include
var returnedFn = opts.client ? fn : function anonymous(data) {
var include = function (path, includeData) {
var d = utils.shallowCopy({}, data);
var d = utils.shallowCopy(createObj(null), data);
if (includeData) {
d = utils.shallowCopy(d, includeData);
}
return includeFile(path, opts)(d);
};
return fn.apply(opts.context, [data || {}, escapeFn, include, rethrow]);
return fn.apply(opts.context,
[data || createObj(null), escapeFn, include, rethrow]);
};
if (opts.filename && typeof Object.defineProperty === 'function') {
var filename = opts.filename;
Expand Down
26 changes: 26 additions & 0 deletions test/ejs.js
View file
Open in desktop
Expand Up @@ -1178,3 +1178,29 @@ suite('meta information', function () {
assert.strictEqual(ejs.name, 'ejs');
});
});

suite('identifier validation', function () {
test('invalid outputFunctionName', function() {
assert.throws(function() {
ejs.compile('<p>yay</p>', {outputFunctionName: 'x;console.log(1);x'});
}, /outputFunctionName is not a valid JS identifier/)
});

test('invalid localsName', function() {
var locals = Object.create(null);
assert.throws(function() {
ejs.compile('<p>yay</p>', {
localsName: 'function(){console.log(1);return locals;}()'});
}, /localsName is not a valid JS identifier/)
});

test('invalid destructuredLocals', function() {
var locals = {};
assert.throws(function() {
ejs.compile('<p>yay</p>', {
destructuredLocals: [
'console.log(1); //'
]});
}, /destructuredLocals\[0\] is not a valid JS identifier/)
});
});