Re: Bugs when using rsa-sha2 (+patches)

[Jakub Jelen <jjelen@xxxxxxxxxx>]

On Thu, 2018-11-15 at 11:09 +0100, Tilo Eckert wrote:
> Hi
> 
> a series of patches is attached.
> 
> There are two bugs when restricting wanted hostkeys to rsa-sha2
> through:
> > ssh_options_set(session, SSH_OPTIONS_HOSTKEYS, "rsa-sha2-512,rsa-
> > sha2-256");
> 
> When connecting to an OpenSSH server that supports rsa-sha2 and the
> permitted hostkeys are restricted like above, every connection
> attempt
> resulted in a timeout.
> 
> This had two reasons:
> 
> 1) The hostkey returned by the server does not have the type
> "rsa-sha2-512" or "rsa-sha2-256", but "ssh-rsa", which causes the
> callback function ssh_packet_newkeys() to incorrectly fail at this
> line:
> > if(!ssh_match_group(session->opts.wanted_methods[SSH_HOSTKEYS],
> > server_key->type_c)) 
> A workaround is to use "rsa-sha2-512,rsa-sha2-256,ssh-rsa". However,
> this allows falling back to RSA with SHA1 if rsa-sha2 is not
> supported
> by the server, which might not be what the user wants.

Hello Tilo,
thank you for the investigation and patches. I planned to have a look
into this specific use case (disabling SHA1), but it is not yet
completely functional in OpenSSH either. The requirement to specify
ssh-rsa in addition to the SHA2 algorithms used to be a kind of known
limitation until recent OpenSSH release.

Looking at your approach, we should really check the type of the actual
signature, not of the public key type, which might be different. But
this would require more refactoring, because the verification is
encapsulated in ssh_pki_signature_verify_blob().

With your proposed patch, we are basically opening us to the downgrade
attack (the client request SHA2 signatures, the server sends the SHA1
signature and it is properly verified and accepted by the client),
similarly as it worked some time ago in OpenSSH (with client
authentication):

https://bugzilla.mindrot.org/show_bug.cgi?id=2799

The patches [1,5-8] look good to me, but the possibility to disable
SHA1, I would like to see solved "correctly" before we can apply [4].
Could you check if my proposal to check against signature type would be
feasible?

> 2) If ssh_packet_newkeys() fails with SSH_ERROR, this error is not
> handled by its caller ssh_packet_process(). The latter only handles
> SSH_PACKET_USED and otherwise ignores the packet, causing the poller
> to
> stall until it eventually times out. Additionally, the timeout error
> suppressed the real error message.
> 
> The attached patches fix both issues, associated tests and some
> smaller
> issues we found during debugging.
> 
> There is another occurrence of "return SSH_ERROR;" in the
> ssh_packet_newkeys() function. I am not sure, but I think it should
> be
> replaced with "goto error;" as well (+an error message).

I assume that this should do the same otherwise we will end up in the
same state as you described. The difference is that this case "should
never happen".

> I had some trouble testing with "-DCLIENT_TESTING=ON". I'm wondering
> why
> it actually works in the CI builds on Gitlab. All client tests failed
> for me because LD_PRELOAD failed. It attempted to load
> > <libsshdir>/build/tests/libchroot_wrapper.so
> instead of
> > <libsshdir>/build/lib/libchroot_wrapper.so
> where the library is actually located.

For me on Fedora 29, the lib is located in
<libssh>/build/tests/libchroot_wrapper.so -- could this be some
different configuration on your system?

> I ended up changing this line in libssh/tests/CMakeLists.txt:
> > set(CHROOT_WRAPPER_LIBRARY
> > ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}chroot_wr
> > apper${CMAKE_SHARED_LIBRARY_SUFFIX})
> to
> > set(CHROOT_WRAPPER_LIBRARY
> > ${CMAKE_CURRENT_BINARY_DIR}/../lib/${CMAKE_SHARED_LIBRARY_PREFIX}ch
> > root_wrapper${CMAKE_SHARED_LIBRARY_SUFFIX})
> to get client tests to work. Any idea why I had to make that change
> and
> why Gitlab CI tests work without it?

Thanks,
-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.