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

CVE-2020-26301 - Upgrade ssh2 to 1.5.0 #74

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

TonyMasse
Copy link
Contributor

Hi @MCluck90 ,

ssh2 versions below 1.4 have a security vulnerability (CVE-2020-26301) where someone could inject some commands via the Agent name.

As I'm using simple-ssh and find it great, I took the liberty to upgrade all its dependencies (including Dev ones) and to adapt the code to work with the latest version of ssh2.

It passes the whole set of tests successfully:

image
(do not mind the super long times for the test to run, I'm working remotely and my test SSH server was far away and slow)

It looks like a lot of changes have been done to the code, but most of them are just reformatting due to the Lint and VSCode beautifying the code on Saves.

In reality, only a few changes were necessary:

  1. Importing and using ssh2.Client as the constructor for the SSH object, as well as in a couple of other places in the code
    • lines 3, 33, 268 and 274
  2. Add one verification in _queueCommand before the this._c.exec command, as ssh2 seems to behave slightly differently than its old versions, and would throw a "Not connected" error/exception on the last test of the batch as it tries to execute one command after having killed the session
    • lines 67 to 74

Do let me know if you have any questions.

Thanks for your work on this module,
Tony

@MCluck90
Copy link
Owner

Wow, this looks great! Thanks so much! I'm out of town this week but I'll happily merge this and push out a new version on Monday. Thanks again!

@TonyMasse
Copy link
Contributor Author

Brilliant.
Thanks Mike.

Will you mind pushing the module to NPM too?

Cheers,
Tony

@MCluck90
Copy link
Owner

Of course. I'll push it out as soon as I get back 😎

@MCluck90 MCluck90 merged commit 3038313 into MCluck90:master Jan 24, 2022
@TonyMasse
Copy link
Contributor Author

Tested importing it via NPM yesterday evening, and it's all working perfectly.

Thanks @MCluck90 for being so quick to merge and publish.

Cheers,
Tony

@Tiuipuv
Copy link

Tiuipuv commented Mar 8, 2022

@MCluck90 Would you also be able to cut a new release on the repo itself for 1.1.0?

@MCluck90
Copy link
Owner

MCluck90 commented Mar 8, 2022

@Tiuipuv Sure thing 😉

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