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.

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

feat: access control to prototype properties via whitelist #1633

Merged
merged 4 commits into from Jan 8, 2020
Merged

Conversation

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Jan 8, 2020

Disallow access to prototype properties and methods by default.
Access to properties is always checked via
Object.prototype.hasOwnProperty.call(parent, propertyName).

New runtime options:

  • allowedProtoMethods: a string-to-boolean map of property-names that are allowed if they are methods of the parent object.
  • allowedProtoProperties: a string-to-boolean map of property-names that are allowed if they are properties but not methods of the parent object.
const template = handlebars.compile('{{aString.trim}}')
const result = template({ aString: '  abc  ' })
// result is empty, because trim is defined at String prototype
const template = handlebars.compile('{{aString.trim}}')
const result = template({ aString: '  abc  ' }, {
  allowedProtoMethods: {
    trim: true
  }
})
// result = 'abc'

Implementation details: The method now "container.lookupProperty"
handles the prototype-checks and the white-lists. It is used in

  • JavaScriptCompiler#nameLookup
  • The "lookup"-helper (passed to all helpers as "options.lookupProperty")
  • The "lookup" function at the container, which is used for recursive lookups in "compat" mode

Compatibility:

  • Old precompiled templates work with new runtimes: The "options.lookupPropery"-function is passed to the helper by a wrapper, not by the compiled templated.
  • New templates work with old runtimes: The template contains a function that is used as fallback if the "lookupProperty"-function cannot be found at the container. However, the runtime-options "allowedProtoProperties" and "allowedProtoMethods" only work with the newest runtime.

BREAKING CHANGE:

  • access to prototype properties is forbidden completely by default

closes #1631
closes #1628

nknapp added 4 commits Dec 16, 2019
Disallow access to prototype properties and methods by default.
Access to properties is always checked via
`Object.prototype.hasOwnProperty.call(parent, propertyName)`.

New runtime options:
- **allowedProtoMethods**: a string-to-boolean map of property-names that are allowed if they are methods of the parent object.
- **allowedProtoProperties**: a string-to-boolean map of property-names that are allowed if they are properties but not methods of the parent object.

```js
const template = handlebars.compile('{{aString.trim}}')
const result = template({ aString: '  abc  ' })
// result is empty, because trim is defined at String prototype
```

```js
const template = handlebars.compile('{{aString.trim}}')
const result = template({ aString: '  abc  ' }, {
  allowedProtoMethods: {
    trim: true
  }
})
// result = 'abc'
```

Implementation details: The method now "container.lookupProperty"
handles the prototype-checks and the white-lists. It is used in
- JavaScriptCompiler#nameLookup
- The "lookup"-helper (passed to all helpers as "options.lookupProperty")
- The "lookup" function at the container, which is used for recursive lookups in "compat" mode

Compatibility:
- **Old precompiled templates work with new runtimes**: The "options.lookupPropery"-function is passed to the helper by a wrapper, not by the compiled templated.
- **New templates work with old runtimes**: The template contains a function that is used as fallback if the "lookupProperty"-function cannot be found at the container. However, the runtime-options "allowedProtoProperties" and "allowedProtoMethods" only work with the newest runtime.

BREAKING CHANGE:
- access to prototype properties is forbidden completely by default
- this allows the test to be run in a debugger
  without the complete PATH
@nknapp nknapp force-pushed the disallow-prototype branch from a4acfd6 to 2648921 Jan 8, 2020
@nknapp nknapp changed the base branch from master to 4.x Jan 8, 2020
@nknapp nknapp merged commit d7f0dcf into 4.x Jan 8, 2020
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@nknapp nknapp deleted the disallow-prototype branch Jan 8, 2020
@Gerrit0
Copy link

@Gerrit0 Gerrit0 commented Jan 9, 2020

@nknapp what would be your recommendation for projects which make heavy use of templates with helper methods attached to the prototype? TypeDoc uses handlebars and passes one of many different model classes to a template that will then render it.

For now, I'm going to pin TypeDoc to an older version of Handlebars with support for prototype properties/methods, but this really isn't a long term solution since staying up to date is important for security issues.

@nknapp
Copy link
Collaborator Author

@nknapp nknapp commented Jan 9, 2020

This change is the response to a security issue. The biggest one I have seen so far, at least if you are running Handlebars in a web-server. See http://mahmoudsec.blogspot.com/2019/04/handlebars-template-injection-and-rce.html for details.

My problem is: This issue has first appeared end of 2018. Nobody has thought about that vulnerability until then. When the principal got public, people started poking around and tried to circumvent my patches. I always tried minimal changes so far. And I have had, I think, three variants of the issue that again led to three new versions of handlebars, during the last year.

The actual proposal is, to use only registered helpers to implement logic in the templates. But I can see that this may be a lot of work for you.

There are two options:

  • Revert, but this may leave servers open to possible attacks
  • add another option that defines the default value of prototype prohibition.
    • make it opt-in (this will again leave servers open to possible attacks)
    • make it opt-out (this will require setting a runtime-option to establish the old behaviour)

Would it be ok for you to have an opt-out?

What I mean is an option: "allowProtoAccessByDefault" that is by default set to "false".

@Gerrit0
Copy link

@Gerrit0 Gerrit0 commented Jan 9, 2020

An opt-out would be ideal for my use case. TypeDoc already runs arbitrary user code when rendering themes, so the mentioned attack is a non-issue. A new option or being able to set allowedProtoMethods to true would work great!

I have been meaning to attack the themes and clean them up, maybe it is time to move that issue up the priority list and remove the need for prototype access entirely. It would be a lot of work, but perhaps that time has come.

@nknapp
Copy link
Collaborator Author

@nknapp nknapp commented Jan 9, 2020

see #1635

Please have a look and comment.

Implementing this was is not so much work now, because the general logic of runtime-options controlling this behaviour is already there.

@nknapp
Copy link
Collaborator Author

@nknapp nknapp commented Jan 10, 2020

Handlebars 4.7.0 has been release with options to disable prototype restrictions:
https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access

@tmaiaroto
Copy link

@tmaiaroto tmaiaroto commented Jan 10, 2020

Would JSON.parse(JSON.stringify(objWithGetter)) not have worked as a safe default behavior? When passing templates responses from ORMs (for example Sequelize ORM), you'll now need to get a plain object instead of being able to leverage its getters, eager loading, etc. In the case of Sequelize, it was a quick fix to call toJSON() but it sure was confusing to track this down and the minor version represented an unexpected break for something extremely common.

@nknapp
Copy link
Collaborator Author

@nknapp nknapp commented Jan 10, 2020

@tmaiaroto do you mean: as safe default behaviour instead of test hasOwnProperty for each access? Or instead of the changes I made in 4.7?

@Gerrit0
Copy link

@Gerrit0 Gerrit0 commented Jan 10, 2020

JSON.parse(JSON.stringify(obj)) is far from safe. JSON doesn't handle recursive data structures, while Handlebars does support them.

@tmaiaroto
Copy link

@tmaiaroto tmaiaroto commented Jan 10, 2020

Yea, and I did run into that. Oh well, bummer.

@nknapp
Copy link
Collaborator Author

@nknapp nknapp commented Jan 11, 2020

JSON.parse also does not support functions. It would also not fix the vulnerabilities to do stringify-parse on the input object because the prototype functions would still be there.

I actually would like to do the white listing differently in 5.0. It would probably make more sense to pass class constructors as a runtime option and allow prototype properties of exactly those classes.

This was referenced Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants