[glib-networking] Revert "Perform certificate verification during, not after, TLS handshake"

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 22c1879b773e4f9df5a1c3761b0845986b504caf
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Mon Aug 13 14:28:23 2018 -0500

    Revert "Perform certificate verification during, not after, TLS handshake"
    
    This reverts commit 45c5f335312680c12f7b7b41a9b323d1122bf0e9.
    
    Hopefully fixes #43. Something is wrong, and this needs to be reverted
    until I've figured out what.

 meson.build                        |   2 +-
 tls/gnutls/gtlsconnection-gnutls.c | 283 ++++++++++---------------------------
 2 files changed, 72 insertions(+), 213 deletions(-)
---
diff --git a/meson.build b/meson.build
index 30fb613..a09ee95 100644
--- a/meson.build
+++ b/meson.build
@@ -75,7 +75,7 @@ if enable_gnome_proxy_support
 endif
 
 # *** Checks for GnuTLS     ***
-gnutls_dep = dependency('gnutls', version: '>= 3.4.6', required: true)
+gnutls_dep = dependency('gnutls', version: '>= 3.4.4', required: true)
 
 # *** Checks for p11-kit  ***
 enable_pkcs11_support = get_option('pkcs11_support')
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 44adcf8..959fa68 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -117,14 +117,12 @@ static P11KitPin*    on_pin_prompt_callback  (const char     *pinfile,
 
 static void g_tls_connection_gnutls_init_priorities (void);
 
-static int verify_certificate_cb (gnutls_session_t session);
-
 static gboolean do_implicit_handshake (GTlsConnectionGnutls  *gnutls,
                                        gint64                 timeout,
                                        GCancellable          *cancellable,
                                        GError               **error);
 static gboolean finish_handshake (GTlsConnectionGnutls  *gnutls,
-                                  GTask                 *task,
+                                  GTask                 *thread_task,
                                   GError               **error);
 
 enum
@@ -178,11 +176,8 @@ typedef struct
 
   GTlsCertificate *certificate, *peer_certificate;
   GTlsCertificateFlags peer_certificate_errors;
-
-  GMutex verify_certificate_mutex;
-  GCond verify_certificate_condition;
-  gboolean peer_certificate_accepted;
-  gboolean peer_certificate_examined;
+  GTlsCertificate *peer_certificate_tmp;
+  GTlsCertificateFlags peer_certificate_errors_tmp;
 
   gboolean require_close_notify;
   GTlsRehandshakeMode rehandshake_mode;
@@ -213,7 +208,6 @@ typedef struct
    */
   gboolean need_handshake, need_finish_handshake;
   gboolean started_handshake, handshaking, ever_handshaked;
-  GMainContext *handshake_context;
   GTask *implicit_handshake;
   GError *handshake_error;
   GByteArray *app_data_buf;
@@ -261,9 +255,6 @@ g_tls_connection_gnutls_init (GTlsConnectionGnutls *gnutls)
 
   gnutls_certificate_allocate_credentials (&priv->creds);
 
-  g_mutex_init (&priv->verify_certificate_mutex);
-  g_cond_init (&priv->verify_certificate_condition);
-
   priv->need_handshake = TRUE;
 
   priv->database_is_unset = TRUE;
@@ -398,9 +389,6 @@ g_tls_connection_gnutls_initable_init (GInitable     *initable,
 
   gnutls_init (&priv->session, flags);
 
-  gnutls_session_set_ptr (priv->session, gnutls);
-  gnutls_session_set_verify_function (priv->session, verify_certificate_cb);
-
   status = gnutls_credentials_set (priv->session,
                                    GNUTLS_CRD_CERTIFICATE,
                                    priv->creds);
@@ -461,9 +449,7 @@ g_tls_connection_gnutls_finalize (GObject *object)
   g_clear_object (&priv->database);
   g_clear_object (&priv->certificate);
   g_clear_object (&priv->peer_certificate);
-
-  g_mutex_clear (&priv->verify_certificate_mutex);
-  g_cond_clear (&priv->verify_certificate_condition);
+  g_clear_object (&priv->peer_certificate_tmp);
 
   g_clear_pointer (&priv->app_data_buf, g_byte_array_unref);
 
@@ -478,8 +464,6 @@ g_tls_connection_gnutls_finalize (GObject *object)
   g_clear_error (&priv->read_error);
   g_clear_error (&priv->write_error);
 
-  g_clear_pointer (&priv->handshake_context, g_main_context_unref);
-
   /* This must always be NULL here, as it holds a reference to @gnutls as
    * its source object. However, we clear it anyway just in case this changes
    * in future. */
@@ -767,7 +751,6 @@ claim_op (GTlsConnectionGnutls    *gnutls,
           g_mutex_unlock (&priv->op_mutex);
           success = finish_handshake (gnutls, priv->implicit_handshake, &my_error);
           g_clear_object (&priv->implicit_handshake);
-          g_clear_pointer (&priv->handshake_context, g_main_context_unref);
           g_mutex_lock (&priv->op_mutex);
 
           if (op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_BOTH &&
@@ -1039,12 +1022,6 @@ end_gnutls_io (GTlsConnectionGnutls  *gnutls,
                            _("TLS connection peer did not send a certificate"));
       return status;
     }
-  else if (status == GNUTLS_E_CERTIFICATE_ERROR)
-    {
-      g_set_error (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
-                   _("Unacceptable TLS certificate"));
-      return status;
-    }
   else if (status == GNUTLS_E_FATAL_ALERT_RECEIVED)
     {
       g_set_error (error, G_TLS_ERROR, G_TLS_ERROR_MISC,
@@ -1805,102 +1782,6 @@ verify_peer_certificate (GTlsConnectionGnutls *gnutls,
   return errors;
 }
 
-static gboolean
-accept_peer_certificate (GTlsConnectionGnutls *gnutls,
-                         GTlsCertificate      *peer_certificate,
-                         GTlsCertificateFlags  peer_certificate_errors)
-{
-  gboolean accepted = FALSE;
-
-  if (G_IS_TLS_CLIENT_CONNECTION (gnutls))
-    {
-      GTlsCertificateFlags validation_flags;
-
-      if (!g_tls_connection_gnutls_is_dtls (gnutls))
-        validation_flags =
-          g_tls_client_connection_get_validation_flags (G_TLS_CLIENT_CONNECTION (gnutls));
-      else
-        validation_flags =
-          g_dtls_client_connection_get_validation_flags (G_DTLS_CLIENT_CONNECTION (gnutls));
-
-      if ((peer_certificate_errors & validation_flags) == 0)
-        accepted = TRUE;
-    }
-
-  if (!accepted)
-    {
-      accepted = g_tls_connection_emit_accept_certificate (G_TLS_CONNECTION (gnutls),
-                                                           peer_certificate,
-                                                           peer_certificate_errors);
-    }
-
-  return accepted;
-}
-
-static gboolean
-accept_certificate_cb (gpointer user_data)
-{
-  GTlsConnectionGnutls *gnutls = user_data;
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
-
-  g_assert (g_main_context_is_owner (priv->handshake_context));
-
-  g_mutex_lock (&priv->verify_certificate_mutex);
-
-  if (gnutls_certificate_type_get (priv->session) == GNUTLS_CRT_X509)
-    {
-      priv->peer_certificate = get_peer_certificate_from_session (gnutls);
-      if (priv->peer_certificate)
-        priv->peer_certificate_errors = verify_peer_certificate (gnutls, priv->peer_certificate);
-    }
-
-  priv->peer_certificate_accepted = accept_peer_certificate (gnutls,
-                                                             priv->peer_certificate,
-                                                             priv->peer_certificate_errors);
-
-  priv->peer_certificate_examined = TRUE;
-
-  g_cond_signal (&priv->verify_certificate_condition);
-  g_mutex_unlock (&priv->verify_certificate_mutex);
-
-  g_object_notify (G_OBJECT (gnutls), "peer-certificate");
-  g_object_notify (G_OBJECT (gnutls), "peer-certificate-errors");
-
-  return G_SOURCE_REMOVE;
-}
-
-static int
-verify_certificate_cb (gnutls_session_t session)
-{
-  GTlsConnectionGnutls *gnutls = gnutls_session_get_ptr (session);
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
-  gboolean accepted;
-
-  g_mutex_lock (&priv->verify_certificate_mutex);
-  priv->peer_certificate_examined = FALSE;
-  priv->peer_certificate_accepted = FALSE;
-  g_mutex_unlock (&priv->verify_certificate_mutex);
-
-  /* Invoke the callback on the handshake context's thread. This is
-   * necessary because we need to ensure the accept-certificate signal
-   * is emitted on the original thread.
-   */
-  g_assert (priv->handshake_context);
-  g_main_context_invoke (priv->handshake_context, accept_certificate_cb, gnutls);
-
-  /* We'll block the handshake thread until the original thread has
-   * decided whether to accept the certificate.
-   */
-  g_mutex_lock (&priv->verify_certificate_mutex);
-  while (!priv->peer_certificate_examined)
-    g_cond_wait (&priv->verify_certificate_condition, &priv->verify_certificate_mutex);
-  accepted = priv->peer_certificate_accepted;
-  g_mutex_unlock (&priv->verify_certificate_mutex);
-
-  /* Return 0 for the handshake to continue, non-zero to terminate. */
-  return !accepted;
-}
-
 static void
 handshake_thread (GTask        *task,
                   gpointer      object,
@@ -2017,10 +1898,16 @@ handshake_thread (GTask        *task,
   END_GNUTLS_IO (gnutls, G_IO_IN | G_IO_OUT, ret,
                  _("Error performing TLS handshake"), &error);
 
-  if (G_IS_TLS_CLIENT_CONNECTION (gnutls) && !priv->peer_certificate && !error)
+  if (ret == 0 && gnutls_certificate_type_get (priv->session) == GNUTLS_CRT_X509)
     {
-      g_set_error_literal (&error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
-                           _("Server did not return a valid TLS certificate"));
+      priv->peer_certificate_tmp = get_peer_certificate_from_session (gnutls);
+      if (priv->peer_certificate_tmp)
+        priv->peer_certificate_errors_tmp = verify_peer_certificate (gnutls, priv->peer_certificate_tmp);
+      else if (G_IS_TLS_CLIENT_CONNECTION (gnutls))
+        {
+          g_set_error_literal (&error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
+                               _("Server did not return a valid TLS certificate"));
+        }
     }
 
   /* This calls the finish_handshake code of GTlsClientConnectionGnutls
@@ -2041,6 +1928,38 @@ handshake_thread (GTask        *task,
     }
 }
 
+static gboolean
+accept_peer_certificate (GTlsConnectionGnutls *gnutls,
+                         GTlsCertificate      *peer_certificate,
+                         GTlsCertificateFlags  peer_certificate_errors)
+{
+  gboolean accepted = FALSE;
+
+  if (G_IS_TLS_CLIENT_CONNECTION (gnutls))
+    {
+      GTlsCertificateFlags validation_flags;
+
+      if (!g_tls_connection_gnutls_is_dtls (gnutls))
+        validation_flags =
+          g_tls_client_connection_get_validation_flags (G_TLS_CLIENT_CONNECTION (gnutls));
+      else
+        validation_flags =
+          g_dtls_client_connection_get_validation_flags (G_DTLS_CLIENT_CONNECTION (gnutls));
+
+      if ((peer_certificate_errors & validation_flags) == 0)
+        accepted = TRUE;
+    }
+
+  if (!accepted)
+    {
+      accepted = g_tls_connection_emit_accept_certificate (G_TLS_CONNECTION (gnutls),
+                                                           peer_certificate,
+                                                           peer_certificate_errors);
+    }
+
+  return accepted;
+}
+
 static void
 begin_handshake (GTlsConnectionGnutls *gnutls)
 {
@@ -2053,13 +1972,29 @@ finish_handshake (GTlsConnectionGnutls  *gnutls,
                   GError               **error)
 {
   GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
+  GTlsCertificate *peer_certificate;
+  GTlsCertificateFlags peer_certificate_errors;
+
   g_assert (error != NULL);
 
-  if (g_task_propagate_boolean (task, error) &&
-      priv->peer_certificate && !priv->peer_certificate_accepted)
+  peer_certificate = priv->peer_certificate_tmp;
+  priv->peer_certificate_tmp = NULL;
+  peer_certificate_errors = priv->peer_certificate_errors_tmp;
+  priv->peer_certificate_errors_tmp = 0;
+
+  if (g_task_propagate_boolean (task, error) && peer_certificate)
     {
-      g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
-                           _("Unacceptable TLS certificate"));
+      if (!accept_peer_certificate (gnutls, peer_certificate,
+                                    peer_certificate_errors))
+        {
+          g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
+                               _("Unacceptable TLS certificate"));
+        }
+
+      priv->peer_certificate = peer_certificate;
+      priv->peer_certificate_errors = peer_certificate_errors;
+      g_object_notify (G_OBJECT (gnutls), "peer-certificate");
+      g_object_notify (G_OBJECT (gnutls), "peer-certificate-errors");
     }
 
   if (*error && priv->started_handshake)
@@ -2068,77 +2003,26 @@ finish_handshake (GTlsConnectionGnutls  *gnutls,
   return (*error == NULL);
 }
 
-static void
-sync_handshake_thread_completed (GObject      *object,
-                                 GAsyncResult *result,
-                                 gpointer      user_data)
-{
-  GTlsConnectionGnutls *gnutls = G_TLS_CONNECTION_GNUTLS (object);
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
-
-  g_assert (g_main_context_is_owner (priv->handshake_context));
-
-  g_mutex_lock (&priv->op_mutex);
-  priv->need_finish_handshake = TRUE;
-  g_mutex_unlock (&priv->op_mutex);
-
-  g_main_context_wakeup (priv->handshake_context);
-}
-
-static void
-crank_sync_handshake_context (GTlsConnectionGnutls *gnutls,
-                              GCancellable         *cancellable)
-{
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
-
-  /* need_finish_handshake will be set inside sync_handshake_thread_completed(),
-   * which should only ever be invoked while iterating the handshake context
-   * here. So need_finish_handshake should only change on this thread.
-   */
-  g_mutex_lock (&priv->op_mutex);
-  priv->need_finish_handshake = FALSE;
-  while (!priv->need_finish_handshake && !g_cancellable_is_cancelled (cancellable))
-    {
-      g_mutex_unlock (&priv->op_mutex);
-      g_main_context_iteration (priv->handshake_context, TRUE);
-      g_mutex_lock (&priv->op_mutex);
-    }
-  g_mutex_unlock (&priv->op_mutex);
-}
-
 static gboolean
 g_tls_connection_gnutls_handshake (GTlsConnection   *conn,
                                    GCancellable     *cancellable,
                                    GError          **error)
 {
   GTlsConnectionGnutls *gnutls = G_TLS_CONNECTION_GNUTLS (conn);
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
   GTask *task;
   gboolean success;
   gint64 *timeout = NULL;
   GError *my_error = NULL;
 
-  g_assert (priv->handshake_context == NULL);
-  priv->handshake_context = g_main_context_new ();
-
-  g_main_context_push_thread_default (priv->handshake_context);
-
-  begin_handshake (gnutls);
-
-  task = g_task_new (conn, cancellable, sync_handshake_thread_completed, NULL);
+  task = g_task_new (conn, cancellable, NULL, NULL);
   g_task_set_source_tag (task, g_tls_connection_gnutls_handshake);
-  g_task_set_return_on_cancel (task, TRUE);
 
   timeout = g_new0 (gint64, 1);
   *timeout = -1;  /* blocking */
   g_task_set_task_data (task, timeout, g_free);
 
-  g_task_run_in_thread (task, handshake_thread);
-  crank_sync_handshake_context (gnutls, cancellable);
-
-  g_main_context_pop_thread_default (priv->handshake_context);
-
-  g_clear_pointer (&priv->handshake_context, g_main_context_unref);
+  begin_handshake (gnutls);
+  g_task_run_in_thread_sync (task, handshake_thread);
   success = finish_handshake (gnutls, task, &my_error);
   g_object_unref (task);
 
@@ -2174,8 +2058,6 @@ handshake_thread_completed (GObject      *object,
   GError *error = NULL;
   gboolean need_finish_handshake, success;
 
-  g_clear_pointer (&priv->handshake_context, g_main_context_unref);
-
   g_mutex_lock (&priv->op_mutex);
   if (priv->need_finish_handshake)
     {
@@ -2232,13 +2114,9 @@ g_tls_connection_gnutls_handshake_async (GTlsConnection       *conn,
                                          GAsyncReadyCallback   callback,
                                          gpointer              user_data)
 {
-  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (G_TLS_CONNECTION_GNUTLS 
(conn));
   GTask *thread_task, *caller_task;
   gint64 *timeout = NULL;
 
-  g_assert (!priv->handshake_context);
-  priv->handshake_context = g_main_context_ref_thread_default ();
-
   caller_task = g_task_new (conn, cancellable, callback, user_data);
   g_task_set_source_tag (caller_task, g_tls_connection_gnutls_handshake_async);
   g_task_set_priority (caller_task, io_priority);
@@ -2299,21 +2177,8 @@ do_implicit_handshake (GTlsConnectionGnutls  *gnutls,
 
   /* We have op_mutex */
 
-  g_assert (priv->handshake_context == NULL);
-  if (timeout != 0)
-    {
-      priv->handshake_context = g_main_context_new ();
-      g_main_context_push_thread_default (priv->handshake_context);
-    }
-  else
-    {
-      priv->handshake_context = g_main_context_ref_thread_default ();
-    }
-
   g_assert (priv->implicit_handshake == NULL);
-  priv->implicit_handshake = g_task_new (gnutls, cancellable,
-                                         timeout ? sync_handshake_thread_completed : NULL,
-                                         NULL);
+  priv->implicit_handshake = g_task_new (gnutls, cancellable, NULL, NULL);
   g_task_set_source_tag (priv->implicit_handshake,
                          do_implicit_handshake);
 
@@ -2337,14 +2202,8 @@ do_implicit_handshake (GTlsConnectionGnutls  *gnutls,
 
       g_mutex_unlock (&priv->op_mutex);
 
-      g_task_set_return_on_cancel (priv->implicit_handshake, TRUE);
-      g_task_run_in_thread (priv->implicit_handshake, handshake_thread);
-      crank_sync_handshake_context (gnutls, cancellable);
-
-      g_main_context_pop_thread_default (priv->handshake_context);
-
-      g_clear_pointer (&priv->handshake_context, g_main_context_unref);
-
+      g_task_run_in_thread_sync (priv->implicit_handshake,
+                                 handshake_thread);
       success = finish_handshake (gnutls,
                                   priv->implicit_handshake,
                                   &my_error);