From 7af1c12db6222ab4b689bb60820628209d295049 Mon Sep 17 00:00:00 2001 From: Nils Knappmeier Date: Thu, 9 Jan 2020 23:00:32 +0100 Subject: [PATCH] feat: default options for controlling proto access This commmit adds the runtime options - `allowProtoPropertiesByDefault` (boolean, default: false) and - `allowProtoMethodsByDefault` (boolean, default: false)` which can be used to allow access to prototype properties and functions in general. Specific properties and methods can still be disabled from access via `allowedProtoProperties` and `allowedProtoMethods` by setting the corresponding values to false. The methods `constructor`, `__defineGetter__`, `__defineSetter__`, `__lookupGetter__` and the property `__proto__` will be disabled, even if the allow...ByDefault-options are set to true. In order to allow access to those properties and methods, they have to be explicitly set to true in the 'allowedProto...'-options. A warning is logged when the a proto-access it attempted and denied by default (i.e. if no option is set by the user to make the access decision explicit) --- ...pObject.js => create-new-lookup-object.js} | 0 lib/handlebars/internal/proto-access.js | 55 ++++ lib/handlebars/runtime.js | 29 +- spec/security.js | 275 +++++++++++------- types/index.d.ts | 6 +- types/test.ts | 6 +- 6 files changed, 251 insertions(+), 120 deletions(-) rename lib/handlebars/internal/{createNewLookupObject.js => create-new-lookup-object.js} (100%) create mode 100644 lib/handlebars/internal/proto-access.js diff --git a/lib/handlebars/internal/createNewLookupObject.js b/lib/handlebars/internal/create-new-lookup-object.js similarity index 100% rename from lib/handlebars/internal/createNewLookupObject.js rename to lib/handlebars/internal/create-new-lookup-object.js diff --git a/lib/handlebars/internal/proto-access.js b/lib/handlebars/internal/proto-access.js new file mode 100644 index 000000000..59bf14448 --- /dev/null +++ b/lib/handlebars/internal/proto-access.js @@ -0,0 +1,55 @@ +import { createNewLookupObject } from './create-new-lookup-object'; + +export function createProtoAccessControl(runtimeOptions) { + let defaultMethodWhiteList = Object.create(null); + defaultMethodWhiteList['constructor'] = false; + defaultMethodWhiteList['__defineGetter__'] = false; + defaultMethodWhiteList['__defineSetter__'] = false; + defaultMethodWhiteList['__lookupGetter__'] = false; + + let defaultPropertyWhiteList = Object.create(null); + // eslint-disable-next-line no-proto + defaultPropertyWhiteList['__proto__'] = false; + + return { + properties: { + whitelist: createNewLookupObject( + defaultPropertyWhiteList, + runtimeOptions.allowedProtoProperties + ), + defaultValue: runtimeOptions.allowProtoPropertiesByDefault + }, + methods: { + whitelist: createNewLookupObject( + defaultMethodWhiteList, + runtimeOptions.allowedProtoMethods + ), + defaultValue: runtimeOptions.allowProtoMethodsByDefault + } + }; +} + +export function resultIsAllowed(result, protoAccessControl, propertyName) { + if (typeof result === 'function') { + return checkWhiteList(protoAccessControl.methods, propertyName); + } else { + return checkWhiteList(protoAccessControl.properties, propertyName); + } +} + +function checkWhiteList(protoAccessControlForType, propertyName) { + if (protoAccessControlForType.whitelist[propertyName] !== undefined) { + return protoAccessControlForType.whitelist[propertyName] === true; + } + if (protoAccessControlForType.defaultValue !== undefined) { + return protoAccessControlForType.defaultValue; + } + + // eslint-disable-next-line no-console + console.error( + `Handlebars: Access has been denied to resolve the property "${propertyName}" because it is not an "own property" of its parent.\n` + + `You can add a runtime option to disable the check or this warning:\n` + + `See http://localhost:8080/api-reference/runtime-options.html#options-to-control-prototype-access for details` + ); + return false; +} diff --git a/lib/handlebars/runtime.js b/lib/handlebars/runtime.js index 7d63e87b8..67a500d79 100644 --- a/lib/handlebars/runtime.js +++ b/lib/handlebars/runtime.js @@ -8,7 +8,10 @@ import { } from './base'; import { moveHelperToHooks } from './helpers'; import { wrapHelper } from './internal/wrapHelper'; -import { createNewLookupObject } from './internal/createNewLookupObject'; +import { + createProtoAccessControl, + resultIsAllowed +} from './internal/proto-access'; export function checkRevision(compilerInfo) { const compilerRevision = (compilerInfo && compilerInfo[0]) || 1, @@ -73,8 +76,7 @@ export function template(templateSpec, env) { let extendedOptions = Utils.extend({}, options, { hooks: this.hooks, - allowedProtoMethods: this.allowedProtoMethods, - allowedProtoProperties: this.allowedProtoProperties + protoAccessControl: this.protoAccessControl }); let result = env.VM.invokePartial.call( @@ -126,15 +128,14 @@ export function template(templateSpec, env) { }, lookupProperty: function(parent, propertyName) { let result = parent[propertyName]; + if (result == null) { + return result; + } if (Object.prototype.hasOwnProperty.call(parent, propertyName)) { return result; } - const whitelist = - typeof result === 'function' - ? container.allowedProtoMethods - : container.allowedProtoProperties; - if (whitelist[propertyName] === true) { + if (resultIsAllowed(result, container.protoAccessControl, propertyName)) { return result; } return undefined; @@ -237,6 +238,7 @@ export function template(templateSpec, env) { ) ); } + main = executeDecorators( templateSpec.main, main, @@ -247,6 +249,7 @@ export function template(templateSpec, env) { ); return main(context, options); } + ret.isTop = true; ret._setup = function(options) { @@ -271,12 +274,7 @@ export function template(templateSpec, env) { } container.hooks = {}; - container.allowedProtoProperties = createNewLookupObject( - options.allowedProtoProperties - ); - container.allowedProtoMethods = createNewLookupObject( - options.allowedProtoMethods - ); + container.protoAccessControl = createProtoAccessControl(options); let keepHelperInHelpers = options.allowCallsToHelperMissing || @@ -284,8 +282,7 @@ export function template(templateSpec, env) { moveHelperToHooks(container, 'helperMissing', keepHelperInHelpers); moveHelperToHooks(container, 'blockHelperMissing', keepHelperInHelpers); } else { - container.allowedProtoProperties = options.allowedProtoProperties; - container.allowedProtoMethods = options.allowedProtoMethods; + container.protoAccessControl = options.protoAccessControl; // internal option container.helpers = options.helpers; container.partials = options.partials; container.decorators = options.decorators; diff --git a/spec/security.js b/spec/security.js index 7bf9d89a0..bf0be1485 100644 --- a/spec/security.js +++ b/spec/security.js @@ -149,176 +149,251 @@ describe('security issues', function() { }); }); - describe('GH-1595', function() { - it('properties, that are required to be own properties', function() { - expectTemplate('{{constructor}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{__defineGetter__}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{__defineSetter__}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{__lookupGetter__}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{__proto__}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{lookup this "constructor"}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{lookup this "__defineGetter__"}}') - .withInput({}) - .toCompileTo(''); - - expectTemplate('{{lookup this "__defineSetter__"}}') - .withInput({}) - .toCompileTo(''); + describe('GH-1595: dangerous properties', function() { + var templates = [ + '{{constructor}}', + '{{__defineGetter__}}', + '{{__defineSetter__}}', + '{{__lookupGetter__}}', + '{{__proto__}}', + '{{lookup this "constructor"}}', + '{{lookup this "__defineGetter__"}}', + '{{lookup this "__defineSetter__"}}', + '{{lookup this "__lookupGetter__"}}', + '{{lookup this "__proto__"}}' + ]; + + templates.forEach(function(template) { + describe('access should be denied to ' + template, function() { + it('by default', function() { + expectTemplate(template) + .withInput({}) + .toCompileTo(''); + }); + it(' with proto-access enabled', function() { + expectTemplate(template) + .withInput({}) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: true, + allowProtoMethodsByDefault: true + }) + .toCompileTo(''); + }); + }); + }); + }); + describe('GH-1631: disallow access to prototype functions', function() { + function TestClass() {} - expectTemplate('{{lookup this "__lookupGetter__"}}') - .withInput({}) - .toCompileTo(''); + TestClass.prototype.aProperty = 'propertyValue'; + TestClass.prototype.aMethod = function() { + return 'returnValue'; + }; - expectTemplate('{{lookup this "__proto__"}}') - .withInput({}) - .toCompileTo(''); + afterEach(function() { + sinon.restore(); }); - describe('GH-1631: disallow access to prototype functions', function() { - function TestClass() {} + describe('control access to prototype methods via "allowedProtoMethods"', function() { + checkProtoMethodAccess({}); + + describe('in compat mode', function() { + checkProtoMethodAccess({ compat: true }); + }); - TestClass.prototype.aProperty = 'propertyValue'; - TestClass.prototype.aMethod = function() { - return 'returnValue'; - }; + function checkProtoMethodAccess(compileOptions) { + it('should be prohibited by default and log a warning', function() { + var spy = sinon.spy(console, 'error'); - describe('control access to prototype methods via "allowedProtoMethods"', function() { - it('should be prohibited by default', function() { expectTemplate('{{aMethod}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .toCompileTo(''); + + expect(spy.calledOnce).to.be.true(); + expect(spy.args[0][0]).to.match(/Handlebars: Access has been denied/); }); - it('can be allowed', function() { + it('can be allowed, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ allowedProtoMethods: { aMethod: true } }) .toCompileTo('returnValue'); + + expect(spy.callCount).to.equal(0); }); - it('should be prohibited by default (in "compat" mode)', function() { + it('can be turned on by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) - .toCompileTo(''); + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoMethodsByDefault: true + }) + .toCompileTo('returnValue'); + + expect(spy.callCount).to.equal(0); }); - it('can be allowed (in "compat" mode)', function() { + it('can be turned off by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aMethod}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) + .withCompileOptions(compileOptions) .withRuntimeOptions({ - allowedProtoMethods: { - aMethod: true - } + allowProtoMethodsByDefault: false }) - .toCompileTo('returnValue'); - }); + .toCompileTo(''); - it('should cause the recursive lookup by default (in "compat" mode)', function() { - expectTemplate('{{#aString}}{{trim}}{{/aString}}') - .withInput({ aString: ' abc ', trim: 'trim' }) - .withCompileOptions({ compat: true }) - .toCompileTo('trim'); + expect(spy.callCount).to.equal(0); }); - it('should not cause the recursive lookup if allowed through options(in "compat" mode)', function() { - expectTemplate('{{#aString}}{{trim}}{{/aString}}') - .withInput({ aString: ' abc ', trim: 'trim' }) - .withCompileOptions({ compat: true }) + it('can be turned off, if turned on by default', function() { + expectTemplate('{{aMethod}}') + .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ + allowProtoMethodsByDefault: true, allowedProtoMethods: { - trim: true + aMethod: false } }) - .toCompileTo('abc'); + .toCompileTo(''); }); + } + + it('should cause the recursive lookup by default (in "compat" mode)', function() { + expectTemplate('{{#aString}}{{trim}}{{/aString}}') + .withInput({ aString: ' abc ', trim: 'trim' }) + .withCompileOptions({ compat: true }) + .toCompileTo('trim'); + }); + + it('should not cause the recursive lookup if allowed through options(in "compat" mode)', function() { + expectTemplate('{{#aString}}{{trim}}{{/aString}}') + .withInput({ aString: ' abc ', trim: 'trim' }) + .withCompileOptions({ compat: true }) + .withRuntimeOptions({ + allowedProtoMethods: { + trim: true + } + }) + .toCompileTo('abc'); }); + }); + + describe('control access to prototype non-methods via "allowedProtoProperties" and "allowProtoPropertiesByDefault', function() { + checkProtoPropertyAccess({}); + + describe('in compat-mode', function() { + checkProtoPropertyAccess({ compat: true }); + }); + + function checkProtoPropertyAccess(compileOptions) { + it('should be prohibited by default and log a warning', function() { + var spy = sinon.spy(console, 'error'); + + expectTemplate('{{aProperty}}') + .withInput(new TestClass()) + .withCompileOptions(compileOptions) + .toCompileTo(''); + + expect(spy.calledOnce).to.be.true(); + expect(spy.args[0][0]).to.match(/Handlebars: Access has been denied/); + }); + + it('can be explicitly prohibited by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); - describe('control access to prototype non-methods via "allowedProtoProperties"', function() { - it('should be prohibited by default', function() { expectTemplate('{{aProperty}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: false + }) .toCompileTo(''); + + expect(spy.callCount).to.equal(0); }); - it('can be turned on', function() { + it('can be turned on, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aProperty}}') .withInput(new TestClass()) + .withCompileOptions(compileOptions) .withRuntimeOptions({ allowedProtoProperties: { aProperty: true } }) .toCompileTo('propertyValue'); + + expect(spy.callCount).to.equal(0); }); - it('should be prohibited by default (in "compat" mode)', function() { + it('can be turned on by default, which disables the warning', function() { + var spy = sinon.spy(console, 'error'); + expectTemplate('{{aProperty}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) - .toCompileTo(''); + .withCompileOptions(compileOptions) + .withRuntimeOptions({ + allowProtoPropertiesByDefault: true + }) + .toCompileTo('propertyValue'); + + expect(spy.callCount).to.equal(0); }); - it('can be turned on (in "compat" mode)', function() { + it('can be turned off, if turned on by default', function() { expectTemplate('{{aProperty}}') .withInput(new TestClass()) - .withCompileOptions({ compat: true }) + .withCompileOptions(compileOptions) .withRuntimeOptions({ + allowProtoPropertiesByDefault: true, allowedProtoProperties: { - aProperty: true + aProperty: false } }) - .toCompileTo('propertyValue'); + .toCompileTo(''); }); - }); + } + }); - describe('compatibility with old runtimes, that do not provide the function "container.lookupProperty"', function() { - beforeEach(function simulateRuntimeWithoutLookupProperty() { - var oldTemplateMethod = handlebarsEnv.template; - sinon.replace(handlebarsEnv, 'template', function(templateSpec) { - templateSpec.main = wrapToAdjustContainer(templateSpec.main); - return oldTemplateMethod.call(this, templateSpec); - }); + describe('compatibility with old runtimes, that do not provide the function "container.lookupProperty"', function() { + beforeEach(function simulateRuntimeWithoutLookupProperty() { + var oldTemplateMethod = handlebarsEnv.template; + sinon.replace(handlebarsEnv, 'template', function(templateSpec) { + templateSpec.main = wrapToAdjustContainer(templateSpec.main); + return oldTemplateMethod.call(this, templateSpec); }); + }); - afterEach(function() { - sinon.restore(); - }); + afterEach(function() { + sinon.restore(); + }); - it('should work with simple properties', function() { - expectTemplate('{{aProperty}}') - .withInput({ aProperty: 'propertyValue' }) - .toCompileTo('propertyValue'); - }); + it('should work with simple properties', function() { + expectTemplate('{{aProperty}}') + .withInput({ aProperty: 'propertyValue' }) + .toCompileTo('propertyValue'); + }); - it('should work with Array.prototype.length', function() { - expectTemplate('{{anArray.length}}') - .withInput({ anArray: ['a', 'b', 'c'] }) - .toCompileTo('3'); - }); + it('should work with Array.prototype.length', function() { + expectTemplate('{{anArray.length}}') + .withInput({ anArray: ['a', 'b', 'c'] }) + .toCompileTo('3'); }); }); }); diff --git a/types/index.d.ts b/types/index.d.ts index 606741f5a..1fa83680d 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -30,8 +30,10 @@ declare namespace Handlebars { data?: any; blockParams?: any[]; allowCallsToHelperMissing?: boolean; - allowedProtoProperties?: { [name: string]: boolean } - allowedProtoMethods?: { [name: string]: boolean } + allowedProtoProperties?: { [name: string]: boolean }; + allowedProtoMethods?: { [name: string]: boolean }; + allowProtoPropertiesByDefault?: boolean; + allowProtoMethodsByDefault?: boolean; } export interface HelperOptions { diff --git a/types/test.ts b/types/test.ts index b081ae4fb..7a34b7eba 100644 --- a/types/test.ts +++ b/types/test.ts @@ -241,12 +241,14 @@ function testExceptionWithNodeTypings() { let stack: string | undefined = exception.stack; } -function testProtoPropertyControlOptions() { +function testProtoAccessControlControlOptions() { Handlebars.compile('test')( {}, { allowedProtoMethods: { allowedMethod: true, forbiddenMethod: false }, - allowedProtoProperties: { allowedProperty: true, forbiddenProperty: false } + allowedProtoProperties: { allowedProperty: true, forbiddenProperty: false }, + allowProtoMethodsByDefault: true, + allowProtoPropertiesByDefault: false, } ); }