[PATCH] catch expired certificates in the chain

Joe Orton joe at manyfish.co.uk
Wed Feb 11 07:47:32 EST 2009


On Mon, Feb 02, 2009 at 05:35:49PM +0100, Ludwig Nussel wrote:
> Next try. This time with separate failure bits. I don't know how to
> implement that for gnutls though.

Hiya - thanks for reposting.

I had a look into this some more.

1) I think it's sufficient to give a single new NE_SSL_* failure bit to 
indicate "a certificate higher in the cert chain is outside its validity 
period".  I had a modified version of your patch calling this 
NE_SSL_BADCHAIN.

2) I don't think that it's possible to rely on the return value of 
SSL_get_verify_result() to catch this case - in the case where the cert 
itself is also untrusted, only the first branch of the switch will be 
taken.

3) NE_SSL_FAILMASK needs to be bumped to cover the new failure bits.

This is what I have in my current wc: it is over-complicated since it 
requires doing a re-verification of the cert.  It should be possible to 
hook into the OpenSSL verify callback (SSL_CTX_set_verify) to do this 
properly but my naive attempts to do so caused test failures.

Index: src/ne_openssl.c
===================================================================
--- src/ne_openssl.c	(revision 1617)
+++ src/ne_openssl.c	(working copy)
@@ -343,30 +343,47 @@
 }
 
 /* Return a linked list of certificate objects from an OpenSSL chain. */
-static ne_ssl_certificate *make_chain(STACK_OF(X509) *chain)
+static ne_ssl_certificate *make_chain(ne_session *sess, STACK_OF(X509) *chain)
 {
-    int n, count = sk_X509_num(chain);
-    ne_ssl_certificate *top = NULL, *current = NULL;
-    
+    int n = 0, count = sk_X509_num(chain);
+    ne_ssl_certificate *top = NULL, **nextptr = ⊤
+    X509_STORE_CTX ctx;
+    X509 *x5, *next;
+
+    X509_STORE_CTX_init(&ctx, SSL_CTX_get_cert_store(sess->ssl_context->ctx),
+                        sk_X509_value(chain, 0), chain);
+
+    X509_verify_cert(&ctx); /* ignore return value. */
+
+    next = X509_dup(sk_X509_value(chain, 0));
+
     NE_DEBUG(NE_DBG_SSL, "Chain depth: %d\n", count);
 
-    for (n = 0; n < count; n++) {
+    do {
         ne_ssl_certificate *cert = ne_malloc(sizeof *cert);
-        populate_cert(cert, X509_dup(sk_X509_value(chain, n)));
+
+        x5 = next;
+
+        populate_cert(cert, x5);
+        
 #ifdef NE_DEBUGGING
         if (ne_debug_mask & NE_DBG_SSL) {
-            fprintf(ne_debug_stream, "Cert #%d:\n", n);
+            fprintf(ne_debug_stream, "Cert %d:\n", n++);
             X509_print_fp(ne_debug_stream, cert->subject);
         }
 #endif
-        if (top == NULL) {
-            current = top = cert;
-        } else {
-            current->issuer = cert;
-            current = cert;
-        }
-    }
 
+        *nextptr = cert;
+        nextptr = &cert->issuer;
+        
+        /* Loop whilst the issuer can be retrieved and the issuer
+         * is not the same cert as this (any complete chain will
+         * end in a self-signed cert).  */
+    } while (X509_STORE_CTX_get1_issuer(&next, &ctx, x5) == 1
+             && X509_cmp(next, x5) != 0);
+
+    X509_STORE_CTX_cleanup(&ctx);
+
     return top;
 }
 
@@ -374,18 +391,23 @@
 static int check_certificate(ne_session *sess, SSL *ssl, ne_ssl_certificate *chain)
 {
     X509 *cert = chain->subject;
-    ASN1_TIME *notBefore = X509_get_notBefore(cert);
-    ASN1_TIME *notAfter = X509_get_notAfter(cert);
     int ret, failures = 0;
     long result;
     ne_uri server;
 
-    /* check expiry dates */
-    if (X509_cmp_current_time(notBefore) >= 0)
-	failures |= NE_SSL_NOTYETVALID;
-    else if (X509_cmp_current_time(notAfter) <= 0)
-	failures |= NE_SSL_EXPIRED;
+    /* Check cert expiry/validity times. */
+    {
+        ne_ssl_certificate *c;
 
+        for (c = chain; c; c = c->issuer) {
+            if (X509_cmp_current_time(X509_get_notBefore(c->subject)) >= 0)
+                failures |= (c == chain) ? NE_SSL_NOTYETVALID : NE_SSL_BADCHAIN;
+            else if (X509_cmp_current_time(X509_get_notAfter(c->subject)) <= 0)
+                failures |= (c == chain) ? NE_SSL_EXPIRED : NE_SSL_BADCHAIN;
+            NE_DEBUG(NE_DBG_SSL, "ssl: cert check %d\n", failures);
+        }
+    }
+
     /* Check certificate was issued to this server; pass URI of
      * server. */
     memset(&server, 0, sizeof server);
@@ -411,20 +433,22 @@
 	/* TODO: and probably more result codes here... */
 	failures |= NE_SSL_UNTRUSTED;
 	break;
-	/* ignore these, since we've already noticed them: */
+    case X509_V_OK:
+	break;
     case X509_V_ERR_CERT_NOT_YET_VALID:
     case X509_V_ERR_CERT_HAS_EXPIRED:
-        /* cert was trusted: */
-    case X509_V_OK:
-	break;
+        /* Should have been caught in the expiry checks above... */
+        NE_DEBUG(NE_DBG_SSL, "ssl: validity failure, %d vs %d\n",
+                 failures, failures & (NE_SSL_NOTYETVALID | NE_SSL_EXPIRED | NE_SSL_BADCHAIN));
+        if (failures & (NE_SSL_NOTYETVALID | NE_SSL_EXPIRED | NE_SSL_BADCHAIN))
+            break;
+        /* ...but if not, then fallthrough to hard failure since this
+         * is some case that was not anticipated... */
     default:
-	/* TODO: tricky to handle the 30-odd failure cases OpenSSL
-	 * presents here (see x509_vfy.h), and present a useful API to
-	 * the application so it in turn can then present a meaningful
-	 * UI to the user.  The only thing to do really would be to
-	 * pass back the error string, but that's not localisable.  So
-	 * just fail the verification here - better safe than
-	 * sorry. */
+        /* Semantics of other errors are not mapped to a particular
+         * error and exposed via the API, so such cases muts cause
+         * hard failure with the appropriate error string (sadly,
+         * untranslated) from OpenSSL: */
 	ne_set_error(sess, _("Certificate verification error: %s"),
 		     X509_verify_cert_error_string(result));
 	return NE_ERROR;
@@ -658,7 +682,7 @@
 	 * verify it again. */
     } else {
 	/* new connection: create the chain. */
-        ne_ssl_certificate *cert = make_chain(chain);
+        ne_ssl_certificate *cert = make_chain(sess, chain);
 
         if (freechain) sk_X509_free(chain); /* no longer need the chain */
 
Index: src/ne_session.h
===================================================================
--- src/ne_session.h	(revision 1617)
+++ src/ne_session.h	(working copy)
@@ -1,6 +1,6 @@
 /* 
    HTTP session handling
-   Copyright (C) 1999-2008, Joe Orton <joe at manyfish.co.uk>
+   Copyright (C) 1999-2009, Joe Orton <joe at manyfish.co.uk>
 
    This library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Library General Public
@@ -218,11 +218,18 @@
  * not trusted: there is no indicatation the server is who they claim
  * to be: */
 #define NE_SSL_UNTRUSTED (0x08)
+/* The certificate chain contained a certificate other than the server
+ * cert which failed verification for a reason other than lack of
+ * trust; for example, due to a CA cert being outside its validity
+ * period: */
+#define NE_SSL_BADCHAIN (0x10)
 
-/* The bitmask of known failure bits: if (failures & ~NE_SSL_FAILMASK)
- * is non-zero, an unrecognized failure is given, and the verification
- * should be failed. */
-#define NE_SSL_FAILMASK (0x0f)
+/* For purposes of forwards-compatibility, the bitmask of all
+ * currently defined failure bits is given as NE_SSL_FAILMASK.  If the
+ * expression (failures & ~NE_SSL_FAILMASK) is non-zero a failure type
+ * is present which the application does not recognize but must treat
+ * as a verification failure nonetheless. */
+#define NE_SSL_FAILMASK (0x1f)
 
 /* A callback which is used when server certificate verification is
  * needed.  The reasons for verification failure are given in the




More information about the neon mailing list