aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDirkjan Bussink <d.bussink@gmail.com>2020-09-29 15:14:09 +0200
committerJakub Jelen <jjelen@redhat.com>2020-10-14 11:07:40 +0200
commit42741b18832aa8acab51c53505efa263e8193537 (patch)
treee0261526c7b7b826120431416302e70389732a1c
parent39cbe8178e94279282ca3a045ab126baaea2bb31 (diff)
downloadlibssh-42741b18832aa8acab51c53505efa263e8193537.tar.gz
libssh-42741b18832aa8acab51c53505efa263e8193537.tar.xz
libssh-42741b18832aa8acab51c53505efa263e8193537.zip
Fix handshake bug with AEAD ciphers and no HMAC overlap
There's currently a bug in libssh that a handshake doesn't complete if there is no overlap between HMAC methods, but when an AEAD cipher is used. In case of an AEAD cipher such as chacha20-poly1305 or aes256-gcm, the HMAC algorithm that is being picked is not relevant. But the problem here is that the HMAC still needs to have an overlap in the handshake, even if it is not used afterwards. This was found with a very strict server side configuration with libssh where only AEAD ciphers and EtM HMAC modes are accepted. The client tested against was dropbear. Dropbear does have support for chacha20-poly1305 and AES GCM modes, but no support for EtM HMAC modes. This meant that the libssh server in this case rejected the dropbear client, even though it is perfectly able to serve it since dropbear supports AEAD algorithms. The fix implemented here updates the HMAC phase of the handshake to handle this case. If it detects an AEAD cipher is used, it uses the HMAC abbreviations for the method instead. This is the same name that is used in other places as well. It matches the client to server and server to client values, but it does depend on the order of things in the ssh_kex_types_e enum, which I'm assuming here is ok since it's explicit. I've looked at how to add a test for this, but I couldn't really find a suitable place for it. I would love some tips if this is easily possible, or if it's easier for someone else to contribute, that's of course welcome too. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Reviewed-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com> Reviewed-by: Jakub Jelen <jjelen@redhat.com>
-rw-r--r--src/kex.c57
1 files changed, 41 insertions, 16 deletions
diff --git a/src/kex.c b/src/kex.c
index 1df22830..95948136 100644
--- a/src/kex.c
+++ b/src/kex.c
@@ -755,13 +755,29 @@ int ssh_set_client_kex(ssh_session session)
return SSH_OK;
}
+static const char *ssh_find_aead_hmac(const char *cipher)
+{
+ if (cipher == NULL) {
+ return NULL;
+ } else if (strcmp(cipher, "chacha20-poly1305@openssh.com") == 0) {
+ return "aead-poly1305";
+ } else if (strcmp(cipher, "aes256-gcm@openssh.com") == 0) {
+ return "aead-gcm";
+ } else if (strcmp(cipher, "aes128-gcm@openssh.com") == 0) {
+ return "aead-gcm";
+ }
+ return NULL;
+}
+
/** @brief Select the different methods on basis of client's and
* server's kex messages, and watches out if a match is possible.
*/
-int ssh_kex_select_methods (ssh_session session){
+int ssh_kex_select_methods (ssh_session session)
+{
struct ssh_kex_struct *server = &session->next_crypto->server_kex;
struct ssh_kex_struct *client = &session->next_crypto->client_kex;
char *ext_start = NULL;
+ const char *aead_hmac = NULL;
int i;
/* Here we should drop the ext-info-c from the list so we avoid matching.
@@ -773,7 +789,15 @@ int ssh_kex_select_methods (ssh_session session){
for (i = 0; i < SSH_KEX_METHODS; i++) {
session->next_crypto->kex_methods[i]=ssh_find_matching(server->methods[i],client->methods[i]);
- if(session->next_crypto->kex_methods[i] == NULL && i < SSH_LANG_C_S){
+
+ if (i == SSH_MAC_C_S || i == SSH_MAC_S_C) {
+ aead_hmac = ssh_find_aead_hmac(session->next_crypto->kex_methods[i-2]);
+ if (aead_hmac) {
+ free(session->next_crypto->kex_methods[i]);
+ session->next_crypto->kex_methods[i] = strdup(aead_hmac);
+ }
+ }
+ if (session->next_crypto->kex_methods[i] == NULL && i < SSH_LANG_C_S){
ssh_set_error(session,SSH_FATAL,"kex error : no match for method %s: server [%s], client [%s]",
ssh_kex_descriptions[i],server->methods[i],client->methods[i]);
return SSH_ERROR;
@@ -782,31 +806,31 @@ int ssh_kex_select_methods (ssh_session session){
session->next_crypto->kex_methods[i] = strdup("");
}
}
- if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group1-sha1") == 0){
+ if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group1-sha1") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GROUP1_SHA1;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group14-sha1") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group14-sha1") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GROUP14_SHA1;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group14-sha256") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group14-sha256") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GROUP14_SHA256;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group16-sha512") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group16-sha512") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GROUP16_SHA512;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group18-sha512") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group18-sha512") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GROUP18_SHA512;
#ifdef WITH_GEX
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group-exchange-sha1") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group-exchange-sha1") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GEX_SHA1;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group-exchange-sha256") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "diffie-hellman-group-exchange-sha256") == 0){
session->next_crypto->kex_type=SSH_KEX_DH_GEX_SHA256;
#endif /* WITH_GEX */
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp256") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp256") == 0){
session->next_crypto->kex_type=SSH_KEX_ECDH_SHA2_NISTP256;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp384") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp384") == 0){
session->next_crypto->kex_type=SSH_KEX_ECDH_SHA2_NISTP384;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp521") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "ecdh-sha2-nistp521") == 0){
session->next_crypto->kex_type=SSH_KEX_ECDH_SHA2_NISTP521;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "curve25519-sha256@libssh.org") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "curve25519-sha256@libssh.org") == 0){
session->next_crypto->kex_type=SSH_KEX_CURVE25519_SHA256_LIBSSH_ORG;
- } else if(strcmp(session->next_crypto->kex_methods[SSH_KEX], "curve25519-sha256") == 0){
+ } else if (strcmp(session->next_crypto->kex_methods[SSH_KEX], "curve25519-sha256") == 0){
session->next_crypto->kex_type=SSH_KEX_CURVE25519_SHA256;
}
SSH_LOG(SSH_LOG_INFO, "Negotiated %s,%s,%s,%s,%s,%s,%s,%s,%s,%s",
@@ -826,7 +850,8 @@ int ssh_kex_select_methods (ssh_session session){
/* this function only sends the predefined set of kex methods */
-int ssh_send_kex(ssh_session session, int server_kex) {
+int ssh_send_kex(ssh_session session, int server_kex)
+{
struct ssh_kex_struct *kex = (server_kex ? &session->next_crypto->server_kex :
&session->next_crypto->client_kex);
ssh_string str = NULL;
@@ -1043,7 +1068,7 @@ int ssh_make_sessionid(ssh_session session)
ssh_buffer_get(server_hash),
server_pubkey_blob);
SSH_STRING_FREE(server_pubkey_blob);
- if(rc != SSH_OK){
+ if (rc != SSH_OK){
goto error;
}