[glib-networking/mcatanzaro/#127] Fix peer-certificate[-errors] props set too soon

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 9d09d051869a0bf96d246da8c35f48f01fd4138c
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Sat Feb 1 17:03:57 2020 -0600

    Fix peer-certificate[-errors] props set too soon
    
    Our documentation says these properties will not be set until *after* a
    *successful* handshake. I didn't know this, and there's no particular
    reason it needs to be that way other than because the documentation says
    so. Leave a warning comment to avoid breaking this in the future.
    
    Since OpenSSL allows its handshake to succeed before manually failing it
    afterwards, we need to manually add an error in that case. I haven't
    investigated far enough to know why, but it doesn't matter because I
    have a sidebranch where OpenSSL handshakes normally without this hack,
    which I hope to land soon.

 tls/base/gtlsconnection-base.c       |  72 ++++++++++---------
 tls/openssl/gtlsconnection-openssl.c |   6 +-
 tls/tests/connection.c               | 131 +++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 35 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 4fb224a..8878026 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -1272,51 +1272,28 @@ verify_peer_certificate (GTlsConnectionBase *tls,
   return errors;
 }
 
-static void
-update_peer_certificate_and_compute_errors (GTlsConnectionBase *tls)
+static gboolean
+accept_or_reject_peer_certificate (gpointer user_data)
 {
+  GTlsConnectionBase *tls = user_data;
   GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
   GTlsCertificate *peer_certificate = NULL;
   GTlsCertificateFlags peer_certificate_errors = 0;
+  gboolean accepted = FALSE;
 
   /* This function must be called from the handshake context thread
    * (probably the main thread, NOT the handshake thread) because
    * it emits notifies that are application-visible.
-   *
-   * verify_certificate_mutex should be locked.
    */
   g_assert (priv->handshake_context);
   g_assert (g_main_context_is_owner (priv->handshake_context));
 
   peer_certificate = G_TLS_CONNECTION_BASE_GET_CLASS (tls)->retrieve_peer_certificate (tls);
-  if (peer_certificate)
-    peer_certificate_errors = verify_peer_certificate (tls, peer_certificate);
-
-  g_clear_object (&priv->peer_certificate);
-  priv->peer_certificate = g_steal_pointer (&peer_certificate);
-  g_clear_object (&peer_certificate);
-
-  priv->peer_certificate_errors = peer_certificate_errors;
-
-  g_object_notify (G_OBJECT (tls), "peer-certificate");
-  g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
-}
-
-static gboolean
-accept_or_reject_peer_certificate (gpointer user_data)
-{
-  GTlsConnectionBase *tls = user_data;
-  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
-  gboolean accepted = FALSE;
 
-  g_assert (g_main_context_is_owner (priv->handshake_context));
-
-  g_mutex_lock (&priv->verify_certificate_mutex);
-
-  update_peer_certificate_and_compute_errors (tls);
-
-  if (priv->peer_certificate)
+  if (peer_certificate)
     {
+      peer_certificate_errors = verify_peer_certificate (tls, peer_certificate);
+
       if (G_IS_TLS_CLIENT_CONNECTION (tls))
         {
           GTlsCertificateFlags validation_flags;
@@ -1328,7 +1305,7 @@ accept_or_reject_peer_certificate (gpointer user_data)
             validation_flags =
               g_dtls_client_connection_get_validation_flags (G_DTLS_CLIENT_CONNECTION (tls));
 
-          if ((priv->peer_certificate_errors & validation_flags) == 0)
+          if ((peer_certificate_errors & validation_flags) == 0)
             accepted = TRUE;
         }
 
@@ -1344,8 +1321,8 @@ accept_or_reject_peer_certificate (gpointer user_data)
             g_main_context_pop_thread_default (priv->handshake_context);
 
           accepted = g_tls_connection_emit_accept_certificate (G_TLS_CONNECTION (tls),
-                                                               priv->peer_certificate,
-                                                               priv->peer_certificate_errors);
+                                                               peer_certificate,
+                                                               peer_certificate_errors);
 
           if (sync_handshake_in_progress)
             g_main_context_push_thread_default (priv->handshake_context);
@@ -1363,8 +1340,28 @@ accept_or_reject_peer_certificate (gpointer user_data)
         accepted = TRUE;
     }
 
+  g_mutex_lock (&priv->verify_certificate_mutex);
+
   priv->peer_certificate_accepted = accepted;
 
+  if (accepted)
+    {
+      /* The API documentation indicates that these properties are not
+       * set until after a successful handshake.
+       */
+      g_clear_object (&priv->peer_certificate);
+      priv->peer_certificate = g_steal_pointer (&peer_certificate);
+      priv->peer_certificate_errors = peer_certificate_errors;
+    }
+  else
+    {
+      g_clear_object (&peer_certificate);
+      g_clear_object (&priv->peer_certificate);
+      priv->peer_certificate_errors = 0;
+    }
+  g_object_notify (G_OBJECT (tls), "peer-certificate");
+  g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
+
   /* This has to be the very last statement before signaling the
    * condition variable because otherwise the code could spuriously
    * wakeup and continue before we are done here.
@@ -1574,7 +1571,14 @@ finish_handshake (GTlsConnectionBase  *tls,
            * anything with the result here.
            */
           g_mutex_lock (&priv->verify_certificate_mutex);
-          update_peer_certificate_and_compute_errors (tls);
+
+          g_clear_object (&priv->peer_certificate);
+          priv->peer_certificate = G_TLS_CONNECTION_BASE_GET_CLASS (tls)->retrieve_peer_certificate (tls);
+          priv->peer_certificate_errors = verify_peer_certificate (tls, priv->peer_certificate);
+
+          g_object_notify (G_OBJECT (tls), "peer-certificate");
+          g_object_notify (G_OBJECT (tls), "peer-certificate-errors");
+
           priv->peer_certificate_examined = TRUE;
           priv->peer_certificate_accepted = TRUE;
           g_mutex_unlock (&priv->verify_certificate_mutex);
diff --git a/tls/openssl/gtlsconnection-openssl.c b/tls/openssl/gtlsconnection-openssl.c
index d751344..a78309d 100644
--- a/tls/openssl/gtlsconnection-openssl.c
+++ b/tls/openssl/gtlsconnection-openssl.c
@@ -304,7 +304,11 @@ g_tls_connection_openssl_handshake_thread_handshake (GTlsConnectionBase  *tls,
   if (ret > 0)
     {
       if (!g_tls_connection_base_handshake_thread_verify_certificate (G_TLS_CONNECTION_BASE (openssl)))
-        return G_TLS_CONNECTION_BASE_ERROR;
+        {
+          g_set_error_literal (error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE,
+                               _("Unacceptable TLS certificate"));
+          return G_TLS_CONNECTION_BASE_ERROR;
+        }
     }
 
   return status;
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 21a150a..1b1583b 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2440,6 +2440,135 @@ test_socket_timeout (TestConnection *test,
   g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS);
 }
 
+typedef struct {
+  TestConnection *test;
+  gboolean peer_certificate_notified;
+  gboolean peer_certificate_errors_notified;
+} NotifyTestData;
+
+static gboolean
+on_accept_certificate_peer_certificate_notify (GTlsConnection       *conn,
+                                               GTlsCertificate      *cert,
+                                               GTlsCertificateFlags  errors,
+                                               NotifyTestData       *data)
+{
+  TestConnection *test = data->test;
+
+  g_assert_true (G_IS_TLS_CERTIFICATE (cert));
+
+  /* We guarantee these props are not set until after the handshake. */
+  g_assert_null (g_tls_connection_get_peer_certificate (conn));
+  g_assert_cmpint (g_tls_connection_get_peer_certificate_errors (conn), ==, 0);
+
+  g_assert_false (data->peer_certificate_notified);
+  g_assert_false (data->peer_certificate_errors_notified);
+
+  return errors == test->accept_flags;
+}
+
+static void
+on_peer_certificate_notify (GTlsConnection *conn,
+                            GParamSpec     *pspec,
+                            gboolean       *notified)
+{
+  *notified = TRUE;
+}
+
+static void
+on_peer_certificate_errors_notify (GTlsConnection *conn,
+                                   GParamSpec     *pspec,
+                                   gboolean       *notified)
+{
+  *notified = TRUE;
+}
+
+static void
+test_peer_certificate_notify (TestConnection *test,
+                              gconstpointer   data)
+{
+  NotifyTestData notify_data = { test, FALSE, FALSE };
+  GIOStream *connection;
+  GError *error = NULL;
+
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+  test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
+  g_assert_no_error (error);
+  g_object_unref (connection);
+
+  /* For this test, we need validation to fail to ensure that the
+   * accept-certificate signal gets emitted.
+   */
+  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+                                                G_TLS_CERTIFICATE_VALIDATE_ALL);
+
+  g_signal_connect (test->client_connection, "accept-certificate",
+                    G_CALLBACK (on_accept_certificate_peer_certificate_notify), &notify_data);
+  g_signal_connect (test->client_connection, "notify::peer-certificate",
+                    G_CALLBACK (on_peer_certificate_notify), &notify_data.peer_certificate_notified);
+  g_signal_connect (test->client_connection, "notify::peer-certificate-errors",
+                    G_CALLBACK (on_peer_certificate_errors_notify), 
&notify_data.peer_certificate_errors_notified);
+
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);
+  wait_until_server_finished (test);
+
+  g_assert_true (notify_data.peer_certificate_notified);
+  g_assert_true (notify_data.peer_certificate_errors_notified);
+
+  /* These properties should only be set after a *successful* handshake. */
+  g_assert_null (g_tls_connection_get_peer_certificate (G_TLS_CONNECTION (test->client_connection)));
+  g_assert_cmpint (g_tls_connection_get_peer_certificate_errors (G_TLS_CONNECTION 
(test->client_connection)), ==, 0);
+
+  g_assert_error (test->read_error, G_TLS_ERROR, G_TLS_ERROR_BAD_CERTIFICATE);
+
+#ifdef BACKEND_IS_GNUTLS
+  g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS);
+#elif defined(BACKEND_IS_OPENSSL)
+  /* FIXME: This is not OK. There should be an error here. */
+  g_assert_no_error (test->server_error);
+#endif
+
+  /* Now do it all again, this time with a successful connection. */
+
+  g_clear_object (&test->service);
+  g_clear_object (&test->server_connection);
+  g_clear_object (&test->client_connection);
+
+  g_clear_error (&test->read_error);
+  g_clear_error (&test->server_error);
+
+  notify_data.peer_certificate_notified = FALSE;
+  notify_data.peer_certificate_errors_notified = FALSE;
+
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+  test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
+  g_assert_no_error (error);
+  g_object_unref (connection);
+
+  g_signal_connect (test->client_connection, "accept-certificate",
+                    G_CALLBACK (on_accept_certificate_peer_certificate_notify), &notify_data);
+  g_signal_connect (test->client_connection, "notify::peer-certificate",
+                    G_CALLBACK (on_peer_certificate_notify), &notify_data.peer_certificate_notified);
+  g_signal_connect (test->client_connection, "notify::peer-certificate-errors",
+                    G_CALLBACK (on_peer_certificate_errors_notify), 
&notify_data.peer_certificate_errors_notified);
+
+  /* Disable validation so we have a successful handshake. */
+  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+                                                0);
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);
+  wait_until_server_finished (test);
+
+  g_assert_true (notify_data.peer_certificate_notified);
+  g_assert_true (notify_data.peer_certificate_errors_notified);
+
+  g_assert_true (G_IS_TLS_CERTIFICATE (g_tls_connection_get_peer_certificate (G_TLS_CONNECTION 
(test->client_connection))));
+  g_assert_cmpint (g_tls_connection_get_peer_certificate_errors (G_TLS_CONNECTION 
(test->client_connection)), !=, 0);
+
+  g_assert_no_error (test->read_error);
+  g_assert_no_error (test->server_error);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -2526,6 +2655,8 @@ main (int   argc,
               setup_connection, test_sync_op_during_handshake, teardown_connection);
   g_test_add ("/tls/" BACKEND "/connection/socket-timeout", TestConnection, NULL,
               setup_connection, test_socket_timeout, teardown_connection);
+  g_test_add ("/tls/" BACKEND "/connection/peer-certificate-notify", TestConnection, NULL,
+              setup_connection, test_peer_certificate_notify, teardown_connection);
 
   ret = g_test_run ();