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

Harden against prototype pollution. #275

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sayrer
Copy link
Collaborator

@sayrer sayrer commented Dec 18, 2021

This makes it more difficult for RCEs in other libraries to overwrite the prototype chain to manipulate Hogan, but also prevents authors from manipulating the prototype chain on purpose. I think the latter is rare enough that this change is worth it.

This first patch just fixes node.indent. See #274.

@sayrer sayrer changed the title Harden node.indent against prototype pollution. Harden createPartials against prototype pollution. Dec 18, 2021
@sayrer sayrer changed the title Harden createPartials against prototype pollution. Harden against prototype pollution. Dec 18, 2021
@sayrer
Copy link
Collaborator Author

sayrer commented Dec 18, 2021

Well, I typoed this, but it does fix the pollution. Will fix.

@ex1st
Copy link

ex1st commented Dec 19, 2021

Hi @sayrer, What do you think about that solution?

var context = Object.create(null, { 
  code: {
    value: "",
    writable: true,
    enumerable: true,
    configurable: true
  }, 
  subs: {
    value: Object.create(null),
    writable: true,
    enumerable: true,
    configurable: true
  }, 
  partials: {
    value: Object.create(null),
    writable: true,
    enumerable: true,
    configurable: true
  }
});

@sayrer
Copy link
Collaborator Author

sayrer commented Dec 19, 2021

Hi @sayrer, What do you think about that solution?

It doesn't quite work, but that is another way.

Welcome to Node.js v17.2.0.
Type ".help" for more information.
> var context = Object.create(null, { 
...   code: {
.....     value: "",
.....     writable: true,
.....     enumerable: true,
.....     configurable: true
.....   }}); 
undefined
> context
[Object: null prototype] { code: '' }
> context.__proto__
undefined
> context.code.__proto__
{}
> context.code.__proto__.indent = "foo"
'foo'
> context.code.__proto__.indent
'foo'

@sayrer
Copy link
Collaborator Author

sayrer commented Dec 19, 2021

It looks like I'll have to fix the test harness before I check this in, and your way will still break anyone relying on the prototype chain (probably unintentionally). But I'll see which one is cleaner in the end.

@ex1st
Copy link

ex1st commented Dec 20, 2021

Also, another simple way - predefined empty prefix

var context = { code: '', subs: {}, partials: {}, prefix: '' };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants