[glib-networking/mcatanzaro/#135] Return bad identity error if identity is unset

[Michael Catanzaro <mcatanzaro src gnome org>]

commit ddc25dc92f160cae75be3cf5c3f7ea1d80f572b1
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Mon May 4 17:47:28 2020 -0500

    Return bad identity error if identity is unset
    
    When the server-identity property of GTlsClientConnection is unset, the
    documentation sasy we need to fail the certificate verification with
    G_TLS_CERTIFICATE_BAD_IDENTITY. This is important because otherwise,
    it's easy for applications to fail to specify server identity.
    
    Unfortunately, we did not correctly implement the intended, documented
    behavior. When server identity is missing, we check the validity of the
    TLS certificate, but do not check if it corresponds to the expected
    server (since we have no expected server). Then we assume the identity
    is good, instead of returning bad identity, as documented. This means,
    for example, that evil.com can present a valid certificate issued to
    evil.com, and we would happily accept it for paypal.com.
    
    Fixes #135

 tls/base/gtlsconnection-base.c | 20 ++++++------
 tls/tests/connection.c         | 70 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 9 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index f7ad660..7f0e6fe 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -1233,21 +1233,23 @@ static GTlsCertificateFlags
 verify_peer_certificate (GTlsConnectionBase *tls,
                          GTlsCertificate    *peer_certificate)
 {
-  GSocketConnectable *peer_identity;
+  GSocketConnectable *peer_identity = NULL;
   GTlsDatabase *database;
-  GTlsCertificateFlags errors;
+  GTlsCertificateFlags errors = 0;
   gboolean is_client;
 
   is_client = G_IS_TLS_CLIENT_CONNECTION (tls);
 
-  if (!is_client)
-    peer_identity = NULL;
-  else if (!g_tls_connection_base_is_dtls (tls))
-    peer_identity = g_tls_client_connection_get_server_identity (G_TLS_CLIENT_CONNECTION (tls));
-  else
-    peer_identity = g_dtls_client_connection_get_server_identity (G_DTLS_CLIENT_CONNECTION (tls));
+  if (is_client)
+    {
+      if (!g_tls_connection_base_is_dtls (tls))
+        peer_identity = g_tls_client_connection_get_server_identity (G_TLS_CLIENT_CONNECTION (tls));
+      else
+        peer_identity = g_dtls_client_connection_get_server_identity (G_DTLS_CLIENT_CONNECTION (tls));
 
-  errors = 0;
+      if (!peer_identity)
+        errors |= G_TLS_CERTIFICATE_BAD_IDENTITY;
+    }
 
   database = g_tls_connection_get_database (G_TLS_CONNECTION (tls));
   if (!database)
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index 61bf09b..936d1f9 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2444,6 +2444,74 @@ test_socket_timeout (TestConnection *test,
   g_assert_error (test->server_error, G_TLS_ERROR, G_TLS_ERROR_NOT_TLS);
 }
 
+static void
+test_connection_missing_server_identity (TestConnection *test,
+                                         gconstpointer   data)
+{
+  GIOStream *connection;
+  GError *error = NULL;
+
+  test->database = g_tls_file_database_new (tls_test_file_path ("ca-roots.pem"), &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (test->database);
+
+  /* We pass NULL instead of test->identity when creating the client
+   * connection. This means verification must fail with
+   * G_TLS_CERTIFICATE_BAD_IDENTITY.
+   */
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+  test->client_connection = g_tls_client_connection_new (connection, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (test->client_connection);
+  g_object_unref (connection);
+
+  g_tls_connection_set_database (G_TLS_CONNECTION (test->client_connection), test->database);
+
+  /* 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);
+  wait_until_server_finished (test);
+
+  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 a NOT_TLS errors. But some times
+   * we either get no error or BROKEN_PIPE
+   */
+#endif
+
+  g_clear_error (&test->read_error);
+  g_clear_error (&test->server_error);
+
+  g_clear_object (&test->client_connection);
+  g_clear_object (&test->server_connection);
+
+  /* Now do the same thing again, this time ignoring bad identity. */
+
+  connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+  test->client_connection = g_tls_client_connection_new (connection, NULL, &error);
+  g_assert_no_error (error);
+  g_assert_nonnull (test->client_connection);
+  g_object_unref (connection);
+
+  g_tls_connection_set_database (G_TLS_CONNECTION (test->client_connection), test->database);
+
+  g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+                                                G_TLS_CERTIFICATE_VALIDATE_ALL & 
~G_TLS_CERTIFICATE_BAD_IDENTITY);
+
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);
+  wait_until_server_finished (test);
+
+  g_assert_no_error (test->read_error);
+  g_assert_no_error (test->server_error);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -2530,6 +2598,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/missing-server-identity", TestConnection, NULL,
+              setup_connection, test_connection_missing_server_identity, teardown_connection);
 
   ret = g_test_run ();