[glib-networking] Fix crash when setting client cert without private key

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 15531b53dc596ea6a6b022ec9e3ef6b771808d68
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Sun Feb 25 21:19:04 2018 -0600

    Fix crash when setting client cert without private key
    
    If, if g_tls_client_connection_gnutls_retrieve_function, we return to
    GnuTLS a gnutls_retr2_st with positive ncerts but no key, GnuTLS will
    crash. GnuTLS should probably become robust to this scenario, just as it
    must be robust to any manner of invalid input here. But it makes sense
    for us to handle it, as well. We must abort the handshake in this case.
    
    A new test is added that triggers this crash. Thanks to Martin Pitt for
    contributing a reproducer that formed the basis for this test.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=793712

 tls/gnutls/gtlscertificate-gnutls.c      |    4 ++-
 tls/gnutls/gtlsclientconnection-gnutls.c |   30 ++++++++++++++++----
 tls/gnutls/gtlsconnection-gnutls.c       |    4 +-
 tls/tests/connection.c                   |   43 ++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 9 deletions(-)
---
diff --git a/tls/gnutls/gtlscertificate-gnutls.c b/tls/gnutls/gtlscertificate-gnutls.c
index b4263c8..95b8093 100644
--- a/tls/gnutls/gtlscertificate-gnutls.c
+++ b/tls/gnutls/gtlscertificate-gnutls.c
@@ -410,6 +410,7 @@ g_tls_certificate_gnutls_real_copy (GTlsCertificateGnutls    *gnutls,
     }
 
   st->ncerts = 0;
+  st->cert_type = GNUTLS_CRT_X509;
   st->cert.x509 = gnutls_malloc (sizeof (gnutls_x509_crt_t) * num_certs);
 
   /* Now do the actual copy of the whole chain. */
@@ -443,8 +444,9 @@ g_tls_certificate_gnutls_real_copy (GTlsCertificateGnutls    *gnutls,
         {
           gnutls_x509_privkey_init (&st->key.x509);
           gnutls_x509_privkey_cpy (st->key.x509, priv->key);
-          st->key_type = GNUTLS_PRIVKEY_X509;
         }
+
+      st->key_type = GNUTLS_PRIVKEY_X509;
     }
 
   st->deinit_all = TRUE;
diff --git a/tls/gnutls/gtlsclientconnection-gnutls.c b/tls/gnutls/gtlsclientconnection-gnutls.c
index eb4c4fa..f0fa94e 100644
--- a/tls/gnutls/gtlsclientconnection-gnutls.c
+++ b/tls/gnutls/gtlsclientconnection-gnutls.c
@@ -56,7 +56,7 @@ struct _GTlsClientConnectionGnutls
   GBytes *session_id;
   GBytes *session_data;
 
-  gboolean cert_requested;
+  gboolean requested_cert_missing;
   GError *cert_error;
   GPtrArray *accepted_cas;
 };
@@ -308,8 +308,6 @@ g_tls_client_connection_gnutls_retrieve_function (gnutls_session_t             s
    * the algorithms given in pk_algos.
    */
 
-  gnutls->cert_requested = TRUE;
-
   accepted_cas = g_ptr_array_new_with_free_func ((GDestroyNotify)g_byte_array_unref);
   for (i = 0; i < nreqs; i++)
     {
@@ -330,6 +328,27 @@ g_tls_client_connection_gnutls_retrieve_function (gnutls_session_t             s
       g_clear_error (&gnutls->cert_error);
       if (g_tls_connection_gnutls_request_certificate (conn, &gnutls->cert_error))
         g_tls_connection_gnutls_get_certificate (conn, st);
+
+      if (st->ncerts == 0)
+        {
+          /* If there is still no client certificate, this connection will
+           * probably fail, but no reason to give up: let's try anyway.
+           */
+          gnutls->requested_cert_missing = TRUE;
+          return 0;
+        }
+    }
+
+  g_assert (st->key_type == GNUTLS_PRIVKEY_X509 ||
+            st->key_type == GNUTLS_PRIVKEY_PKCS11);
+  if ((st->key_type == GNUTLS_PRIVKEY_X509 && st->key.x509 == NULL) ||
+      (st->key_type == GNUTLS_PRIVKEY_PKCS11 && st->key.pkcs11 == NULL))
+    {
+      /* No private key. GnuTLS expects it to be non-null if ncerts is nonzero,
+       * so we have to abort now.
+       */
+      gnutls->requested_cert_missing = TRUE;
+      return -1;
     }
 
   return 0;
@@ -375,7 +394,7 @@ g_tls_client_connection_gnutls_begin_handshake (GTlsConnectionGnutls *conn)
         }
     }
 
-  gnutls->cert_requested = FALSE;
+  gnutls->requested_cert_missing = FALSE;
 }
 
 static void
@@ -387,8 +406,7 @@ g_tls_client_connection_gnutls_finish_handshake (GTlsConnectionGnutls  *conn,
 
   g_assert (inout_error != NULL);
 
-  if (g_error_matches (*inout_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS) &&
-      gnutls->cert_requested)
+  if (*inout_error != NULL && gnutls->requested_cert_missing)
     {
       g_clear_error (inout_error);
       if (gnutls->cert_error)
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index a0d72a0..6aa17bc 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -658,11 +658,11 @@ g_tls_connection_gnutls_get_certificate (GTlsConnectionGnutls *gnutls,
   GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
   GTlsCertificate *cert;
 
-  cert = g_tls_connection_get_certificate (G_TLS_CONNECTION (gnutls));
-
   st->cert_type = GNUTLS_CRT_X509;
   st->ncerts = 0;
 
+  cert = g_tls_connection_get_certificate (G_TLS_CONNECTION (gnutls));
+
   if (cert)
     g_tls_certificate_gnutls_copy (G_TLS_CERTIFICATE_GNUTLS (cert),
                                    priv->interaction_id, st);
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 235c056..c93cc2e 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -1088,6 +1088,47 @@ test_client_auth_failure (TestConnection *test,
 }
 
 static void
+test_client_auth_fail_missing_client_private_key (TestConnection *test,
+                                                  gconstpointer   data)
+{
+  GTlsCertificate *cert;
+  GIOStream *connection;
+  GError *error = NULL;
+
+  g_test_bug ("793712");
+
+  test->database = g_tls_file_database_new (tls_test_file_path ("ca-roots.pem"), &error);
+  g_assert_no_error (error);
+  g_assert (test->database);
+
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_REQUIRED, TRUE);
+  test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
+  g_assert_no_error (error);
+  g_assert (test->client_connection);
+  g_object_unref (connection);
+
+  g_tls_connection_set_database (G_TLS_CONNECTION (test->client_connection), test->database);
+
+  /* Oops: we "accidentally" set client.pem rather than client-and-key.pem. The
+   * connection will fail, but we should not crash.
+   */
+  cert = g_tls_certificate_new_from_file (tls_test_file_path ("client.pem"), &error);
+  g_assert_no_error (error);
+
+  g_tls_connection_set_certificate (G_TLS_CONNECTION (test->client_connection), cert);
+
+  /* All validation in this test */
+  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+                                                G_TLS_CERTIFICATE_VALIDATE_ALL);
+
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);
+
+  g_assert_error (test->read_error, G_TLS_ERROR, G_TLS_ERROR_CERTIFICATE_REQUIRED);
+  g_assert_no_error (test->server_error);
+}
+
+static void
 test_client_auth_request_cert (TestConnection *test,
                                gconstpointer   data)
 {
@@ -2072,6 +2113,8 @@ main (int   argc,
               setup_connection, test_client_auth_rehandshake, teardown_connection);
   g_test_add ("/tls/connection/client-auth-failure", TestConnection, NULL,
               setup_connection, test_client_auth_failure, teardown_connection);
+  g_test_add ("/tls/connection/client-auth-fail-missing-client-private-key", TestConnection, NULL,
+              setup_connection, test_client_auth_fail_missing_client_private_key, teardown_connection);
   g_test_add ("/tls/connection/client-auth-request-cert", TestConnection, NULL,
               setup_connection, test_client_auth_request_cert, teardown_connection);
   g_test_add ("/tls/connection/client-auth-request-fail", TestConnection, NULL,