GNOME.org
 

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

commit f72f792be877656b789c86652eb301af2d556a90
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Sat Nov 10 12:26:12 2018 -0600

    Revert "Revert "Perform certificate verification during, not after, TLS handshake""
    
    This reverts commit 22c1879b773e4f9df5a1c3761b0845986b504caf.

 meson.build                        |   2 +-
 tls/gnutls/gtlsconnection-gnutls.c | 283 +++++++++++++++++++++++++++----------
 2 files changed, 213 insertions(+), 72 deletions(-)
---
diff --git a/meson.build b/meson.build
index 4900d28..70180b3 100644
--- a/meson.build
+++ b/meson.build
@@ -71,7 +71,7 @@ gsettings_desktop_schemas_dep = dependency('gsettings-desktop-schemas', required
 backends = []
 
 # *** Checks for GnuTLS     ***
-gnutls_dep = dependency('gnutls', version: '>= 3.4.4', required: get_option('gnutls'))
+gnutls_dep = dependency('gnutls', version: '>= 3.4.6', required: get_option('gnutls'))
 
 if gnutls_dep.found()
   backends += ['gnutls']
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 37f40a4..a2fd4f1 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -118,12 +118,14 @@ 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                 *thread_task,
+                                  GTask                 *task,
                                   GError               **error);
 
 enum
@@ -177,8 +179,11 @@ typedef struct
 
   GTlsCertificate *certificate, *peer_certificate;
   GTlsCertificateFlags peer_certificate_errors;
-  GTlsCertificate *peer_certificate_tmp;
-  GTlsCertificateFlags peer_certificate_errors_tmp;
+
+  GMutex verify_certificate_mutex;
+  GCond verify_certificate_condition;
+  gboolean peer_certificate_accepted;
+  gboolean peer_certificate_examined;
 
   gboolean require_close_notify;
   GTlsRehandshakeMode rehandshake_mode;
@@ -209,6 +214,7 @@ 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;
@@ -256,6 +262,9 @@ 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;
@@ -390,6 +399,9 @@ 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);
@@ -450,7 +462,9 @@ g_tls_connection_gnutls_finalize (GObject *object)
   g_clear_object (&priv->database);
   g_clear_object (&priv->certificate);
   g_clear_object (&priv->peer_certificate);
-  g_clear_object (&priv->peer_certificate_tmp);
+
+  g_mutex_clear (&priv->verify_certificate_mutex);
+  g_cond_clear (&priv->verify_certificate_condition);
 
   g_clear_pointer (&priv->app_data_buf, g_byte_array_unref);
 
@@ -465,6 +479,8 @@ 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. */
@@ -752,6 +768,7 @@ 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 &&
@@ -1023,6 +1040,12 @@ 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,
@@ -1783,6 +1806,102 @@ 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,
@@ -1900,16 +2019,10 @@ handshake_thread (GTask        *task,
   END_GNUTLS_IO (gnutls, G_IO_IN | G_IO_OUT, ret,
                  _("Error performing TLS handshake"), &error);
 
-  if (ret == 0 && gnutls_certificate_type_get (priv->session) == GNUTLS_CRT_X509)
+  if (G_IS_TLS_CLIENT_CONNECTION (gnutls) && !priv->peer_certificate && !error)
     {
-      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"));
-        }
+      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
@@ -1930,38 +2043,6 @@ 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)
 {
@@ -1974,29 +2055,13 @@ 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);
 
-  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)
+  if (g_task_propagate_boolean (task, error) &&
+      priv->peer_certificate && !priv->peer_certificate_accepted)
     {
-      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");
+      g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
+                           _("Unacceptable TLS certificate"));
     }
 
   if (*error && priv->started_handshake)
@@ -2005,26 +2070,77 @@ 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;
 
-  task = g_task_new (conn, cancellable, NULL, 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);
   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);
 
-  begin_handshake (gnutls);
-  g_task_run_in_thread_sync (task, handshake_thread);
+  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);
   success = finish_handshake (gnutls, task, &my_error);
   g_object_unref (task);
 
@@ -2060,6 +2176,8 @@ 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)
     {
@@ -2116,9 +2234,13 @@ 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);
@@ -2179,8 +2301,21 @@ 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, NULL, NULL);
+  priv->implicit_handshake = g_task_new (gnutls, cancellable,
+                                         timeout ? sync_handshake_thread_completed : NULL,
+                                         NULL);
   g_task_set_source_tag (priv->implicit_handshake,
                          do_implicit_handshake);
 
@@ -2204,8 +2339,14 @@ do_implicit_handshake (GTlsConnectionGnutls  *gnutls,
 
       g_mutex_unlock (&priv->op_mutex);
 
-      g_task_run_in_thread_sync (priv->implicit_handshake,
-                                 handshake_thread);
+      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);
+
       success = finish_handshake (gnutls,
                                   priv->implicit_handshake,
                                   &my_error);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]