From 15ee698583c98dadc456639d6245580d17a24baf Mon Sep 17 00:00:00 2001 From: Nicolas Dumazet Date: Sun, 30 May 2021 07:00:58 +0200 Subject: [PATCH 1/2] Sanitize option names. This prevents injection of arbitrary code if the server is already vulnerable to prototype poisoning. This resolves #451. I deliberately opted to not support complex Unicode identifiers even though they're valid JS identifiers. They're complex to validate and users probably shouldn't even try to be that creative. --- lib/ejs.js | 10 ++++++++++ test/ejs.js | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/lib/ejs.js b/lib/ejs.js index aa6322e3..e46ece39 100755 --- a/lib/ejs.js +++ b/lib/ejs.js @@ -64,6 +64,7 @@ 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_$]*$/; /** * EJS template function cache. This can be a LRU object from lru-cache NPM @@ -587,12 +588,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 '; } diff --git a/test/ejs.js b/test/ejs.js index 1e99d420..a8d8a814 100755 --- a/test/ejs.js +++ b/test/ejs.js @@ -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('

yay

', {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('

yay

', { + 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('

yay

', { + destructuredLocals: [ + 'console.log(1); //' + ]}); + }, /destructuredLocals\[0\] is not a valid JS identifier/) + }); +}); \ No newline at end of file From be9a9bb397ed6f4f4e563eed4aa53700f30a837e Mon Sep 17 00:00:00 2001 From: Nicolas Dumazet Date: Sun, 30 May 2021 07:50:50 +0200 Subject: [PATCH 2/2] Create Objects without prototypes. This generally helps mitigate prototype pollution: even if another library allows prototype pollution, ejs will not allow escalating this into Remote Code Execution. --- lib/ejs.js | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/ejs.js b/lib/ejs.js index e46ece39..bd7ca927 100755 --- a/lib/ejs.js +++ b/lib/ejs.js @@ -44,6 +44,7 @@ * @public */ + var fs = require('fs'); var path = require('path'); var utils = require('./utils'); @@ -66,6 +67,17 @@ 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; +}(); + /** * EJS template function cache. This can be a LRU object from lru-cache NPM * module. By default, it is {@link module:utils.cache}, a simple in-process @@ -306,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); @@ -412,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 @@ -484,7 +496,7 @@ exports.renderFile = function () { opts.filename = filename; } else { - data = {}; + data = createObj(null); } return tryHandleCache(opts, data, cb); @@ -506,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; @@ -693,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;