diff --git a/lib/handlebars/compiler/compiler.js b/lib/handlebars/compiler/compiler.js index 894f7a7c0..9c08e9b0d 100644 --- a/lib/handlebars/compiler/compiler.js +++ b/lib/handlebars/compiler/compiler.js @@ -3,6 +3,7 @@ import Exception from '../exception'; import {isArray, indexOf, extend} from '../utils'; import AST from './ast'; +import {newObjectWithoutPrototypeProperties} from '../internal/newObjectWithoutPrototypeProperties'; const slice = [].slice; @@ -56,7 +57,8 @@ Compiler.prototype = { // These changes will propagate to the other compiler components let knownHelpers = options.knownHelpers; - options.knownHelpers = { + + options.knownHelpers = newObjectWithoutPrototypeProperties({ 'helperMissing': true, 'blockHelperMissing': true, 'each': true, @@ -65,14 +67,11 @@ Compiler.prototype = { 'with': true, 'log': true, 'lookup': true - }; + }); if (knownHelpers) { - // the next line should use "Object.keys", but the code has been like this a long time and changing it, might - // cause backwards-compatibility issues... It's an old library... - // eslint-disable-next-line guard-for-in - for (let name in knownHelpers) { - this.options.knownHelpers[name] = knownHelpers[name]; - } + Object.keys(knownHelpers).forEach(name => { + this.options.knownHelpers[name] = knownHelpers[name]; + }); } return this.accept(program); diff --git a/lib/handlebars/compiler/javascript-compiler.js b/lib/handlebars/compiler/javascript-compiler.js index b21df1d8a..9a60fbe30 100644 --- a/lib/handlebars/compiler/javascript-compiler.js +++ b/lib/handlebars/compiler/javascript-compiler.js @@ -2,6 +2,7 @@ import { COMPILER_REVISION, REVISION_CHANGES } from '../base'; import Exception from '../exception'; import {isArray} from '../utils'; import CodeGen from './code-gen'; +import {propertyMustBeEnumerable} from '../internal/propertyMustBeEnumerable'; function Literal(value) { this.value = value; @@ -13,9 +14,9 @@ JavaScriptCompiler.prototype = { // PUBLIC API: You can override these methods in a subclass to provide // alternative compiled forms for name lookup and buffering semantics nameLookup: function(parent, name/* , type*/) { - const isEnumerable = [ this.aliasable('container.propertyIsEnumerable'), '.call(', parent, ',"constructor")']; + const isEnumerable = [ this.aliasable('container.propertyIsEnumerable'), '.call(', parent, ',', JSON.stringify(name), ')']; - if (name === 'constructor') { + if (propertyMustBeEnumerable(name)) { return ['(', isEnumerable, '?', _actualLookup(), ' : undefined)']; } return _actualLookup(); @@ -93,6 +94,7 @@ JavaScriptCompiler.prototype = { this.useDepths = this.useDepths || environment.useDepths || environment.useDecorators || this.options.compat; this.useBlockParams = this.useBlockParams || environment.useBlockParams; + this.allowNonHelperFunctionCall = this.options.allowNonHelperFunctionCall !== false; // default if true for 4.x let opcodes = environment.opcodes, opcode, @@ -634,12 +636,20 @@ JavaScriptCompiler.prototype = { if (isSimple) { // direct call to helper possibleFunctionCalls.push(helper.name); } + // call a function from the input object - possibleFunctionCalls.push(nonHelper); + if (this.allowNonHelperFunctionCall) { + possibleFunctionCalls.push(nonHelper); + } + if (!this.options.strict) { possibleFunctionCalls.push(this.aliasable('container.hooks.helperMissing')); } + if (possibleFunctionCalls.length === 0) { + throw new Exception('Cannot create code for calling "' + name + '": Non-helper function calls are not allowed.'); + } + let functionLookupCode = ['(', this.itemsSeparatedBy(possibleFunctionCalls, '||'), ')']; let functionCall = this.source.functionCall(functionLookupCode, 'call', helper.callParams); this.push(functionCall); @@ -1009,6 +1019,7 @@ JavaScriptCompiler.prototype = { let params = [], paramsInit = this.setupHelperArgs(name, paramSize, params, blockHelper); let foundHelper = this.nameLookup('helpers', name, 'helper'), + // NEXT-MAJOR-UPGRADE: Always use "nullContext" in Handlebars 5.0 callContext = this.aliasable(`${this.contextName(0)} != null ? ${this.contextName(0)} : (container.nullContext || {})`); return { diff --git a/lib/handlebars/helpers/lookup.js b/lib/handlebars/helpers/lookup.js index 0654cc393..c261b0517 100644 --- a/lib/handlebars/helpers/lookup.js +++ b/lib/handlebars/helpers/lookup.js @@ -1,9 +1,11 @@ +import {propertyMustBeEnumerable} from '../internal/propertyMustBeEnumerable'; + export default function(instance) { instance.registerHelper('lookup', function(obj, field) { if (!obj) { return obj; } - if (String(field) === 'constructor' && !obj.propertyIsEnumerable(field)) { + if (propertyMustBeEnumerable(field) && !obj.propertyIsEnumerable(field)) { return undefined; } return obj[field]; diff --git a/lib/handlebars/internal/newObjectWithoutPrototypeProperties.js b/lib/handlebars/internal/newObjectWithoutPrototypeProperties.js new file mode 100644 index 000000000..2976afc89 --- /dev/null +++ b/lib/handlebars/internal/newObjectWithoutPrototypeProperties.js @@ -0,0 +1,23 @@ +/** + * Create an object without Object.prototype methods like __defineGetter__, constructor, + * __defineSetter__ and __proto__. + * + * Those methods should not be accessed from template code, because that can lead to + * security leaks. This method should be used to create internal objects that. + * + * @private + * @param {...object} sourceObjects + * @returns {object} + */ +export function newObjectWithoutPrototypeProperties(...sourceObjects) { + let result = Object.create(null); + sourceObjects.forEach(sourceObject => { + if (sourceObject != null) { + Object.keys(sourceObject).forEach(key => { + result[key] = sourceObject[key]; + }); + } + }); + return result; +} + diff --git a/lib/handlebars/internal/propertyMustBeEnumerable.js b/lib/handlebars/internal/propertyMustBeEnumerable.js new file mode 100644 index 000000000..ad541acec --- /dev/null +++ b/lib/handlebars/internal/propertyMustBeEnumerable.js @@ -0,0 +1,59 @@ +import {newObjectWithoutPrototypeProperties} from './newObjectWithoutPrototypeProperties'; + +const dangerousProperties = newObjectWithoutPrototypeProperties(); + +getFunctionPropertiesOf(Object.prototype).forEach(propertyName => { + dangerousProperties[propertyName] = true; +}); +getFunctionPropertiesOf(Array.prototype).forEach(propertyName => { + dangerousProperties[propertyName] = true; +}); +getFunctionPropertiesOf(Function.prototype).forEach(propertyName => { + dangerousProperties[propertyName] = true; +}); +getFunctionPropertiesOf(String.prototype).forEach(propertyName => { + dangerousProperties[propertyName] = true; +}); + +// eslint-disable-next-line no-proto +dangerousProperties.__proto__ = true; +dangerousProperties.__defineGetter__ = true; +dangerousProperties.__defineSetter__ = true; + +// Following properties are not _that_ dangerous +delete dangerousProperties.toString; +delete dangerousProperties.includes; +delete dangerousProperties.slice; +delete dangerousProperties.isPrototypeOf; +delete dangerousProperties.propertyIsEnumerable; +delete dangerousProperties.indexOf; +delete dangerousProperties.keys; +delete dangerousProperties.lastIndexOf; + +function getFunctionPropertiesOf(obj) { + return Object.getOwnPropertyNames(obj) + .filter(propertyName => { + try { + return typeof obj[propertyName] === 'function'; + } catch (error) { + // TypeError happens here when accessing 'caller', 'callee', 'arguments' on Function + return true; + } + }); +} + +/** + * Checks if a property can be harmful and should only processed when it is enumerable on its parent. + * + * This is necessary because of various "arbitrary-code-execution" issues that Handlebars has faced in the past. + * + * @param propertyName + * @returns {boolean} + */ +export function propertyMustBeEnumerable(propertyName) { + return dangerousProperties[propertyName]; +} + +export function getAllDangerousProperties() { + return dangerousProperties; +} diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 958afc3ef..c2836934c 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -2,6 +2,7 @@ import * as Utils from './utils'; import Exception from './exception'; import {COMPILER_REVISION, createFrame, LAST_COMPATIBLE_COMPILER_REVISION, REVISION_CHANGES} from './base'; import {moveHelperToHooks} from './helpers'; +import {newObjectWithoutPrototypeProperties} from './internal/newObjectWithoutPrototypeProperties'; export function checkRevision(compilerInfo) { const compilerRevision = compilerInfo && compilerInfo[0] || 1, @@ -158,13 +159,13 @@ export function template(templateSpec, env) { ret._setup = function(options) { if (!options.partial) { - container.helpers = Utils.extend({}, env.helpers, options.helpers); + container.helpers = newObjectWithoutPrototypeProperties(env.helpers, options.helpers); if (templateSpec.usePartial) { - container.partials = Utils.extend({}, env.partials, options.partials); + container.partials = newObjectWithoutPrototypeProperties(env.partials, options.partials); } if (templateSpec.usePartial || templateSpec.useDecorators) { - container.decorators = Utils.extend({}, env.decorators, options.decorators); + container.decorators = newObjectWithoutPrototypeProperties(env.decorators, options.decorators); } container.hooks = {}; diff --git a/spec/security.js b/spec/security.js index 876b67292..88e572f94 100644 --- a/spec/security.js +++ b/spec/security.js @@ -113,14 +113,67 @@ describe('security issues', function() { describe('GH-1563', function() { it('should not allow to access constructor after overriding via __defineGetter__', function() { - if (({}).__defineGetter__ == null || ({}).__lookupGetter__ == null) { - return; // Browser does not support this exploit anyway - } - shouldCompileTo('{{__defineGetter__ "undefined" valueOf }}' + - '{{#with __lookupGetter__ }}' + - '{{__defineGetter__ "propertyIsEnumerable" (this.bind (this.bind 1)) }}' + - '{{constructor.name}}' + - '{{/with}}', {}, ''); + + shouldThrow(function() { + compileWithPartials('{{__defineGetter__ "undefined" valueOf }}' + + '{{#with __lookupGetter__ }}' + + '{{__defineGetter__ "propertyIsEnumerable" (this.bind (this.bind 1)) }}' + + '{{constructor.name}}' + + '{{/with}}', [{}, {}, {}, {}])({}); + } + ); + }); + }); + + describe('the compile option "allowNonHelperFunctionCall"', function() { + it('when set to false should prevent calling functions in input objects', function() { + shouldThrow(function() { + var template = compileWithPartials('{{test abc}}', [{}, {}, {}, {allowNonHelperFunctionCall: false}]); + template({test: function() { return 'abc'; }}); + }, null, /Missing helper/); + }); + it('when set to false should prevent calling functions in input objects (in strict mode)', function() { + shouldThrow(function() { + var template = compileWithPartials('{{obj.method abc}}', [{}, {}, {}, {allowNonHelperFunctionCall: false, strict: true}]); + template({}); + }, null, /Cannot create code.*obj\.method.*Non-helper/); + }); + + }); + + describe('Properties that are required to be enumerable', function() { + + it('access should be restricted if not enumerable', function() { + shouldCompileTo('{{__defineGetter__}}', {}, ''); + shouldCompileTo('{{__defineSetter__}}', {}, ''); + shouldCompileTo('{{__proto__}}', {}, ''); + shouldCompileTo('{{constructor}}', {}, ''); + }); + + it('access should be allowed if enumerable', function() { + shouldCompileTo('{{__defineGetter__}}', {__defineGetter__: 'abc'}, 'abc'); + shouldCompileTo('{{__defineSetter__}}', {__defineSetter__: 'abc'}, 'abc'); + shouldCompileTo('{{constructor}}', {constructor: 'abc'}, 'abc'); + }); + + it('access can be allowed via the compile-option "propertyMustBeEnumerable"', function() { + var context = {}; + ['__defineGetter__', '__defineSetter__'].forEach(function(property) { + Object.defineProperty(context, property, { + get: function() { + return property; + }, + enumerable: false + }); + }); + + var compileOptions = { + propertyMustBeEnumerable: { + __defineGetter__: false + } + }; + + shouldCompileTo('{{__defineGetter__}}{{__defineSetter__}}', [context, {}, {}, compileOptions], '__defineGetter__'); }); }); }); diff --git a/types/index.d.ts b/types/index.d.ts index bb0656a51..d8e64307a 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -214,6 +214,7 @@ interface CompileOptions { preventIndent?: boolean; ignoreStandalone?: boolean; explicitPartialContext?: boolean; + allowNonHelperFunctionCall?: boolean; } type KnownHelpers = {