Revert "crypto: implement workaround for GNUTLS thread safety problems"

This reverts commit 24ad5e1995.

The workaround for the GNUTLS bug was not correct and it was
decided to rely on GNUTLS being patched by distros instead,
or users manually disabling use of the problematic ciphers.

Acked-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

crypto/tlssession.c

@@ -19,7 +19,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/thread.h"
 #include "crypto/tlssession.h"
 #include "crypto/tlscredsanon.h"
 #include "crypto/tlscredspsk.h"
@@ -53,14 +52,6 @@ struct QCryptoTLSSession {
      */
     Error *rerr;
     Error *werr;
-
-    /*
-     * Used to protect against broken GNUTLS thread safety
-     * https://gitlab.com/gnutls/gnutls/-/issues/1717
-     */
-    bool requireThreadSafety;
-    bool lockEnabled;
-    QemuMutex lock;
 };
 
 
@@ -80,7 +71,6 @@ qcrypto_tls_session_free(QCryptoTLSSession *session)
     g_free(session->authzid);
     qcrypto_tls_creds_box_unref(session->credsbox);
     object_unref(OBJECT(session->creds));
-    qemu_mutex_destroy(&session->lock);
     g_free(session);
 }
 
@@ -96,19 +86,10 @@ qcrypto_tls_session_push(void *opaque, const void *buf, size_t len)
         return -1;
     };
 
-    if (session->lockEnabled) {
-        qemu_mutex_unlock(&session->lock);
-    }
-
     error_free(session->werr);
     session->werr = NULL;
 
     ret = session->writeFunc(buf, len, session->opaque, &session->werr);
-
-    if (session->lockEnabled) {
-        qemu_mutex_lock(&session->lock);
-    }
-
     if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
         errno = EAGAIN;
         return -1;
@@ -135,16 +116,7 @@ qcrypto_tls_session_pull(void *opaque, void *buf, size_t len)
     error_free(session->rerr);
     session->rerr = NULL;
 
-    if (session->lockEnabled) {
-        qemu_mutex_unlock(&session->lock);
-    }
-
     ret = session->readFunc(buf, len, session->opaque, &session->rerr);
-
-    if (session->lockEnabled) {
-        qemu_mutex_lock(&session->lock);
-    }
-
     if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
         errno = EAGAIN;
         return -1;
@@ -181,8 +153,6 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     session->creds = creds;
     object_ref(OBJECT(creds));
 
-    qemu_mutex_init(&session->lock);
-
     if (creds->endpoint != endpoint) {
         error_setg(errp, "Credentials endpoint doesn't match session");
         goto error;
@@ -247,11 +217,6 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     return NULL;
 }
 
-void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess)
-{
-    sess->requireThreadSafety = true;
-}
-
 static int
 qcrypto_tls_session_check_certificate(QCryptoTLSSession *session,
                                       Error **errp)
@@ -438,17 +403,7 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
                           size_t len,
                           Error **errp)
 {
-    ssize_t ret;
-
-    if (session->lockEnabled) {
-        qemu_mutex_lock(&session->lock);
-    }
-
-    ret = gnutls_record_send(session->handle, buf, len);
-
-    if (session->lockEnabled) {
-        qemu_mutex_unlock(&session->lock);
-    }
+    ssize_t ret = gnutls_record_send(session->handle, buf, len);
 
     if (ret < 0) {
         if (ret == GNUTLS_E_AGAIN) {
@@ -476,17 +431,7 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
                          size_t len,
                          Error **errp)
 {
-    ssize_t ret;
-
-    if (session->lockEnabled) {
-        qemu_mutex_lock(&session->lock);
-    }
-
-    ret = gnutls_record_recv(session->handle, buf, len);
-
-    if (session->lockEnabled) {
-        qemu_mutex_unlock(&session->lock);
-    }
+    ssize_t ret = gnutls_record_recv(session->handle, buf, len);
 
     if (ret < 0) {
         if (ret == GNUTLS_E_AGAIN) {
@@ -556,29 +501,8 @@ int
 qcrypto_tls_session_handshake(QCryptoTLSSession *session,
                               Error **errp)
 {
-    int ret;
-    ret = gnutls_handshake(session->handle);
-
+    int ret = gnutls_handshake(session->handle);
     if (!ret) {
-#ifdef CONFIG_GNUTLS_BUG1717_WORKAROUND
-        gnutls_cipher_algorithm_t cipher =
-            gnutls_cipher_get(session->handle);
-
-        /*
-         * Any use of rekeying in TLS 1.3 is unsafe for
-         * a gnutls with bug 1717, however, we know that
-         * QEMU won't initiate manual rekeying. Thus we
-         * only have to protect against automatic rekeying
-         * which doesn't trigger with CHACHA20
-         */
-        if (session->requireThreadSafety &&
-            gnutls_protocol_get_version(session->handle) ==
-            GNUTLS_TLS1_3 &&
-            cipher != GNUTLS_CIPHER_CHACHA20_POLY1305) {
-            session->lockEnabled = true;
-        }
-#endif
-
         session->handshakeComplete = true;
         return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
     }
@@ -616,15 +540,8 @@ qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
         return 0;
     }
 
-    if (session->lockEnabled) {
-        qemu_mutex_lock(&session->lock);
-    }
     ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR);
 
-    if (session->lockEnabled) {
-        qemu_mutex_unlock(&session->lock);
-    }
-
     if (!ret) {
         return QCRYPTO_TLS_BYE_COMPLETE;
     }
@@ -690,9 +607,6 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds G_GNUC_UNUSED,
     return NULL;
 }
 
-void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess)
-{
-}
 
 void
 qcrypto_tls_session_free(QCryptoTLSSession *sess G_GNUC_UNUSED)

include/crypto/tlssession.h

@@ -166,20 +166,6 @@ void qcrypto_tls_session_free(QCryptoTLSSession *sess);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, qcrypto_tls_session_free)
 
-/**
- * qcrypto_tls_session_require_thread_safety:
- * @sess: the TLS session object
- *
- * Mark that this TLS session will require thread safety
- * for concurrent I/O in both directions. This must be
- * called before the handshake is performed.
- *
- * This will activate a workaround for GNUTLS thread
- * safety issues, where appropriate for the negotiated
- * TLS session parameters.
- */
-void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess);
-
 /**
  * qcrypto_tls_session_check_credentials:
  * @sess: the TLS session object

meson.build

@@ -1795,18 +1795,11 @@ endif
 
 gnutls = not_found
 gnutls_crypto = not_found
-gnutls_bug1717_workaround = false
 if get_option('gnutls').enabled() or (get_option('gnutls').auto() and have_system)
   gnutls = dependency('gnutls', version: '>=3.7.5',
                       method: 'pkg-config',
                       required: get_option('gnutls'))
   gnutls_crypto = gnutls
-
-  #if gnutls.found() and not get_option('gnutls-bug1717-workaround').disabled()
-    # XXX: when bug 1717 is resolved, add logic to probe for
-    # the GNUTLS fixed version number to handle the 'auto' case
-  #  gnutls_bug1717_workaround = true
-  #endif
 endif
 
 # We prefer use of gnutls for crypto, unless the options
@@ -2533,7 +2526,6 @@ config_host_data.set('CONFIG_KEYUTILS', keyutils.found())
 config_host_data.set('CONFIG_GETTID', has_gettid)
 config_host_data.set('CONFIG_GNUTLS', gnutls.found())
 config_host_data.set('CONFIG_GNUTLS_CRYPTO', gnutls_crypto.found())
-config_host_data.set('CONFIG_GNUTLS_BUG1717_WORKAROUND', gnutls_bug1717_workaround)
 config_host_data.set('CONFIG_TASN1', tasn1.found())
 config_host_data.set('CONFIG_GCRYPT', gcrypt.found())
 config_host_data.set('CONFIG_NETTLE', nettle.found())
@@ -4906,7 +4898,6 @@ summary_info += {'TLS priority':      get_option('tls_priority')}
 summary_info += {'GNUTLS support':    gnutls}
 if gnutls.found()
   summary_info += {'  GNUTLS crypto':   gnutls_crypto.found()}
-  summary_info += {'  GNUTLS bug 1717 workaround': gnutls_bug1717_workaround }
 endif
 summary_info += {'libgcrypt':         gcrypt}
 summary_info += {'nettle':            nettle}