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

fix: use context property for template variables #163

Closed
wants to merge 3 commits into from

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Apr 29, 2021

BREAKING CHANGE:

An object passed to template data will need to be passed as an object in the context property.
This prevents mixing untrusted data with express-handlebars options.
For more information see https://blog.shoebpatel.com/2021/01/23/The-Secret-Parameter-LFR-and-Potential-RCE-in-NodeJS-Apps/

Thanks @agustingianni for bringing this to my attention.

Example:

<h1>Hi, {{name}}</h1>

<= v5

res.render('hi', {name: "Tony", layout: false})

v6

res.render('hi', {context: {name: "Tony"}, layout: false})

BREAKING CHANGE:

An object passed to template data with need to be passed as an object in the `context` property.
This prevents mixing untrusted data with express-handlebars options.
For more information see https://blog.shoebpatel.com/2021/01/23/The-Secret-Parameter-LFR-and-Potential-RCE-in-NodeJS-Apps/

**Example:**

```handlebars
<h1>Hi, {{name}}</h1>
```

**<= v5**

```js
res.render('hi', {name: "Tony", layout: false})
```

**v6**

```js
res.render('hi', {context: {name: "Tony"}, layout: false})
```
@agustingianni
Copy link

Hello, thanks for tagging me and sorry for the slight delay, I've been swamped with work. I will give it a look now.

@agustingianni
Copy link

So with this change if you force users to pass the template arguments via { context: { ... } } that should stop the attack, but I think this is not whats happening from my understanding of the code. If the user supplies an object that does not have the context handlebars just creates a default one and proceeds with the rendering:

	async renderView (viewPath, options = {}, callback = null) {
		const context = options.context || {};
    }

My concern would be that users that call res.render("name", taintedObject) are still vulnerable because taintedObject can be made in a way that it has the context key recently introduced and they still can override the same set of properties that allow for the bug to happen.

The problem with trying to fix this is that the problem is at three different levels, first the user uses the express render method in a way that is dangerous, second express merges the object passed by the user with data that is intended to be used by the underlying template engine, and third the renderer assumes that the configuration data comming from express is trustworthy.

In my opinion the most balanced way to fix this is to warn clients of handlebars to never pass objects whose keys are attacker controlled to the render method. This also has the benefit of not breaking the API for your users.

This is what eta did to address the same issue here, https://eta.js.org/docs/examples/express while they wait for express to implement a better API for the next big release.

Until then, I can't imagine a proper solution to the issue.

Let me know what you think!

@UziTech
Copy link
Member Author

UziTech commented May 4, 2021

Thanks! I added a note to the readme about this.

@UziTech
Copy link
Member Author

UziTech commented May 4, 2021

I'm going to hold off on this type of change until express fixes their API. Express v5 has been in alpha for almost 7 years 🤞 they will release it one of these days 🤣.

@UziTech UziTech closed this May 4, 2021
@UziTech UziTech reopened this May 4, 2021
@UziTech UziTech closed this May 4, 2021
@agustingianni
Copy link

Fantastic @UziTech thank you for your time addressing this issue!

@bavarianbytes
Copy link

Is it still neccessary to put template variables in a context object when passing to the render method? Can't find anything about that in the current documentation.

If yes, is it right that i need to access these variables in the handlebar template by calling {{context.myvariable}}?

@UziTech
Copy link
Member Author

UziTech commented Mar 22, 2022

@bavarianbytes no this change was not merged as it didn't fix the underlying vulnerability. The documentation should be correct.

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.

None yet

3 participants