[PATCH] filter expired certs from $certificate_file for OpenSSL verification

[Kevin J. McCarthy]

On Thu, Feb 09, 2017 at 10:56:36PM +0000, isdtor wrote:
> > What I hear you saying is that *with* the expired imap.google.com
> > certificate, you are getting a prompt for an expired Google G2 cert
> > (the 2nd in the chain).  But without the expired imap.google.com you
> > are getting no prompt.  Is that right?
> 
> That is correct. With only two certs in the local store and no cert
> for imap.google.com present, it proceeds straight to the password
> prompt.  It's like the actual server cert is considered optional
> because the rest of the chain checks out.

Hi isdtor,

So, after some research it seems there is a warning about this issue in
the SSL_CTX_load_verify_locations man page.

The attached patch seems to fix the problem for me: it filters out
expired certs for the initial verification/chain construction.  The
entire $certificate_file is still used in the verification callback if
needed, but is just filtered from OpenSSL's trusted stored.

I'd appreciate if you could give this a try.

-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

# HG changeset patch
# User Kevin McCarthy <ke...@8t8.us>
# Date 1486852952 28800
#      Sat Feb 11 14:42:32 2017 -0800
# Node ID 6f8279f5a3aead18d2ace266d493040d03a90c1d
# Parent  7c97a8af8718ae7807e0ed0f1eb2b11d436a2e91
Filter expired local certs for OpenSSL verification.

OpenSSL has trouble establishing the chain and verifying when
duplicate expired certs are loaded in from $certificate_file.  A
warning about this is mentioned in
SSL_CTX_load_verify_locations(3SSL).

Filter out expired certs when loading verify certs.  Note that the
full certicates file is still used for verification in
check_certificate_by_digest().

diff --git a/mutt_ssl.c b/mutt_ssl.c
--- a/mutt_ssl.c
+++ b/mutt_ssl.c
@@ -81,16 +81,57 @@
 static void ssl_dprint_err_stack (void);
 static int ssl_cache_trusted_cert (X509 *cert);
 static int ssl_verify_callback (int preverify_ok, X509_STORE_CTX *ctx);
 static int interactive_check_cert (X509 *cert, int idx, int len);
 static void ssl_get_client_cert(sslsockdata *ssldata, CONNECTION *conn);
 static int ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
 static int ssl_negotiate (CONNECTION *conn, sslsockdata*);
 
+/* ssl certificate verification can behave strangely if there are expired
+ * certs loaded into the trusted store.  This function filters out expired
+ * certs.
+ * Previously the code used this form:
+ *     SSL_CTX_load_verify_locations (ssldata->ctx, SslCertFile, NULL);
+ */
+static int ssl_load_certificates (SSL_CTX *ctx)
+{
+  FILE *fp;
+  X509 *cert;
+  X509_STORE *store;
+  char buf[STRING];
+
+  dprint (2, (debugfile, "ssl_load_certificates: loading trusted 
certificates\n"));
+  store = SSL_CTX_get_cert_store (ctx);
+  if (!store)
+  {
+    store = X509_STORE_new ();
+    SSL_CTX_set_cert_store (ctx, store);
+  }
+
+  if ((fp = fopen (SslCertFile, "rt")) == NULL)
+    return 0;
+
+  while ((cert = PEM_read_X509 (fp, NULL, NULL, NULL)) != NULL)
+  {
+    if ((X509_cmp_current_time (X509_get_notBefore (cert)) >= 0) ||
+        (X509_cmp_current_time (X509_get_notAfter (cert)) <= 0))
+    {
+      dprint (2, (debugfile, "ssl_load_certificates: filtering expired cert: 
%s\n",
+              X509_NAME_oneline (X509_get_subject_name (cert), buf, sizeof 
(buf))));
+      X509_free (cert);
+    }
+    else
+      X509_STORE_add_cert (store, cert);
+  }
+  safe_fclose (&fp);
+
+  return 1;
+}
+
 /* mutt_ssl_starttls: Negotiate TLS over an already opened connection.
  *   TODO: Merge this code better with ssl_socket_open. */
 int mutt_ssl_starttls (CONNECTION* conn)
 {
   sslsockdata* ssldata;
   int maxbits;
   long ssl_options = 0;
 
@@ -139,17 +180,17 @@
   {
     if (! SSL_CTX_set_default_verify_paths (ssldata->ctx))
     {
       dprint (1, (debugfile, "mutt_ssl_starttls: Error setting default verify 
paths\n"));
       goto bail_ctx;
     }
   }
 
-  if (SslCertFile && ! SSL_CTX_load_verify_locations (ssldata->ctx, 
SslCertFile, NULL))
+  if (SslCertFile && !ssl_load_certificates (ssldata->ctx))
     dprint (1, (debugfile, "mutt_ssl_starttls: Error loading trusted 
certificates\n"));
 
   ssl_get_client_cert(ssldata, conn);
 
   if (SslCiphers) {
     if (!SSL_CTX_set_cipher_list (ssldata->ctx, SslCiphers)) {
       dprint (1, (debugfile, "mutt_ssl_starttls: Could not select preferred 
ciphers\n"));
       goto bail_ctx;
@@ -389,17 +430,17 @@
     if (! SSL_CTX_set_default_verify_paths (data->ctx))
     {
       dprint (1, (debugfile, "ssl_socket_open: Error setting default verify 
paths\n"));
       mutt_socket_close (conn);
       return -1;
     }
   }
 
-  if (SslCertFile && ! SSL_CTX_load_verify_locations (data->ctx, SslCertFile, 
NULL))
+  if (SslCertFile && !ssl_load_certificates (data->ctx))
     dprint (1, (debugfile, "ssl_socket_open: Error loading trusted 
certificates\n"));
 
   ssl_get_client_cert(data, conn);
 
   if (SslCiphers) {
     SSL_CTX_set_cipher_list (data->ctx, SslCiphers);
   }
 
@@ -699,17 +740,17 @@
     {
       dprint (2, (debugfile, "Server certificate is not yet valid\n"));
       mutt_error (_("Server certificate is not yet valid"));
       mutt_sleep (2);
       return 0;
     }
     if (X509_cmp_current_time (X509_get_notAfter (peercert)) <= 0)
     {
-      dprint (2, (debugfile, "Server certificate has expired"));
+      dprint (2, (debugfile, "Server certificate has expired\n"));
       mutt_error (_("Server certificate has expired"));
       mutt_sleep (2);
       return 0;
     }
   }
 
   if ((fp = fopen (SslCertFile, "rt")) == NULL)
     return 0;

signature.asc
Description: PGP signature