meduketto / iksemel

Automatically exported from code.google.com/p/iksemel
GNU Lesser General Public License v2.1
31 stars 24 forks source link

utterly insecure GNUTLS settings #48

Open onlyjob opened 8 years ago

onlyjob commented 8 years ago

As reported in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=803204:

Since I changed my XMPP server, Zabbix failed to send alerts via XMPP with "tls handshake failed". The XMPP server said "no shared cipher". After some research to see how Zabbix do its job I ended up into this library. I confirmed there is no way to setup the ciphers into Zabbix, but I was then astonished to see them hardcoded and very low grade in libiksemel:

       const int protocol_priority[] = { GNUTLS_TLS1, GNUTLS_SSL3, 0 };
       const int kx_priority[] = { GNUTLS_KX_RSA, 0 };
       const int cipher_priority[] = { GNUTLS_CIPHER_3DES_CBC, 
GNUTLS_CIPHER_ARCFOUR, 0};
       const int comp_priority[] = { GNUTLS_COMP_ZLIB, GNUTLS_COMP_NULL, 
0 };
       const int mac_priority[] = { GNUTLS_MAC_SHA, GNUTLS_MAC_MD5, 0 };

SSL3, 3DES, RC4, SSL compression… With this setting not only low grade ciphers are available, but higher grades are disabled. So this is a major security issue, also affecting stable.

The following patch fixes the security problem (and compatibility problem with servers rejecting low grade ciphers). You should nevertheless proofread my choices, as I'm no security expert. The patch does not change the original priority lists because I failed somehow to fix them all, so I replaced it by a priority string (which is a non-obsolete method to do it anyway).

Index: libiksemel-1.4/src/stream.c
===================================================================
--- libiksemel-1.4.orig/src/stream.c
+++ libiksemel-1.4/src/stream.c
@@ -63,11 +63,7 @@ tls_pull (iksparser *prs, char *buffer,
 static int
 handshake (struct stream_data *data)
 {
-   const int protocol_priority[] = { GNUTLS_TLS1, GNUTLS_SSL3, 0 };
-   const int kx_priority[] = { GNUTLS_KX_RSA, 0 };
-   const int cipher_priority[] = { GNUTLS_CIPHER_3DES_CBC, GNUTLS_CIPHER_ARCFOUR, 0};
-   const int comp_priority[] = { GNUTLS_COMP_ZLIB, GNUTLS_COMP_NULL, 0 };
-   const int mac_priority[] = { GNUTLS_MAC_SHA, GNUTLS_MAC_MD5, 0 };
+   const char *priority_string = "SECURE256:+SECURE192:-VERS-TLS-ALL:+VERS-TLS1.2";
    int ret;

    if (gnutls_global_init () != 0)
@@ -80,11 +76,7 @@ handshake (struct stream_data *data)
        gnutls_certificate_free_credentials (data->cred);
        return IKS_NOMEM;
    }
-   gnutls_protocol_set_priority (data->sess, protocol_priority);
-   gnutls_cipher_set_priority(data->sess, cipher_priority);
-   gnutls_compression_set_priority(data->sess, comp_priority);
-   gnutls_kx_set_priority(data->sess, kx_priority);
-   gnutls_mac_set_priority(data->sess, mac_priority);
+   gnutls_priority_set_direct(data->sess, priority_string, NULL);
    gnutls_credentials_set (data->sess, GNUTLS_CRD_CERTIFICATE, data->cred);

    gnutls_transport_set_push_function (data->sess, (gnutls_push_func) tls_push);

Regards. Marc Dequènes

chris001 commented 8 years ago

Paging @meduketto I think you should pull @onlyjob Marc Dequenes security fix into the main branch of this jabber code, mate..