From dfdd5c26595bea830c6dcd55e8f7647bab6841b5 Mon Sep 17 00:00:00 2001 From: Seonah Moon Date: Thu, 2 Dec 2021 11:06:41 +0900 Subject: [PATCH] openssl: remove manual check for certificate expiration We should rely on OpenSSL to do this for us instead. Doing it here is wrong because we wind up checking certificates that may not actually be used in the final certificate chain constructed by OpenSSL. We don't have any way to know which chain OpenSSL will build from the certificates that we pass to it, so there is no way to safely perform certificate validity checks at the glib-networking level. Fixes #179 Corresponding change for GTlsDatabaseGnutls: https://gitlab.gnome.org/GNOME/glib-networking/-/commit/a2cc9b8e08063745d9ba1091e030fbe43fc5a055 Corresponding change for GTlsCertificateGnutls: https://gitlab.gnome.org/GNOME/glib-networking/-/commit/e1a8d06648328f3c5cb2de5ca016de8ac3ddc2b2 Documented by: https://gitlab.gnome.org/GNOME/glib/-/commit/780af9cff3cc636525a973c3f0c0244f2422a39e Change-Id: I354e8163001cdb4bdd8d06b0d9863debc3a0bf50 --- tls/openssl/gtlscertificate-openssl.c | 17 ----------------- tls/openssl/gtlsfiledatabase-openssl.c | 33 --------------------------------- tls/tests/certificate.c | 8 ++++++-- 3 files changed, 6 insertions(+), 52 deletions(-) diff --git a/tls/openssl/gtlscertificate-openssl.c b/tls/openssl/gtlscertificate-openssl.c index 4bd9f80..d5c0412 100644 --- a/tls/openssl/gtlscertificate-openssl.c +++ b/tls/openssl/gtlscertificate-openssl.c @@ -287,7 +287,6 @@ g_tls_certificate_openssl_verify (GTlsCertificate *cert, GTlsCertificateFlags gtls_flags; X509 *x; STACK_OF(X509) *untrusted; - gint i; cert_openssl = G_TLS_CERTIFICATE_OPENSSL (cert); priv = g_tls_certificate_openssl_get_instance_private (cert_openssl); @@ -336,22 +335,6 @@ g_tls_certificate_openssl_verify (GTlsCertificate *cert, X509_STORE_free (store); } - /* We have to check these ourselves since openssl - * does not give us flags and UNKNOWN_CA will take priority. - */ - for (i = 0; i < sk_X509_num (untrusted); i++) - { - X509 *c = sk_X509_value (untrusted, i); - ASN1_TIME *not_before = X509_get_notBefore (c); - ASN1_TIME *not_after = X509_get_notAfter (c); - - if (X509_cmp_current_time (not_before) > 0) - gtls_flags |= G_TLS_CERTIFICATE_NOT_ACTIVATED; - - if (X509_cmp_current_time (not_after) < 0) - gtls_flags |= G_TLS_CERTIFICATE_EXPIRED; - } - sk_X509_free (untrusted); if (identity) diff --git a/tls/openssl/gtlsfiledatabase-openssl.c b/tls/openssl/gtlsfiledatabase-openssl.c index e45a619..6eb1c65 100644 --- a/tls/openssl/gtlsfiledatabase-openssl.c +++ b/tls/openssl/gtlsfiledatabase-openssl.c @@ -527,34 +527,6 @@ g_tls_file_database_openssl_lookup_certificates_issued_by (GTlsDatabase return issued; } -static GTlsCertificateFlags -double_check_before_after_dates (GTlsCertificateOpenssl *chain) -{ - GTlsCertificateFlags gtls_flags = 0; - X509 *cert; - - while (chain) - { - ASN1_TIME *not_before; - ASN1_TIME *not_after; - - cert = g_tls_certificate_openssl_get_cert (chain); - not_before = X509_get_notBefore (cert); - not_after = X509_get_notAfter (cert); - - if (X509_cmp_current_time (not_before) > 0) - gtls_flags |= G_TLS_CERTIFICATE_NOT_ACTIVATED; - - if (X509_cmp_current_time (not_after) < 0) - gtls_flags |= G_TLS_CERTIFICATE_EXPIRED; - - chain = G_TLS_CERTIFICATE_OPENSSL (g_tls_certificate_get_issuer - (G_TLS_CERTIFICATE (chain))); - } - - return gtls_flags; -} - static STACK_OF(X509) * convert_certificate_chain_to_openssl (GTlsCertificateOpenssl *chain) { @@ -626,11 +598,6 @@ g_tls_file_database_openssl_verify_chain (GTlsDatabase *database, if (g_cancellable_set_error_if_cancelled (cancellable, error)) return G_TLS_CERTIFICATE_GENERIC_ERROR; - /* We have to check these ourselves since openssl - * does not give us flags and UNKNOWN_CA will take priority. - */ - result |= double_check_before_after_dates (G_TLS_CERTIFICATE_OPENSSL (chain)); - if (identity) result |= g_tls_certificate_openssl_verify_identity (G_TLS_CERTIFICATE_OPENSSL (chain), identity); diff --git a/tls/tests/certificate.c b/tls/tests/certificate.c index 3310269..9ca543b 100644 --- a/tls/tests/certificate.c +++ b/tls/tests/certificate.c @@ -510,13 +510,17 @@ test_verify_certificate_bad_combo (TestVerify *test, * - Use unrelated cert as CA * - Use wrong identity. * - Use expired certificate. + * + * Once upon a time, we might have asserted to see that all of these errors + * are set. But this is impossible to do correctly, so nowadays we only + * guarantee that at least one error will be set. See glib-networking#179 and + * glib!2214 for rationale. */ identity = g_network_address_new ("other.example.com", 80); errors = g_tls_certificate_verify (cert, identity, cacert); - g_assert_cmpuint (errors, ==, G_TLS_CERTIFICATE_UNKNOWN_CA | - G_TLS_CERTIFICATE_BAD_IDENTITY | G_TLS_CERTIFICATE_EXPIRED); + g_assert_cmpuint (errors, !=, 0); g_object_unref (cert); g_object_unref (cacert); -- 2.7.4