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

Prevent access to other prototype properties #1594

Closed
wants to merge 4 commits into from
Closed

Conversation

nknapp
Copy link
Collaborator

@nknapp nknapp commented Nov 5, 2019

The npm security advisories 755 and 1164 were both caused by access to the constructor property. The initial fix was circumvented in advisory (1164) by the use of __defineGetter__ and __defineSetter.

In order to prevent other attacks in the future, this PR restricts access to properties in two way:

  • if the compile-option allowNonHelperFunctionCall is set to false, any use of properties of the input object as function-calls with parameters is prohibited. Lambdas are still allowed.
  • Access to the properties __defineGetter__, __defineSetter and __proto__ are only allowed if they are enumerable on their parent. The list is configurable through the compile-option propertyMustBeEnumerable.
Handlebars.compile('{{__defineGetter__}}', {
   propertyMustBeEnumerable: {
      "__defineGetter__": false,
      "push": true
   }
})

The main question is: Which other properties should be restricted by default?

  • push, pop, fill, shift, unshift
  • bind, call, apply

I don't want to cause trouble by creating unnecessary restrictions, but I also want to prevent other security leaks.

@nknapp nknapp added possibly breaking Add a comment to this issue, if you think this will break your build topic for discussion labels Nov 5, 2019
@nknapp nknapp force-pushed the security-fixes-next branch from ab74b7f to 4c034c8 Compare November 10, 2019 20:10
@nknapp
Copy link
Collaborator Author

nknapp commented Nov 10, 2019

@skinny85 could you have a look at this?

Update: Wait a moment. Two of the commits don't actually belong here. I'll move them away

Update: I think, now I am ready. I'm not completely happy with the comment that is needed in the code to understand what I am doing, but I think it is not avoidable.

@skinny85
Copy link

@nknapp while I appreciate the vote of confidence, I think this PR is way beyond my knowledge of the subtleties of JavaScript 😱

One general comment: I think the PR description does a great job of justifying why is the change needed. I think it would be great if a similar description made its way into the comments in the code itself - I think future maintainers of this codebase (which might very well be you 😉) will appreciate it when they see this a year (or later) from now :).

@nknapp nknapp force-pushed the security-fixes-next branch from 96df165 to 31f3343 Compare November 14, 2019 07:12
Use map-objects (with `null`-prototype
for some internal maps that are accessed by
the compiled template code to prevent
accidental access of Object prototype
properties (this includes the "helpers"-object,
"partials"-object and "knownHelpers")

Added compile options:
- allowNonHelperFunctionCall:
    default: true, if set to false, the
    template will not call functions with
    arguments) that are defined on the current
    context object. Lambdas (function calls
    without arguments) are still allowed.

- propertyMustBeEnumerable
    a object (propertyName: boolean) of
    properties that must be enumerable in
    order to be resolved from the current
    context object.
    By default `__defineGetter__`,
    `__defineSetter__` , `__proto__`
    and `constructor` are forbidden (if not
    enumerable) and will return "undefined"
    instead of the actual property.
    Use `{ __defineGetter_: false }`
    to allow __defineGetter__ (not recomended).
- properties not configurable anymore
- add more harmful properties to the list
  (push, pop, splice...)
@nknapp nknapp force-pushed the security-fixes-next branch from 31f3343 to caacce4 Compare November 17, 2019 08:05
@nknapp nknapp closed this Dec 3, 2019
@nknapp nknapp deleted the security-fixes-next branch December 3, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
possibly breaking Add a comment to this issue, if you think this will break your build topic for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants