Re: [Qemu-block] [PATCH v10] ssh: switch from libssh2 to libssh

[Max Reitz]

On 20.06.19 22:03, Pino Toscano wrote:
> On Thursday, 20 June 2019 14:58:40 CEST Max Reitz wrote:
>> On 20.06.19 11:49, Pino Toscano wrote:
>>> On Tuesday, 18 June 2019 15:14:30 CEST Max Reitz wrote:
>>>> On 18.06.19 11:24, Pino Toscano wrote:
>>>>> Rewrite the implementation of the ssh block driver to use libssh instead
>>>>> of libssh2.  The libssh library has various advantages over libssh2:
>>>>> - easier API for authentication (for example for using ssh-agent)
>>>>> - easier API for known_hosts handling
>>>>> - supports newer types of keys in known_hosts
>>>>>
>>>>> Use APIs/features available in libssh 0.8 conditionally, to support
>>>>> older versions (which are not recommended though).
>>>>>
>>>>> Adjust the tests according to the different error message, and to the
>>>>> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
>>>>> and libssh >= 0.7.0.
>>>>>
>>>>> Adjust the various Docker/Travis scripts to use libssh when available
>>>>> instead of libssh2. The mingw/mxe testing is dropped for now, as there
>>>>> are no packages for it.
>>>>>
>>>>> Signed-off-by: Pino Toscano <address@hidden>
>>>>> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>>>>> Acked-by: Alex Bennée <address@hidden>
>>>>> ---
>>>>>
>>>>> Changes from v9:
>>>>> - restored "default" case in the server status switch for libssh < 0.8.0
>>>>> - print the host key type & fingerprint on mismatch with known_hosts
>>>>> - improve/fix message for failed socket_set_nodelay()
>>>>> - reset s->sock properly
>>>>>
>>>>> Changes from v8:
>>>>> - use a newer key type in iotest 207
>>>>> - improve the commit message
>>>>>
>>>>> Changes from v7:
>>>>> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
>>>>> - ptrdiff_t -> size_t
>>>>>
>>>>> Changes from v6:
>>>>> - fixed few checkpatch style issues
>>>>> - detect libssh 0.8 via symbol detection
>>>>> - adjust travis/docker test material
>>>>> - remove dead "default" case in a switch
>>>>> - use variables for storing MIN() results
>>>>> - adapt a documentation bit
>>>>>
>>>>> Changes from v5:
>>>>> - adapt to newer tracing APIs
>>>>> - disable ssh compression (mimic what libssh2 does by default)
>>>>> - use build time checks for libssh 0.8, and use newer APIs directly
>>>>>
>>>>> Changes from v4:
>>>>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
>>>>> - fix few return code checks
>>>>> - remove now-unused parameters in few internal functions
>>>>> - allow authentication with "none" method
>>>>> - switch to unsigned int for the port number
>>>>> - enable TCP_NODELAY on the socket
>>>>> - fix one reference error message in iotest 207
>>>>>
>>>>> Changes from v3:
>>>>> - fix socket cleanup in connect_to_ssh()
>>>>> - add comments about the socket cleanup
>>>>> - improve the error reporting (closer to what was with libssh2)
>>>>> - improve EOF detection on sftp_read()
>>>>>
>>>>> Changes from v2:
>>>>> - used again an own fd
>>>>> - fixed co_yield() implementation
>>>>>
>>>>> Changes from v1:
>>>>> - fixed jumbo packets writing
>>>>> - fixed missing 'err' assignment
>>>>> - fixed commit message
>>>>>
>>>>>  .travis.yml                                   |   4 +-
>>>>>  block/Makefile.objs                           |   6 +-
>>>>>  block/ssh.c                                   | 665 ++++++++++--------
>>>>>  block/trace-events                            |  14 +-
>>>>>  configure                                     |  65 +-
>>>>>  docs/qemu-block-drivers.texi                  |   2 +-
>>>>>  .../dockerfiles/debian-win32-cross.docker     |   1 -
>>>>>  .../dockerfiles/debian-win64-cross.docker     |   1 -
>>>>>  tests/docker/dockerfiles/fedora.docker        |   4 +-
>>>>>  tests/docker/dockerfiles/ubuntu.docker        |   2 +-
>>>>>  tests/docker/dockerfiles/ubuntu1804.docker    |   2 +-
>>>>>  tests/qemu-iotests/207                        |   4 +-
>>>>>  tests/qemu-iotests/207.out                    |   2 +-
>>>>>  13 files changed, 423 insertions(+), 349 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/block/ssh.c b/block/ssh.c
>>>>> index 6da7b9cbfe..644ae8b82c 100644
>>>>> --- a/block/ssh.c
>>>>> +++ b/block/ssh.c
>>>>
>>>> [...]
>>>>
>>>>> +    case SSH_SERVER_KNOWN_CHANGED:
>>>>> +        ret = -EINVAL;
>>>>> +        r = ssh_get_publickey(s->session, &pubkey);
>>>>> +        if (r == 0) {
>>>>> +            r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA1,
>>>>> +                                       &server_hash, &server_hash_len);
>>>>> +            pubkey_type = ssh_key_type(pubkey);
>>>>> +            ssh_key_free(pubkey);
>>>>> +        }
>>>>> +        if (r == 0) {
>>>>> +            fingerprint = 
>>>>> ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
>>>>> +                                                   server_hash,
>>>>> +                                                   server_hash_len);
>>>>> +            ssh_clean_pubkey_hash(&server_hash);
>>>>> +        }
>>>>> +        if (fingerprint) {
>>>>> +            error_setg(errp,
>>>>> +                       "host key (%s key with fingerprint %s) does not 
>>>>> match "
>>>>> +                       "the one in known_hosts",
>>>>> +                       ssh_key_type_to_char(pubkey_type), fingerprint);
>>>>> +            ssh_string_free_char(fingerprint);
>>>>> +        } else  {
>>>>> +            error_setg(errp, "host key does not match the one in 
>>>>> known_hosts");
>>>>> +        }
>>>>
>>>> Usually I’d say that more information can’t be bad.  But here I don’t
>>>> see how this additional information is useful.  known_hosts contains
>>>> base64-encoded full public keys.  This only prints the SHA-1 digest.
>>>
>>> Note that SHA-1 is printed with libssh < 0.8; with libssh >= 0.8 SHA-256
>>> is used instead.
>>>
>>>> The user cannot add that to known_hosts, or easily scan known_hosts to
>>>> see whether it perhaps belongs to some other hosts (maybe because DHCP
>>>> scrambled something).
>>>>
>>>> Actually, even if this printed the base64 representation of the full
>>>> key, the user probably wouldn’t just add that to known_hosts or do
>>>> anything with it.  They’ll debug the problem with other tools.
>>>>
>>>> So I don’t think this new information is really useful...?
>>>
>>> When this situation happens with openssh, the big scary message prints
>>> three things:
>>> 1) the fingerprint of the server
>>> 2) the line of the server in the known_hosts file
>>> 3) how to remove the keys of the server from known_hosts, i.e. a
>>>    copy-paste'able `ssh-keygen -R host`
>>>
>>> Here I'm doing only (1).  Also, the current libssh2 driver does
>>> something else, i.e. print the base64/printable representation of the
>>> server key in known_hosts.
>>
>> Well, I don’t know whether it’s necessary to reproduce the exact message
>> with all the data it contains.  As I said, I think users can and will
>> investigate the exact root of the problem with tools outside of qemu.
>> (Such as openssh’s ssh itself.)
>>
>>>> (Hmm, I don’t know if this is a response to my “But the specification
>>>> requires a warning!!1!”, but if so, I was actually not referring to more
>>>> information but to this:
>>>
>>> You mentioned this few times: can you please point me to this bit?
>>> I read few RFCs related to ssh, and I did not find this information...
>>
>> For example:
>> http://api.libssh.org/master/group__libssh__session.html#gacbc5d04fe66beee863a0c61a93fdf765
>>> SSH_KNOWN_HOSTS_CHANGED: The server key has changed. Either you are under 
>>> attack or the administrator changed the key. You HAVE to warn the user 
>>> about a possible attack.
> 
> Ah, now I see what you mean! I thought that "libssh specification" was
> any of the SSH RFCs, and that's why I did not find what you meant.
> I see you were talking about the libssh API documentation :-)
> (Never heard the API docs of a library called as "specification" before,
> TBH.)

Ah, sorry.  I have no excuse.  I just call everything a “spec” that’s
telling me what to do.  (I should probably stop doing that.)

>> This doesn’t require any specific formatting or data to be given to the
>> user.  All it requires is “to warn the user about a possible attack”.
>> That can be as simple as appending “This may be due to a
>> man-in-the-middle attack” to the error message.
> 
> Makes sense -- I just asked to the libssh people, and appending
> "this may be a possible attack" should be enough, especially that this
> is not a UI message like the one written by the ssh client.

OK, great!

>>>> $ ssh 192.168.0.12
>>>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>>>> @    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
>>>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>>>> IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
>>>> Someone could be eavesdropping on you right now (man-in-the-middle attack)!
>>>> It is also possible that a host key has just been changed.
>>>>
>>>>
>>>> I mean, I was also only half-serious.  I should be serious because the
>>>> libssh specification requires us to print some warning like that, but,
>>>> well.  Yes, I’ll stop mumbling about this stuff now.)
>>>
>>> To be on the explic side: are you asking to basically put the first 6
>>> lines of the openssh error message (as you quoted them above) as error
>>> message in the ssh driver?
>>
>> God forbid no.  I was just making an example of “Here is a warning about
>> a possible attack”.  I fully agree with Dan (and probably you) that this
>> is completely unsuitable to qemu’s interface.
>>
>> Sorry if that came across in another way.
> 
> Not a problem. I preferred to ask explicitly to make sure to get it
> right -- any amount of information shown is fine for me, I want to make
> sure to follow QEMU best practices (if any).

I mean, as you’ve seen yourself, there currently is no warning.  So it’s
not like there is any practice, not to mention a best one...

Max

signature.asc
Description: OpenPGP digital signature