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

snyk security vuln. - update sshpk version #68

Closed
wants to merge 2 commits into from

Conversation

knoxcard
Copy link

✗ High severity vulnerability found on sshpk@1.13.1

  • desc: Regular Expression Denial of Service (ReDoS)
  • info: https://snyk.io/vuln/npm:sshpk:20180409
  • from: node_services@1.0.0 > node-gyp@3.6.2 > request@2.83.0 > http-signature@1.2.0 > sshpk@1.13.1
    Your dependencies are out of date, otherwise you would be using a newer sshpk than sshpk@1.13.1.

✗ High severity vulnerability found on sshpk@1.13.1
- desc: Regular Expression Denial of Service (ReDoS)
- info: https://snyk.io/vuln/npm:sshpk:20180409
- from: node_services@1.0.0 > node-gyp@3.6.2 > request@2.83.0 > http-signature@1.2.0 > sshpk@1.13.1
Your dependencies are out of date, otherwise you would be using a newer sshpk than sshpk@1.13.1.
@lc3t35
Copy link

lc3t35 commented Apr 21, 2018

+1

@austinkelleher
Copy link

Can we please get this merged @arekinath @davidlehn? Thanks in advance.

@davidlehn
Copy link
Contributor

@austinkelleher I'm not a maintainer here, just a former contributor and user. Note that in this case it's likely a semver based install will use the newer sshpk version anyway.

@wyardley
Copy link

wyardley commented May 9, 2018

FWIW, even if I remove node_modules and reinstall, I get an nsp warning for https://nodesecurity.io/advisories/606, e.g.,
opsgenie-sdk@0.4.2 > requestretry@1.13.0 > request@2.83.0 > http-signature@1.2.0 > sshpk@1.13.1
or
@google-cloud/pubsub@0.18.0 > google-gax@0.16.1 > grpc@1.8.4 > node-pre-gyp@0.6.39 > request@2.83.0 > http-signature@1.2.0 > sshpk@1.13.1

@NejcZdovc
Copy link

@wyardley security problem was fixed with 1.14.1, where you have version 1.13.1 installed

@knoxcard
Copy link
Author

knoxcard commented May 10, 2018

GitHub needs a community pull voting system that does not require permission of a maintainer. This project has 24 contributors, but appears to only have one maintainer @arekinath? Not good. Democracy should rule all, looks like I need to submit a pull request under GitHub to enable? :-)

@benwiggins
Copy link

benwiggins commented May 14, 2018

Or you could simply not create unnecessary pull requests.

You're pulling in an old, out of date dependency, most likely caused by your own package lock file.

There is a hint right there in your snyk output. When you see something like

Your dependencies are out of date, otherwise you would be using a newer sshpk than sshpk@1.13.1.

Snyk is telling you that you are pinning sshpk at @1.13.1, and it's not node-http-signature.

@knoxcard
Copy link
Author

@benwiggins - haha, I was unaware of this, thanks for educating me about this. Right on.

@knoxcard
Copy link
Author

knoxcard commented May 14, 2018

On that note, closing pull request and exiting building...

@knoxcard knoxcard closed this May 14, 2018
@davidlehn
Copy link
Contributor

Lock file nonsense aside, this PR is still a good idea to ensure a proper sshpk version is used.

@knoxcard
Copy link
Author

knoxcard commented May 15, 2018

What I am gathering so far is that you can solve this issue now by utilizing the snyk package.
How about those who are unaware of Snyk or fail to do frequent maintenance checks (snyk wizard)?
I'd then tend to agreee with @davidlehn, that this PR still does some good.

Personally, I run snyk wizard twice per day to ensure my system is up to par.

npm install snyk -g 

or

yarn add snyk global

then

snyk wizard

Follow the prompts accordingly to patch your system, updating local dependencies and applying available patches wherever possible. If my general assessment stated here is incorrect or you have any additional helpful recommendations, please share...

@knoxcard knoxcard reopened this May 15, 2018
@benwiggins
Copy link

benwiggins commented May 15, 2018

Don’t get me wrong, I 100% agree that bumping the minimum version to a known “good” version is good practice / courtesy.

But it does nothing to educate those tearing their hair out creating pull requests and issues because they don’t understand how dependency resolution/semvers/package locks actually work.

Nor does it necessarily solve someone’s nsp or snyk “security issue” if your package is a nested(-nested-nested) dependency, which in this case being a request dependency, is pretty likely. It just moves the pull request/issue noise somewhere else :)

package.json Outdated
@@ -30,7 +30,7 @@
"dependencies": {
"assert-plus": "^1.0.0",
"jsprim": "^1.2.2",
"sshpk": "^1.7.0"
"sshpk": "^1.14.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be ^1.14.2 to account for the new Buffer usage fix. See TritonDataCenter/node-sshpk#46

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, resubmitted pull request with 1.14.2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BYK Is this going to be merged? Thanks in advance.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the hold up?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're pinging the wrong person, I am neither a contributor nor a maintainer here 😊

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that @BYK
@knoxcard i'm working on fixing snyk vulnerabilities generated by sshpk version 1.7 that seems to be fixed on newest version 1.14.2.
I was wondering if there is a release date with that package updated.
Thanks!!

@spanditcaa spanditcaa mentioned this pull request Oct 22, 2019
@kusor
Copy link
Contributor

kusor commented Oct 30, 2019

Done as of PR #86

@kusor kusor closed this Oct 30, 2019
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.

10 participants