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

[Max Reitz]

On 20.06.19 12:11, Daniel P. Berrangé wrote:
> On Tue, Jun 18, 2019 at 03:14:30PM +0200, 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.
>> 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...?
>>
>>
>> (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:
>>
>> $ 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.)
> 
> Those giant messages are targetted at humans though so I'm not so
> convinced it is useful for apps managing QEMU. IMHO it is sufficient
> to just report a simple error that the host ident check failed as
> the patch does now. It gives enough info to then investigate further
> to identify the root cause problems.

I fully agree that that message is absolutely unsuitable for qemu.  I
didn’t state that, which is my mistake, sorry.

The point I wanted to make is that I’m citing something that doesn’t
print any data, but just an attack warning.  I’d be content with just a
single sentence to that effect (and please not in caps lock), like “It
is possible that someone is attempting a man-in-the-middle attack.”

Max

signature.asc
Description: OpenPGP digital signature