From 780af9cff3cc636525a973c3f0c0244f2422a39e Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 4 Aug 2021 14:57:05 -0500 Subject: [PATCH 1/3] Document potential footgun with GTlsCertificateFlags Once upon a time, we tried to return all possible certificate errors, but it never actually worked reliably and nowadays we have given up. This needs to be documented because a reasonable developer would not expect it. Because mistakes could be security-critical, I decided to copy the same warning in several different places rather than relying only on cross-referencese. --- gio/gdtlsconnection.c | 17 +++++++++++++++++ gio/gioenums.h | 14 ++++++++++---- gio/gtlscertificate.c | 9 +++++++-- gio/gtlsconnection.c | 19 +++++++++++++++++++ gio/gtlsdatabase.c | 22 +++++++++++++++------- 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/gio/gdtlsconnection.c b/gio/gdtlsconnection.c index 880d87d2c..ec9234825 100644 --- a/gio/gdtlsconnection.c +++ b/gio/gdtlsconnection.c @@ -223,6 +223,14 @@ g_dtls_connection_default_init (GDtlsConnectionInterface *iface) * #GDtlsConnection::accept-certificate overrode the default * behavior. * + * GLib guarantees that if certificate verification fails, at least + * one error will be set, but it does not guarantee that all possible + * errors will be set. Accordingly, you may not safely decide to + * ignore any particular type of error. For example, it would be + * incorrect to mask %G_TLS_CERTIFICATE_EXPIRED if you want to allow + * expired certificates, because this could potentially be the only + * error flag set even if other problems exist with the certificate. + * * Since: 2.48 */ g_object_interface_install_property (iface, @@ -314,6 +322,15 @@ g_dtls_connection_default_init (GDtlsConnectionInterface *iface) * signal handler. Otherwise, if no handler accepts the certificate, * the handshake will fail with %G_TLS_ERROR_BAD_CERTIFICATE. * + * GLib guarantees that if certificate verification fails, this signal + * will be emitted with at least one error will be set in @errors, but + * it does not guarantee that all possible errors will be set. + * Accordingly, you may not safely decide to ignore any particular + * type of error. For example, it would be incorrect to ignore + * %G_TLS_CERTIFICATE_EXPIRED if you want to allow expired + * certificates, because this could potentially be the only error flag + * set even if other problems exist with the certificate. + * * For a server-side connection, @peer_cert is the certificate * presented by the client, if this was requested via the server's * #GDtlsServerConnection:authentication_mode. On the server side, diff --git a/gio/gioenums.h b/gio/gioenums.h index 2b6b5eb13..47ca06bed 100644 --- a/gio/gioenums.h +++ b/gio/gioenums.h @@ -1588,10 +1588,16 @@ typedef enum { * flags * * A set of flags describing TLS certification validation. This can be - * used to set which validation steps to perform (eg, with - * g_tls_client_connection_set_validation_flags()), or to describe why - * a particular certificate was rejected (eg, in - * #GTlsConnection::accept-certificate). + * used to describe why a particular certificate was rejected (for + * example, in #GTlsConnection::accept-certificate). + * + * GLib guarantees that if certificate verification fails, at least one + * flag will be set, but it does not guarantee that all possible flags + * will be set. Accordingly, you may not safely decide to ignore any + * particular type of error. For example, it would be incorrect to mask + * %G_TLS_CERTIFICATE_EXPIRED if you want to allow expired certificates, + * because this could potentially be the only error flag set even if + * other problems exist with the certificate. * * Since: 2.28 */ diff --git a/gio/gtlscertificate.c b/gio/gtlscertificate.c index 2c238120c..8231ea0e4 100644 --- a/gio/gtlscertificate.c +++ b/gio/gtlscertificate.c @@ -959,8 +959,13 @@ g_tls_certificate_get_issuer (GTlsCertificate *cert) * @trusted_ca is %NULL, that bit will never be set in the return * value. * - * (All other #GTlsCertificateFlags values will always be set or unset - * as appropriate.) + * GLib guarantees that if certificate verification fails, at least one + * error will be set in the return value, but it does not guarantee + * that all possible errors will be set. Accordingly, you may not safely + * decide to ignore any particular type of error. For example, it would + * be incorrect to mask %G_TLS_CERTIFICATE_EXPIRED if you want to allow + * expired certificates, because this could potentially be the only + * error flag set even if other problems exist with the certificate. * * Because TLS session context is not used, #GTlsCertificate may not * perform as many checks on the certificates as #GTlsConnection would. diff --git a/gio/gtlsconnection.c b/gio/gtlsconnection.c index 0239489b7..c4c305528 100644 --- a/gio/gtlsconnection.c +++ b/gio/gtlsconnection.c @@ -248,6 +248,14 @@ g_tls_connection_class_init (GTlsConnectionClass *klass) * #GTlsConnection::accept-certificate overrode the default * behavior. * + * GLib guarantees that if certificate verification fails, at least + * one error will be set, but it does not guarantee that all possible + * errors will be set. Accordingly, you may not safely decide to + * ignore any particular type of error. For example, it would be + * incorrect to mask %G_TLS_CERTIFICATE_EXPIRED if you want to allow + * expired certificates, because this could potentially be the only + * error flag set even if other problems exist with the certificate. + * * Since: 2.28 */ g_object_class_install_property (gobject_class, PROP_PEER_CERTIFICATE_ERRORS, @@ -339,6 +347,15 @@ g_tls_connection_class_init (GTlsConnectionClass *klass) * signal handler. Otherwise, if no handler accepts the certificate, * the handshake will fail with %G_TLS_ERROR_BAD_CERTIFICATE. * + * GLib guarantees that if certificate verification fails, this signal + * will be emitted with at least one error will be set in @errors, but + * it does not guarantee that all possible errors will be set. + * Accordingly, you may not safely decide to ignore any particular + * type of error. For example, it would be incorrect to ignore + * %G_TLS_CERTIFICATE_EXPIRED if you want to allow expired + * certificates, because this could potentially be the only error flag + * set even if other problems exist with the certificate. + * * For a server-side connection, @peer_cert is the certificate * presented by the client, if this was requested via the server's * #GTlsServerConnection:authentication_mode. On the server side, @@ -655,6 +672,8 @@ g_tls_connection_get_peer_certificate (GTlsConnection *conn) * certificate, after the handshake has completed or failed. (It is * not set during the emission of #GTlsConnection::accept-certificate.) * + * See #GTlsConnection:peer-certificate-errors for more information. + * * Returns: @conn's peer's certificate errors * * Since: 2.28 diff --git a/gio/gtlsdatabase.c b/gio/gtlsdatabase.c index 2e5a264e9..13a0b6b68 100644 --- a/gio/gtlsdatabase.c +++ b/gio/gtlsdatabase.c @@ -485,13 +485,21 @@ g_tls_database_class_init (GTlsDatabaseClass *klass) * used. * * If @chain is found to be valid, then the return value will be 0. If - * @chain is found to be invalid, then the return value will indicate - * the problems found. If the function is unable to determine whether - * @chain is valid or not (eg, because @cancellable is triggered - * before it completes) then the return value will be - * %G_TLS_CERTIFICATE_GENERIC_ERROR and @error will be set - * accordingly. @error is not set when @chain is successfully analyzed - * but found to be invalid. + * @chain is found to be invalid, then the return value will indicate at + * least one problem found. If the function is unable to determine + * whether @chain is valid (for example, because @cancellable is + * triggered before it completes) then the return value will be + * %G_TLS_CERTIFICATE_GENERIC_ERROR and @error will be set accordingly. + * @error is not set when @chain is successfully analyzed but found to + * be invalid. + * + * GLib guarantees that if certificate verification fails, at least one + * error will be set in the return value, but it does not guarantee + * that all possible errors will be set. Accordingly, you may not safely + * decide to ignore any particular type of error. For example, it would + * be incorrect to mask %G_TLS_CERTIFICATE_EXPIRED if you want to allow + * expired certificates, because this could potentially be the only + * error flag set even if other problems exist with the certificate. * * Prior to GLib 2.48, GLib's default TLS backend modified @chain to * represent the certification path built by #GTlsDatabase during From 38de97c14804f41597a2be1f662f74c951040d90 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 4 Aug 2021 15:20:03 -0500 Subject: [PATCH 2/3] gtlsclientconnection: deprecate validation-flags property It doesn't work as expected, and you shouldn't be trying to use it anyway. --- gio/gtlsclientconnection.c | 29 ++++++++++++++++++++++++++++- gio/gtlsclientconnection.h | 4 ++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/gio/gtlsclientconnection.c b/gio/gtlsclientconnection.c index d0a740f4f..63dd6bee7 100644 --- a/gio/gtlsclientconnection.c +++ b/gio/gtlsclientconnection.c @@ -59,7 +59,21 @@ g_tls_client_connection_default_init (GTlsClientConnectionInterface *iface) * ways indicated here will be rejected unless the application * overrides the default via #GTlsConnection::accept-certificate. * + * GLib guarantees that if certificate verification fails, at least one + * flag will be set, but it does not guarantee that all possible flags + * will be set. Accordingly, you may not safely decide to ignore any + * particular type of error. For example, it would be incorrect to mask + * %G_TLS_CERTIFICATE_EXPIRED if you want to allow expired certificates, + * because this could potentially be the only error flag set even if + * other problems exist with the certificate. Therefore, there is no + * safe way to use this property. This is not a horrible problem, + * though, because you should not be attempting to ignore validation + * errors anyway. If you really must ignore TLS certificate errors, + * connect to #GTlsConnection::accept-certificate. + * * Since: 2.28 + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. */ g_object_interface_install_property (iface, g_param_spec_flags ("validation-flags", @@ -69,7 +83,8 @@ g_tls_client_connection_default_init (GTlsClientConnectionInterface *iface) G_TLS_CERTIFICATE_VALIDATE_ALL, G_PARAM_READWRITE | G_PARAM_CONSTRUCT | - G_PARAM_STATIC_STRINGS)); + G_PARAM_STATIC_STRINGS | + G_PARAM_DEPRECATED)); /** * GTlsClientConnection:server-identity: @@ -183,9 +198,15 @@ g_tls_client_connection_new (GIOStream *base_io_stream, * * Gets @conn's validation flags * + * This function does not work as originally designed and is impossible + * to use correctly. See #GTlsClientConnection:validation-flags for more + * information. + * * Returns: the validation flags * * Since: 2.28 + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. */ GTlsCertificateFlags g_tls_client_connection_get_validation_flags (GTlsClientConnection *conn) @@ -207,7 +228,13 @@ g_tls_client_connection_get_validation_flags (GTlsClientConnection *conn) * checks performed when validating a server certificate. By default, * %G_TLS_CERTIFICATE_VALIDATE_ALL is used. * + * This function does not work as originally designed and is impossible + * to use correctly. See #GTlsClientConnection:validation-flags for more + * information. + * * Since: 2.28 + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. */ void g_tls_client_connection_set_validation_flags (GTlsClientConnection *conn, diff --git a/gio/gtlsclientconnection.h b/gio/gtlsclientconnection.h index 29dbafcf4..f592fa808 100644 --- a/gio/gtlsclientconnection.h +++ b/gio/gtlsclientconnection.h @@ -59,9 +59,9 @@ GIOStream * g_tls_client_connection_new (GIOStream GSocketConnectable *server_identity, GError **error); -GLIB_AVAILABLE_IN_ALL +GLIB_DEPRECATED_IN_2_72 GTlsCertificateFlags g_tls_client_connection_get_validation_flags (GTlsClientConnection *conn); -GLIB_AVAILABLE_IN_ALL +GLIB_DEPRECATED_IN_2_72 void g_tls_client_connection_set_validation_flags (GTlsClientConnection *conn, GTlsCertificateFlags flags); GLIB_AVAILABLE_IN_ALL From d1e9e0c094a28972df725e3bb07b28e4dd7fb390 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 4 Aug 2021 15:20:41 -0500 Subject: [PATCH 3/3] gsocketclient: deprecate tls-validation-flags property It doesn't work as expected, and you shouldn't be trying to use it anyway. --- gio/gsocketclient.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- gio/gsocketclient.h | 4 ++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c index 62b1afbcd..08505fc2e 100644 --- a/gio/gsocketclient.c +++ b/gio/gsocketclient.c @@ -289,7 +289,9 @@ g_socket_client_get_property (GObject *object, break; case PROP_TLS_VALIDATION_FLAGS: +G_GNUC_BEGIN_IGNORE_DEPRECATIONS g_value_set_flags (value, g_socket_client_get_tls_validation_flags (client)); +G_GNUC_END_IGNORE_DEPRECATIONS break; case PROP_PROXY_RESOLVER: @@ -340,7 +342,9 @@ g_socket_client_set_property (GObject *object, break; case PROP_TLS_VALIDATION_FLAGS: +G_GNUC_BEGIN_IGNORE_DEPRECATIONS g_socket_client_set_tls_validation_flags (client, g_value_get_flags (value)); +G_GNUC_END_IGNORE_DEPRECATIONS break; case PROP_PROXY_RESOLVER: @@ -679,9 +683,15 @@ g_socket_client_set_tls (GSocketClient *client, * Gets the TLS validation flags used creating TLS connections via * @client. * + * This function does not work as originally designed and is impossible + * to use correctly. See #GSocketClient:tls-validation-flags for more + * information. + * * Returns: the TLS validation flags * * Since: 2.28 + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. */ GTlsCertificateFlags g_socket_client_get_tls_validation_flags (GSocketClient *client) @@ -697,7 +707,13 @@ g_socket_client_get_tls_validation_flags (GSocketClient *client) * Sets the TLS validation flags used when creating TLS connections * via @client. The default value is %G_TLS_CERTIFICATE_VALIDATE_ALL. * + * This function does not work as originally designed and is impossible + * to use correctly. See #GSocketClient:tls-validation-flags for more + * information. + * * Since: 2.28 + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. */ void g_socket_client_set_tls_validation_flags (GSocketClient *client, @@ -916,6 +932,29 @@ g_socket_client_class_init (GSocketClientClass *class) G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + + /** + * GSocketClient:tls-validation-flags: + * + * The TLS validation flags used when creating TLS connections. The + * default value is %G_TLS_CERTIFICATE_VALIDATE_ALL. + * + * GLib guarantees that if certificate verification fails, at least one + * flag will be set, but it does not guarantee that all possible flags + * will be set. Accordingly, you may not safely decide to ignore any + * particular type of error. For example, it would be incorrect to mask + * %G_TLS_CERTIFICATE_EXPIRED if you want to allow expired certificates, + * because this could potentially be the only error flag set even if + * other problems exist with the certificate. Therefore, there is no + * safe way to use this property. This is not a horrible problem, + * though, because you should not be attempting to ignore validation + * errors anyway. If you really must ignore TLS certificate errors, + * connect to the #GSocketClient::event signal, wait for it to be + * emitted with %G_SOCKET_CLIENT_TLS_HANDSHAKING, and use that to + * connect to #GTlsConnection::accept-certificate. + * + * Deprecated: 2.72: Do not attempt to ignore validation errors. + */ g_object_class_install_property (gobject_class, PROP_TLS_VALIDATION_FLAGS, g_param_spec_flags ("tls-validation-flags", P_("TLS validation flags"), @@ -924,7 +963,8 @@ g_socket_client_class_init (GSocketClientClass *class) G_TLS_CERTIFICATE_VALIDATE_ALL, G_PARAM_CONSTRUCT | G_PARAM_READWRITE | - G_PARAM_STATIC_STRINGS)); + G_PARAM_STATIC_STRINGS | + G_PARAM_DEPRECATED)); /** * GSocketClient:proxy-resolver: @@ -1209,8 +1249,10 @@ g_socket_client_connect (GSocketClient *client, if (tlsconn) { +G_GNUC_BEGIN_IGNORE_DEPRECATIONS g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn), client->priv->tls_validation_flags); +G_GNUC_END_IGNORE_DEPRECATIONS g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKING, connectable, connection); if (g_tls_connection_handshake (G_TLS_CONNECTION (tlsconn), cancellable, &error_info->tmp_error)) @@ -1635,8 +1677,10 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt) &data->error_info->tmp_error); if (tlsconn) { +G_GNUC_BEGIN_IGNORE_DEPRECATIONS g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn), data->client->priv->tls_validation_flags); +G_GNUC_END_IGNORE_DEPRECATIONS g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_TLS_HANDSHAKING, data->connectable, G_IO_STREAM (tlsconn)); g_tls_connection_handshake_async (G_TLS_CONNECTION (tlsconn), G_PRIORITY_DEFAULT, diff --git a/gio/gsocketclient.h b/gio/gsocketclient.h index f0153450d..8f86ce89f 100644 --- a/gio/gsocketclient.h +++ b/gio/gsocketclient.h @@ -110,9 +110,9 @@ gboolean g_socket_client_get_tls (GSocket GLIB_AVAILABLE_IN_2_28 void g_socket_client_set_tls (GSocketClient *client, gboolean tls); -GLIB_AVAILABLE_IN_2_28 +GLIB_DEPRECATED_IN_2_72 GTlsCertificateFlags g_socket_client_get_tls_validation_flags (GSocketClient *client); -GLIB_AVAILABLE_IN_2_28 +GLIB_DEPRECATED_IN_2_72 void g_socket_client_set_tls_validation_flags (GSocketClient *client, GTlsCertificateFlags flags); GLIB_AVAILABLE_IN_2_36