jsdoc2md / dmd

The default output template for jsdoc2md
MIT License
39 stars 50 forks source link

Update handlebars to address three security vulnerabilities #44

Closed gregswindle closed 7 years ago

gregswindle commented 7 years ago

Issue summary

I use jsdoc-to-markdown (i.e., the jsdoc2md CLI) to create API docs I can save in my repositories' GitHub wikis. It's a great tool: thanks!

I use several quality gates in my projects, including dependency checkers. Unfortunately, jsdoc-to-markdown has some nested dependencies that are out-of-date. In order to keep my projects' dependencies current, I've been shrink-wrapping them and updating the nested dependencies. Just as I was about to write a script to update my npm-shrinkwrap.json files, I thought, "Why don't I just report the issue, provide a (potential) fix, and submit a pull request?" :bowtie:

Expected Behavior

The [handlebars]() package should be updated to version >=4.0.0 in order to address these vulnerabilities:

  1. Quoteless Attributes in Templates can lead to Content Injection Discovered in a nested dependency: jsdoc-to-markdown@3.0.0 --> dmd@3.0.3 --> handlebars@3.0.3
  2. Incorrect Handling of Non-Boolean Comparisons During Minification Discovered in a nested dependency: jsdoc-to-markdown@3.0.0 --> dmd@3.0.3 --> handlebars@3.0.3 --> uglify-js@2.3.6
  3. Regular Expression Denial of Service Discovered in a nested dependency: jsdoc-to-markdown@3.0.0 --> dmd@3.0.3 --> handlebars@3.0.3 --> uglify-js@2.3.6

Current Behavior

The current version of dmd@3.0.3 -- and, consequently, jsdoc-to-markdown@3.0.0 -- uses a lower version of handlebars, which exposes the vulnerabilities described above.

Possible Solution

Update dmd to use handlebars@4.0.6, which itself has updated its nested dependencies, thereby addressing the vulnerabilities:

$ npm install --save handlebars@latest

:information_source: I have a pull request ready, if you wanna see it

I have forked jsdoc2md/dmd with the following updates. npm test passes locally as well as on Travis-CI. I'd be happy to submit a pull request.

Vulnerability Reports

  1. See the Bithound security advisory for one of my personal repositories.
  2. See the David dependency security warning for jsdoc2md/dmd.
75lb commented 7 years ago

hi, thanks for the great issue report!

jsdoc2md is a command-line tool, not a server exposed to the public. The only person with access to the running process is you, therefore jsdoc2md only exposes vulnerabilities if you are trying to hack yourself.

Duplicate of https://github.com/jsdoc2md/dmd/pull/35.

gregswindle commented 7 years ago

Thanks @75lb. Not to be a stickler, but when jsdoc2md is executed on a corporate CI/CD pipeline/workflow, it is technically executed on a server. I agree that the attack surface might be small, but it exists nevertheless, and it's trivial to patch.

So I take it you do not want me to send a pull request, correct?

Either way, thanks for taking the time to reply.

75lb commented 7 years ago

The attack surface is not small, it is non-existent. Even if your CI server was comprised by a hacker and they somehow got access to the running jsdoc2md process and exposed a vulnerability in handlebars, what could they do? Hijack your API docs? Security is not an issue.

If your PR updates the deps while not breaking the jsdoc2md output in any way, yes please raise it :)

gregswindle commented 7 years ago

I see your point: in fact, NSP doesn't seem to do nested package analysis (although I'm not sure why).

Regardless, the package upgrades don't break any of my tests, and I crave that green badge, so I'll send the PR. 💄 :on: 🐷

I'll dig for you JSDoc annotated source and provide more test results with the PR, as well. Thanks!

75lb commented 7 years ago

i think i will remove the David badge on dmd, the security alert is not relevant/applicable and just gets people worried.

75lb commented 6 years ago

handlebars upgraded in the latest release of dmd (v3.0.7).