Missing check for all-zero Curve25519 KEX shared secret

Hi. I noticed that libssh's curve25519 KEX implementation does not appear to check that the resulting DH shared secret is all zero when built without OpenSSL. Or am I missing the check?

Non-OpenSSL code is here:

https://gitlab.com/libssh/libssh-mirror/-/blob/83ce7bfa590da23fc5bcff15774beedb325f64e7/src/curve25519.c#L255

Compare OpenSSH:

https://github.com/openssh/openssh-portable/blob/c88a8788f9865d02b986d00405b9f0be65ad0b5a/kexc25519.c#L68

The OpenSSL code-path uses EVP_PKEY_derive:

https://gitlab.com/libssh/libssh-mirror/-/blob/83ce7bfa590da23fc5bcff15774beedb325f64e7/src/curve25519.c#L240

Which compares against all-zero key:

https://github.com/openssl/openssl/blob/a1c87f64dd6d6b0f1c8b276dc415f69e1102f930/crypto/ec/curve25519.c#L5681

So only non-OpenSSL builds are affected.

RFC 8731 says in https://www.rfc-editor.org/rfc/rfc8731.html#section-3 the following:

The key-agreement schemes "curve25519-sha256" and "curve448-sha512" perform the Diffie-Hellman protocol using the functions X25519 and X448, respectively. Implementations SHOULD compute these functions using the algorithms described in [RFC7748]. When they do so, implementations MUST check whether the computed Diffie-Hellman shared secret is the all-zero value and abort if so, as described in Section 6 of [RFC7748].

The Curve25519 RFC merely suggests these checks: https://www.rfc-editor.org/rfc/rfc7748

There are arguments against adding the check -- https://moderncrypto.org/mail-archive/curves/2017/000896.html -- however, I think that any SSH connection that attempts to use an all-zero shared secret should not be used, so I'm leaning towards actually adding the check. This also makes this compliant with RFC 8731, for whatever standard compliance is worth.

How about a patch like this?

diff --git a/src/curve25519.c b/src/curve25519.c
index 26603681..86c2cb02 100644
--- a/src/curve25519.c
+++ b/src/curve25519.c
@@ -253,12 +253,24 @@ out:
         return ret;
     }
 #else
-    if (session->server) {
-        crypto_scalarmult(k, session->next_crypto->curve25519_privkey,
-                          session->next_crypto->curve25519_client_pubkey);
-    } else {
-        crypto_scalarmult(k, session->next_crypto->curve25519_privkey,
-                          session->next_crypto->curve25519_server_pubkey);
+    {
+      u_char zero[CURVE25519_PUBKEY_SIZE];
+
+      if (session->server) {
+	crypto_scalarmult(k, session->next_crypto->curve25519_privkey,
+			  session->next_crypto->curve25519_client_pubkey);
+      } else {
+	crypto_scalarmult(k, session->next_crypto->curve25519_privkey,
+			  session->next_crypto->curve25519_server_pubkey);
+      }
+
+      /* Check for all-zero shared secret */
+      explicit_bzero(zero, CURVE25519_PUBKEY_SIZE);
+      if (secure_memcmp(zero, k, CURVE25519_PUBKEY_SIZE) == 0)
+	{
+	  SSH_LOG(SSH_LOG_TRACE, "Shared curve25519 secret is all zero");
+	  return SSH_ERROR;
+	}
     }
 #endif /* HAVE_LIBCRYPTO */