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

Bump uglify version to fix vulnerability #1084

Merged
merged 1 commit into from
Aug 25, 2015
Merged

Bump uglify version to fix vulnerability #1084

merged 1 commit into from
Aug 25, 2015

Conversation

John-Steidley
Copy link
Contributor

@kpdecker
Copy link
Collaborator

Ugh... do you know if any of Handlebar's code is impacted by this? Will certainly merge this but leaving open for right now while we assess the impact that this might have. My hope is that it's minor as there aren't a whole lot of combined flags like the exploit mentions but I don't fully understand the exploit yet.

@John-Steidley
Copy link
Contributor Author

A better source if you're having trouble understanding the vulnerability is here: https://zyan.scripts.mit.edu/blog/backdooring-js/

I looked into it, uglify is used in lib/precompiler.js. It seems to me that any code that passed to the precompiler with the -m or --min flags set is run through the vulnerable version of uglify and is therefore vulnerable. So, it's not a matter of the handlebars code itself being vulnerable, rather any client code that's using those options.

@hacker112
Copy link

Even though Handlebars is not effected by the vulnerability. Tools like grunt-nsp-shrinkwrap will give warnings until Uglify-JS is updated. And in my case it breaks our Jenkins build process (which it should since there might be a dangerous vulnerability, that should be fixed).

@John-Steidley
Copy link
Contributor Author

@hacker112, Handlebars may not itself be vulnerable to backdooring, but it is affected by the vulnerability because any code that uses handlebars is now potentially a target.

@kpdecker
Copy link
Collaborator

My read on this is that the code that is vulnerable is not written anywhere in Handlebars nor is it generated by our code generation logic. We have only one location that we combine not operations with non-boolean responses and that case doesn't minify to anything different that what is written.

Merging this but since we have a number of breaking changes on the tree and the current release has some additional breaking changes that are still under review we can't cleanly roll a release right now. This release is close but I can't estimate when I'll get community feedback on the open pull requests to provide a better estimate on when this release might go out.

Those who are cautious can minimize their own copy of Handlebars and perform their own minimization of the templates. Since the uglify code is not shipped to the client in any way and not utilized within node land unless you are using the precompiler, any perceived risk can easily be mitigated. Generally we recommend running your own compilation and minification regardless as the Handlebars CLI is provided as a connivence but is not intended for a production caliber release process.

If someone is aware of a specific bit of our code, generated or written, that is vulnerable in our code please email me directly and I'll be glad to see what we we can do to fix it discretely and out of band if need be.

kpdecker added a commit that referenced this pull request Aug 25, 2015
Bump uglify version to avoid vulnerability flag.
@kpdecker kpdecker merged commit e70430d into handlebars-lang:master Aug 25, 2015
@kpdecker kpdecker added this to the 4.0 milestone Aug 25, 2015
@John-Steidley
Copy link
Contributor Author

Thank you.

@kara-ryli
Copy link

@kpdecker what are the chances of getting this into a 3.0.4 release? I'd love to not have false positives in my NSP alerts.

@kpdecker
Copy link
Collaborator

Pretty much nil for the reasons outlined above. The tree is not in a releasable state and since this isn't an actual exploit it doesn't warrant an out of band release, particularly when other changes have been promised and we cherry-picking those changes would be a nightmare.

@ericelliott
Copy link

You don't think an issue that breaks a lot of dependent builds warrants a patch release? I'm with @RCanine here.

I completely understand it's a distraction and a pain (sincerely), but a lot of companies have a policy of no broken builds, which means this will force a drastic short-term action, like abandoning handlebars in favor of an alternative that doesn't leave builds broken.

@agladysh
Copy link

At a risk of sounding naive, it should be trivial to create a new branch from 3.0.3 tag and apply this change there. What am I missing? Why is it hard to make 3.0.3.1 release?

@hacker112
Copy link

I am thinking the same thing as @agladysh. Its one of the benefits with good versioning system that handles branches well like Git and Mercurial.

@agladysh
Copy link

If problem is really with Git (I suspect that it is not), I can help with creating a branch for the patch release.

@kpdecker
Copy link
Collaborator

kpdecker commented Sep 1, 2015

Released in 4.0.0

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.

6 participants