Bugs when using rsa-sha2 (+patches)

[Tilo Eckert <tilo.eckert@xxxxxxx>]

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.

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 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.

I ended up changing this line in libssh/tests/CMakeLists.txt:
> set(CHROOT_WRAPPER_LIBRARY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}chroot_wrapper${CMAKE_SHARED_LIBRARY_SUFFIX})
to
> set(CHROOT_WRAPPER_LIBRARY ${CMAKE_CURRENT_BINARY_DIR}/../lib/${CMAKE_SHARED_LIBRARY_PREFIX}chroot_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?

Regards,
Tilo

From 21d6f4d22f8bc914a815e9e5315e4da0944f5cda Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 12:21:53 +0100
Subject: [PATCH 1/8] packet: Fix timeout on hostkey type mismatch instead of
 proper error

If the hostkey type was not in the list of acceptable hostkey
types, the function failed to set the error state. Due to the
fact that the calling function ssh_packet_process() does not
handle the SSH_ERROR return code, the newkeys packet from the
server was silently ignored, stalling the connection until a
timeout occurred.

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/packet_cb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/packet_cb.c b/src/packet_cb.c
index dc883244..af5b966c 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -198,7 +198,7 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
                           "preference (%s)",
                           server_key->type_c,
                           session->opts.wanted_methods[SSH_HOSTKEYS]);
-            return -1;
+            goto error;
         }
     }
 
-- 
2.18.0

From 6ded3a15153b683a8f54df1df4222eb17ae8a20e Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:45:47 +0100
Subject: [PATCH 2/8] pki: Add hostkey type validation function

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h |  1 +
 src/pki.c            | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index 241cfdd1..af56f11a 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -140,4 +140,5 @@ ssh_public_key ssh_pki_convert_key_to_publickey(const ssh_key key);
 ssh_private_key ssh_pki_convert_key_to_privatekey(const ssh_key key);
 
 int ssh_key_algorithm_allowed(ssh_session session, const char *type);
+int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type);
 #endif /* PKI_H_ */
diff --git a/src/pki.c b/src/pki.c
index d420e792..8fe39532 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -295,6 +295,43 @@ int ssh_key_algorithm_allowed(ssh_session session, const char *type)
     return ssh_match_group(allowed_list, type);
 }
 
+/**
+ * @brief Checks the given host key against the configured allowed
+ * host key algorithm types
+ *
+ * @param[in] session The SSH session
+ * @param[in] type    The host key algorithm to check
+ * @returns           1 if the host key algorithm is allowed, 0 otherwise
+ */
+int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type)
+{
+    const char *allowed_types;
+
+    allowed_types = session->opts.wanted_methods[SSH_HOSTKEYS];
+    if (allowed_types == NULL) {
+       return 0; /* should never get here */
+    }
+
+    if (ssh_match_group(allowed_types, type)) {
+        return 1;
+    }
+    if (strcmp(type, "ssh-rsa") == 0) {
+        /*
+         * If rsa-sha2 hostkey algo is used, the server returns ssh-rsa as
+         * host key type. Hence, we need to check if one of the rsa-sha2
+         * variants is in the allowed hostkey types list.
+         */
+        if (ssh_match_group(allowed_types, "rsa-sha2-256")) {
+            return 1;
+        }
+        if (ssh_match_group(allowed_types, "rsa-sha2-512")) {
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 /**
  * @brief Convert a key type to a hash type. This is usually unambiguous
  * for all the key types, unless the SHA2 extension (RFC 8332) is
-- 
2.18.0

From 8cae94aa44213a3f9ad5e96b6e6ca916516ee114 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:49:48 +0100
Subject: [PATCH 3/8] packet: Use new hostkey type validation function

The new function accepts hostkeys of type ssh-rsa
if rsa-sha2-256/512 is the only accepted hostkey type

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/packet_cb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/packet_cb.c b/src/packet_cb.c
index af5b966c..784ca33d 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -190,8 +190,7 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
 
     /* check if public key from server matches user preferences */
     if (session->opts.wanted_methods[SSH_HOSTKEYS]) {
-        if(!ssh_match_group(session->opts.wanted_methods[SSH_HOSTKEYS],
-                            server_key->type_c)) {
+        if (!ssh_hostkey_algorithm_allowed(session, server_key->type_c)) {
             ssh_set_error(session,
                           SSH_FATAL,
                           "Public key from server (%s) doesn't match user "
-- 
2.18.0

From 5ef7da9893ef4097be6894f156017a9a419264d2 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:50:48 +0100
Subject: [PATCH 4/8] tests: Fix rsa-sha2 tests

---
 tests/client/torture_hostkey.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/client/torture_hostkey.c b/tests/client/torture_hostkey.c
index 928137d5..a930a293 100644
--- a/tests/client/torture_hostkey.c
+++ b/tests/client/torture_hostkey.c
@@ -163,7 +163,7 @@ static void torture_hostkey_ecdsa(void **state) {
 static void torture_hostkey_rsa_sha256(void **state) {
     struct torture_state *s = *state;
     ssh_session session = s->ssh.session;
-    char rsa[] = "rsa-sha2-256,ssh-rsa";
+    char rsa[] = "rsa-sha2-256";
 
     int rc;
 
@@ -182,7 +182,7 @@ static void torture_hostkey_rsa_sha256(void **state) {
 static void torture_hostkey_rsa_sha512(void **state) {
     struct torture_state *s = *state;
     ssh_session session = s->ssh.session;
-    char rsa[] = "rsa-sha2-512,ssh-rsa";
+    char rsa[] = "rsa-sha2-512";
 
     int rc;
 
-- 
2.18.0

From 389f93f658b6ea855b0cba1525f61bd9a3dcf052 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:58:40 +0100
Subject: [PATCH 5/8] pki: Fix typos in documentation

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/pki.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/pki.c b/src/pki.c
index 8fe39532..232327fa 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -274,13 +274,14 @@ static enum ssh_digest_e ssh_key_hash_from_name(const char *name)
     /* we do not care for others now */
     return SSH_DIGEST_AUTO;
 }
+
 /**
  * @brief Checks the given key against the configured allowed
  * public key algorithm types
  *
  * @param[in] session The SSH session
- * @parma[in] type    The key algorithm to check
- * @returns           1 if the key algorithm is allowed 0 otherwise
+ * @param[in] type    The key algorithm to check
+ * @returns           1 if the key algorithm is allowed, 0 otherwise
  */
 int ssh_key_algorithm_allowed(ssh_session session, const char *type)
 {
-- 
2.18.0

From 6963dc53e22c006a9df0b1b447d7729fdf4e4196 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Thu, 15 Nov 2018 10:37:20 +0100
Subject: [PATCH 6/8] socket: Fix potential buffer overrun

If nread is < 0 and no exception callback is set,
the following code block would cause a buffer overrun.

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/socket.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/socket.c b/src/socket.c
index 8c3e68ec..6012c46e 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -270,12 +270,8 @@ int ssh_socket_pollcallback(struct ssh_poll_handle_struct *p,
                 s->callbacks->exception(SSH_SOCKET_EXCEPTION_ERROR,
                                         s->last_errno,
                                         s->callbacks->userdata);
-
-                /* p may have been freed, so don't use it
-                 * anymore in this function */
-                p = NULL;
-                return -2;
             }
+            return -2;
         }
         if (nread == 0) {
             if (p != NULL) {
@@ -288,12 +284,8 @@ int ssh_socket_pollcallback(struct ssh_poll_handle_struct *p,
                 s->callbacks->exception(SSH_SOCKET_EXCEPTION_EOF,
                                         0,
                                         s->callbacks->userdata);
-
-                /* p may have been freed, so don't use it
-                 * anymore in this function */
-                p = NULL;
-                return -2;
             }
+            return -2;
         }
 
         if (s->session->socket_counter != NULL) {
-- 
2.18.0

From 290714d1fa8c5d449ce7077a9036b9aa5628f2e0 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Thu, 15 Nov 2018 10:37:30 +0100
Subject: [PATCH 7/8] socket: Add missing braces

---
 src/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/socket.c b/src/socket.c
index 6012c46e..20831185 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -236,7 +236,7 @@ int ssh_socket_pollcallback(struct ssh_poll_handle_struct *p,
             (revents & POLLOUT) ? "POLLOUT ":"",
             (revents & POLLERR) ? "POLLERR":"",
             ssh_buffer_get_len(s->out_buffer));
-    if (revents & POLLERR || revents & POLLHUP) {
+    if ((revents & POLLERR) || (revents & POLLHUP)) {
         /* Check if we are in a connecting state */
         if (s->state == SSH_SOCKET_CONNECTING) {
             s->state = SSH_SOCKET_ERROR;
-- 
2.18.0

From 0d403ccc3bede3e1bc68141d0e2922ff2e3f5780 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Thu, 15 Nov 2018 10:39:05 +0100
Subject: [PATCH 8/8] socket: Remove redundant code

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/socket.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/socket.c b/src/socket.c
index 20831185..9823d8e1 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -274,9 +274,6 @@ int ssh_socket_pollcallback(struct ssh_poll_handle_struct *p,
             return -2;
         }
         if (nread == 0) {
-            if (p != NULL) {
-                ssh_poll_remove_events(p, POLLIN);
-            }
             if (p != NULL) {
                 ssh_poll_remove_events(p, POLLIN);
             }
-- 
2.18.0