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

CURLOPT_SSH_HOST_PUBLIC_KEY_MD5 broken with libssh #4971

Closed
fds242 opened this issue Feb 23, 2020 · 3 comments
Closed

CURLOPT_SSH_HOST_PUBLIC_KEY_MD5 broken with libssh #4971

fds242 opened this issue Feb 23, 2020 · 3 comments
Labels

Comments

@fds242
Copy link

@fds242 fds242 commented Feb 23, 2020

Handling the CURLOPT_SSH_HOST_PUBLIC_KEY_MD5 option (also exposed in the curl command-line tool with --hostpubmd5) is hopelessly broken when compiled with libssh (as opposed to libssh2, where it does function as documented).
The option is documented as “a string containing 32 hexadecimal digits.“ The code in lib/vssh/libssh.c instead seems to expect to compare it to a 16-byte binary.
Even if by luck you have a binary hash without any NUL bytes so that you could set it as the NUL-terminated curl string option, the comparison still cannot pass: there's an additional bug in the code. An inexplicable spurious & is making the memcmp() run on the pointer to the option value, reading past it for some additional bytes (16 minus the length of pointers):

memcmp(&data->set.str[STRING_SSH_HOST_PUBLIC_KEY_MD5], hash, hlen)) {

@jay jay added the SCP/SFTP label Feb 23, 2020
jay added a commit to jay/curl that referenced this issue Feb 23, 2020
Prior to this change a match would never be successful because it
was mistakenly coded to compare binary data from libssh to a
user-specified hex string (ie CURLOPT_SSH_HOST_PUBLIC_KEY_MD5).

Reported-by: fds242@users.noreply.github.com

Fixes curl#4971
Closes #xxxx
@jay

This comment has been minimized.

Copy link
Member

@jay jay commented Feb 23, 2020

Ref: https://api.libssh.org/master/group__libssh__session.html#ga7a7b16a4bed6d8d58f10bdb269172ff7

It seems like that doc could be clearer. Can you try #4974 please?

/cc @nmav

@fds242

This comment has been minimized.

Copy link
Author

@fds242 fds242 commented Feb 24, 2020

Thank you, very speedy!
Rebuilt and verified, this can work as documented now, and be made to succeed.

In the interim I was using a quick copy-paste job from lib/vssh/libssh2.c, because that had the exact same task with the hex conversion and case-insensitive comparison afterwards. That used a msnprintf loop for the hex conversion, this does feel neater. One nicety the libssh2 version does have though is that it sets a failf() message on failure, letting the developers looking at the curl error buffer know exactly what was compared to what.

It's probably not worth any further effort. While CURLOPT_SSH_HOST_PUBLIC_KEY_MD5 technically functions now, it's still practically pointless. Most ssh servers have multiple host keys, and the libcurl developer has no influence over which one will be picked for verification here. For my same test server, libssh verifies against the fingerprint of the ssh-ed25519 key, libssh2/1.9.0 against the ecdsa-sha2-nistp256 key, and libssh2/1.8.2 against the ssh-rsa. Each of those possibilities are a valid choice, and can't be faulted for not matching the documentation. I suppose people using the libcurl API could brute force it by looping through trying to make the connection using each possible MD5 hash.
Maybe the libssh developers could chime in, and tell us if it would be easy enough to extend this check to match either of the host keys?

Then again I only ever ended up experimenting with this because of #4972, when for an extended time I couldn't figure out why I cannot make the verification fail on one of my test systems through the file I set as CURLOPT_SSH_KNOWNHOSTS, and was panicking it wasn't doing the verification at all. Turned out it was actually doing the verification, only ignoring my KNOWNHOSTS setting, and picking up the OpenSSH configuration instead… But that's the other report.

I don't know the etiquette on if I should choose the "Close" button myself now, so I'm leaving it as is, but I do consider this bug report fixed & closed. Thank you!

jay added a commit to jay/curl that referenced this issue Feb 24, 2020
Prior to this change a match would never be successful because it
was mistakenly coded to compare binary data from libssh to a
user-specified hex string (ie CURLOPT_SSH_HOST_PUBLIC_KEY_MD5).

Reported-by: fds242@users.noreply.github.com

Fixes curl#4971
Closes #xxxx
@jay

This comment has been minimized.

Copy link
Member

@jay jay commented Feb 24, 2020

Thanks for that additional information. I agree it's rather complicated. I do not have a libssh build and I'm not familiar enough with the SSH code to have a strong opinion. We can leave this open for feedback. I've since updated the libssh check to more closely match the one in libssh2 and pushed those changes to the PR, if you could try that.

jay added a commit to jay/curl that referenced this issue Mar 1, 2020
Prior to this change a match would never be successful because it
was mistakenly coded to compare binary data from libssh to a
user-specified hex string (ie CURLOPT_SSH_HOST_PUBLIC_KEY_MD5).

Reported-by: fds242@users.noreply.github.com

Fixes curl#4971
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Mar 4, 2020
Prior to this change a match would never be successful because it
was mistakenly coded to compare binary data from libssh to a
user-specified hex string (ie CURLOPT_SSH_HOST_PUBLIC_KEY_MD5).

Reported-by: fds242@users.noreply.github.com

Fixes curl#4971
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Mar 6, 2020
Prior to this change a match would never be successful because it
was mistakenly coded to compare binary data from libssh to a
user-specified hex string (ie CURLOPT_SSH_HOST_PUBLIC_KEY_MD5).

Reported-by: fds242@users.noreply.github.com

Fixes curl#4971
Closes #xxxx
@jay jay closed this in 09aa807 Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.