search

mscdex / ssh2

SSH2 client and server modules written in pure JavaScript for node.js
MIT License
5.51k stars 664 forks source link

hostVerifier provides no access to the key type #1421

Closed lantw44 closed 2 days ago

lantw44 commented 5 days ago

The ~/.ssh/known_hosts file has a field used to store the key type, such as ssh-rsa, ecdsa-sha2-nistp256, ssh-ed25519. The SSHFP DNS record also has an algorithm field. This suggests it isn't a useless field, and the OpenSSH ssh command does use the field when checking the host key.

However, the hostVerifier callback only receives the public key binary. It has no access to the key type. This means programs using this library are unable to behave in the same way as the commonly used ssh command. I am not sure if key type confusion can impact security here, but probably it is safer to do what the most commonly used client do.

mscdex commented 5 days ago

You can always use ssh2.utils.parseKey() to get the key type if you have the raw key value.

lantw44 commented 2 days ago

Thank you! It is surprising to know OpenSSH keeps duplicate information in the ~/.ssh/known_hosts file. The README is confusing as hostVerifier is documented to provide hashedKey as a string. It doesn't say it is actually a Buffer with the raw key value which can be passed into parseKey.

I also tried to use the equals method provided by the parseKey output. However, it returned false even if the exactly the same string is passed into parseKey. It seems that it was comparing Buffer objects with === here: https://github.com/mscdex/ssh2/blob/v1.15.0/lib/protocol/keyParser.js#L441-L449, so it couldn't work. getPublicSSH is documented to return a string, but I got a Buffer here.

mscdex commented 2 days ago

It doesn't say it is actually a Buffer with the raw key value which can be passed into parseKey

It was a later addition because originally it was always passed a hashed string value. The documentation is just lacking in that bit.

getPublicSSH is documented to return a string, but I got a Buffer here.

It's a bug, it should be using .equals() to compare the Buffers.

mscdex commented 2 days ago

The aforementioned issues have been addressed in cb6747b038bde771ecc50b02fb706d6f5253d6a6 and cd353df03a082becdcf5456de0c99eed09f4209b.

lantw44 commented 1 day ago

Thanks for your quick response! Now equals is fixed to use the Buffer equals method. It seems to me that getPublicSSH should be documented to return a Buffer instead of a string as well.