[PATCH] catch expired certificates in the chain

Joe Orton joe at manyfish.co.uk
Thu Feb 12 10:20:24 EST 2009


On Wed, Feb 11, 2009 at 12:47:32PM +0000, Joe Orton wrote:
> 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.

I got it working via that callback, and it's much simpler.  I'll commit 
the below soon.

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,19 @@
  * 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)
+/* N.B.: 0x20 is reserved. */
 
-/* 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
Index: src/ne_openssl.c
===================================================================
--- src/ne_openssl.c	(revision 1617)
+++ src/ne_openssl.c	(working copy)
@@ -1,6 +1,6 @@
 /* 
    neon SSL/TLS support using OpenSSL
-   Copyright (C) 2002-2008, Joe Orton <joe at manyfish.co.uk>
+   Copyright (C) 2002-2009, Joe Orton <joe at manyfish.co.uk>
    Portions are:
    Copyright (C) 1999-2000 Tommi Komulainen <Tommi.Komulainen at iki.fi>
 
@@ -84,6 +84,8 @@
     char *friendly_name;
 };
 
+#define NE_SSL_UNHANDLED (0x20) /* failure bit for unhandled case. */
+
 /* Append an ASN.1 DirectoryString STR to buffer BUF as UTF-8.
  * Returns zero on success or non-zero on error. */
 static int append_dirstring(ne_buffer *buf, ASN1_STRING *str)
@@ -342,6 +344,61 @@
     return cert;
 }
 
+/* OpenSSL cert verification callback.  This is invoked for *each*
+ * error which is encoutered whilst verifying the cert chain; multiple
+ * invocations for any particular cert in the chain are possible. */
+static int verify_callback(int ok, X509_STORE_CTX *ctx)
+{
+    /* OpenSSL, living in its own little happy world of global state,
+     * where userdata was just a twinkle in the eye of an API designer
+     * yet to be born.  Or... "Seriously, wtf?"  */
+    SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, 
+                                          SSL_get_ex_data_X509_STORE_CTX_idx());
+    ne_session *sess = SSL_get_app_data(ssl);
+    int depth = X509_STORE_CTX_get_error_depth(ctx);
+    int err = X509_STORE_CTX_get_error(ctx);
+    int failures = 0;
+
+    /* If there's no error, nothing to do here. */
+    if (ok) return ok;
+
+    NE_DEBUG(NE_DBG_SSL, "ssl: Verify callback @ %d => %d\n", depth, err);
+
+    /* Map the error code onto any of the exported cert validation
+     * errors, if possible. */
+    switch (err) {
+    case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY:
+    case X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN:
+    case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
+    case X509_V_ERR_CERT_UNTRUSTED:
+    case X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE:
+	failures |= NE_SSL_UNTRUSTED;
+	break;
+    case X509_V_ERR_CERT_NOT_YET_VALID:
+        failures |= depth > 0 ? NE_SSL_BADCHAIN : NE_SSL_NOTYETVALID;
+        break;
+    case X509_V_ERR_CERT_HAS_EXPIRED:
+        failures |= depth > 0 ? NE_SSL_BADCHAIN : NE_SSL_EXPIRED;
+        break;
+    case X509_V_OK:
+        break;
+    default:
+        /* Clear the failures bitmask so check_certificate knows this
+         * is a bailout. */
+        sess->ssl_context->failures |= NE_SSL_UNHANDLED;
+        NE_DEBUG(NE_DBG_SSL, "ssl: Unhandled verification error %d -> %s\n", 
+                 err, X509_verify_cert_error_string(err));
+        return 0;
+    }
+
+    sess->ssl_context->failures |= failures;
+
+    NE_DEBUG(NE_DBG_SSL, "ssl: Verify failures |= %d => %d\n", failures,
+             sess->ssl_context->failures);
+    
+    return 1;
+}
+
 /* Return a linked list of certificate objects from an OpenSSL chain. */
 static ne_ssl_certificate *make_chain(STACK_OF(X509) *chain)
 {
@@ -374,18 +431,22 @@
 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;
+    int ret, failures = sess->ssl_context->failures;
     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;
+    /* If the verification callback hit a case which can't be mapped
+     * to one of the exported error bits, it's treated as a hard
+     * failure rather than invoking the callback, which can't present
+     * a useful error to the user.  "Um, something is wrong.  OK?" */
+    if (failures & NE_SSL_UNHANDLED) {
+        long result = SSL_get_verify_result(ssl);
 
+        ne_set_error(sess, _("Certificate verification error: %s"),
+                    X509_verify_cert_error_string(result));
+
+        return NE_ERROR;
+    }
+
     /* Check certificate was issued to this server; pass URI of
      * server. */
     memset(&server, 0, sizeof server);
@@ -398,38 +459,6 @@
         return NE_ERROR;
     } else if (ret > 0) failures |= NE_SSL_IDMISMATCH;
 
-    /* get the result of the cert verification out of OpenSSL */
-    result = SSL_get_verify_result(ssl);
-
-    NE_DEBUG(NE_DBG_SSL, "Verify result: %ld = %s\n", result,
-	     X509_verify_cert_error_string(result));
-
-    switch (result) {
-    case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY:
-    case X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN:
-    case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
-	/* TODO: and probably more result codes here... */
-	failures |= NE_SSL_UNTRUSTED;
-	break;
-	/* ignore these, since we've already noticed them: */
-    case X509_V_ERR_CERT_NOT_YET_VALID:
-    case X509_V_ERR_CERT_HAS_EXPIRED:
-        /* cert was trusted: */
-    case X509_V_OK:
-	break;
-    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. */
-	ne_set_error(sess, _("Certificate verification error: %s"),
-		     X509_verify_cert_error_string(result));
-	return NE_ERROR;
-    }
-
     if (failures == 0) {
         /* verified OK! */
         ret = NE_OK;
@@ -524,6 +553,7 @@
         SSL_CTX_set_client_cert_cb(ctx->ctx, provide_client_cert);
         /* enable workarounds for buggy SSL server implementations */
         SSL_CTX_set_options(ctx->ctx, SSL_OP_ALL);
+        SSL_CTX_set_verify(ctx->ctx, SSL_VERIFY_PEER, verify_callback);
     } else if (mode == NE_SSL_CTX_SERVER) {
         ctx->ctx = SSL_CTX_new(SSLv23_server_method());
         SSL_CTX_set_session_cache_mode(ctx->ctx, SSL_SESS_CACHE_CLIENT);
@@ -608,6 +638,7 @@
         sess->flags[NE_SESSFLAG_TLS_SNI] ? sess->server.hostname : NULL;
 
     sess->ssl_cc_requested = 0;
+    ctx->failures = 0;
 
     if (ne_sock_connect_ssl(sess->socket, ctx, sess)) {
 	if (ctx->sess) {
Index: src/ne_privssl.h
===================================================================
--- src/ne_privssl.h	(revision 1617)
+++ src/ne_privssl.h	(working copy)
@@ -1,6 +1,6 @@
 /* 
    SSL interface definitions internal to neon.
-   Copyright (C) 2003-2005, 2008, Joe Orton <joe at manyfish.co.uk>
+   Copyright (C) 2003-2005, 2008, 2009, Joe Orton <joe at manyfish.co.uk>
    Copyright (C) 2004, Aleix Conchillo Flaque <aleix at member.fsf.org>
 
    This library is free software; you can redistribute it and/or
@@ -40,6 +40,7 @@
     SSL_CTX *ctx;
     SSL_SESSION *sess;
     const char *hostname; /* for SNI */
+    int failures; /* bitmask of exposed failure bits. */
 };
 
 typedef SSL *ne_ssl_socket;
Index: src/ne_gnutls.c
===================================================================
--- src/ne_gnutls.c	(revision 1617)
+++ src/ne_gnutls.c	(working copy)
@@ -456,7 +456,9 @@
     return match ? 0 : 1;
 }
 
-/* Populate an ne_ssl_certificate structure from an X509 object. */
+/* Populate an ne_ssl_certificate structure from an X509 object.  Note
+ * that x5 is owned by returned cert object and must not be otherwise
+ * freed by the caller.  */
 static ne_ssl_certificate *populate_cert(ne_ssl_certificate *cert,
                                          gnutls_x509_crt x5)
 {
@@ -778,15 +780,22 @@
     time_t before, after, now = time(NULL);
     int ret, failures = 0;
     ne_uri server;
+    ne_ssl_certificate *cert;
 
-    before = gnutls_x509_crt_get_activation_time(chain->subject);
-    after = gnutls_x509_crt_get_expiration_time(chain->subject);
+    /* Check that all certs within the chain are inside their defined
+     * validity period.  Note that the errors flagged for the server
+     * cert are different from the generic error for issues higher up
+     * the chain. */
+    for (cert = chain; cert; cert = cert->issuer) {
+        before = gnutls_x509_crt_get_activation_time(chain->subject);
+        after = gnutls_x509_crt_get_expiration_time(chain->subject);
+        
+        if (now < before)
+            failures |= (cert == chain) ? NE_SSL_NOTYETVALID : NE_SSL_BADCHAIN;
+        else if (now > after)
+            failures |= (cert == chain) ? NE_SSL_EXPIRED : NE_SSL_BADCHAIN;
+    }        
 
-    if (now < before)
-        failures |= NE_SSL_NOTYETVALID;
-    else if (now > after)
-        failures |= NE_SSL_EXPIRED;
-
     memset(&server, 0, sizeof server);
     ne_fill_server_uri(sess, &server);
     ret = check_identity(&server, chain->subject, NULL);
Index: test/makekeys.sh
===================================================================
--- test/makekeys.sh	(revision 1617)
+++ test/makekeys.sh	(working copy)
@@ -12,7 +12,8 @@
 
 REQDN=reqDN
 STRMASK=default
-export REQDN STRMASK
+CADIR=./ca
+export REQDN STRMASK CADIR
 
 asn1date() {
 	date -d "$1" "+%y%m%d%H%M%SZ"
@@ -22,17 +23,14 @@
 
 set -ex
 
-rm -rf ca ca2
-mkdir ca
-touch ca/index.txt
-echo 01 > ca/serial
+for i in ca ca1 ca2 ca3; do
+    rm -rf $i
+    mkdir $i
+    touch $i/index.txt
+    echo 01 > $i/serial
+    ${OPENSSL} genrsa -rand ${srcdir}/../configure > $i/key.pem
+done
 
-mkdir ca2
-touch ca2/index.txt
-echo 01 > ca2/serial
-
-${OPENSSL} genrsa -rand ${srcdir}/../configure > ca/key.pem
-${OPENSSL} genrsa -rand ${srcdir}/../configure > ca2/key.pem
 ${OPENSSL} genrsa -rand ${srcdir}/../configure > client.key
 
 ${OPENSSL} dsaparam -genkey -rand ${srcdir}/../configure 1024 > client.dsap
@@ -72,6 +70,16 @@
 csr_fields IntermediaryCA | ${REQ} -new -key ca2/key.pem -out ca2.csr
 ${CA} -extensions caExt -days 3560 -in ca2.csr -out ca2/cert.pem
 
+csr_fields ExpiredCA | ${REQ} -new -key ca1/key.pem -out ca1/cert.csr
+
+csr_fields NotYetValidCA | ${REQ} -new -key ca3/key.pem -out ca3/cert.csr
+
+CADIR=./ca1 ${CA} -name neoncainit -startdate `asn1date "2 days ago"` -enddate `asn1date "yesterday"` \
+  -in ca1/cert.csr -keyfile ca1/key.pem -out ca1/cert.pem -selfsign
+
+CADIR=./ca3 ${CA} -name neoncainit -startdate `asn1date "1 year"` -enddate `asn1date "2 years"` \
+  -in ca3/cert.csr -keyfile ca3/key.pem -out ca3/cert.pem -selfsign
+
 csr_fields | ${REQ} -new -key ${srcdir}/server.key -out server.csr
 
 csr_fields | ${REQ} -new -key ${srcdir}/server.key -out expired.csr
@@ -190,11 +198,17 @@
 done
 
 # Sign this CSR using the intermediary CA
-${CA} -name neonca2 -days 900 -in server.csr -out ca2server.cert
+CADIR=./ca2 ${CA} -days 900 -in server.csr -out ca2server.cert
 # And create a file with the concatenation of both EE and intermediary
 # cert.
 cat ca2server.cert ca2/cert.pem > ca2server.pem
+ 
+# sign with expired CA
+CADIR=./ca1 ${CA} -days 3 -in server.csr -out ca1server.cert
 
+# sign with not yet valid CA
+CADIR=./ca3 ${CA} -days 3 -in server.csr -out ca3server.cert
+
 MKPKCS12="${OPENSSL} pkcs12 -export -passout stdin -in client.cert -inkey client.key"
 
 # generate a PKCS12 cert from the client cert: -passOUT because it's the
Index: test/ssl.c
===================================================================
--- test/ssl.c	(revision 1617)
+++ test/ssl.c	(working copy)
@@ -864,6 +864,18 @@
                             "subjaltname not honored", NE_SSL_IDMISMATCH);
 }
 
+static int fail_ca_expired(void)
+{
+    return fail_ssl_request("ca1server.cert", "ca1/cert.pem", "localhost",
+                            "isser ca expired", NE_SSL_BADCHAIN);
+}
+
+static int fail_ca_notyetvalid(void)
+{
+    return fail_ssl_request("ca3server.cert", "ca3/cert.pem", "localhost",
+                            "isser ca not yet valid", NE_SSL_BADCHAIN);
+}
+
 /* Test that the SSL session is cached across connections. */
 static int session_cache(void)
 {
@@ -1763,6 +1775,8 @@
     T(fail_bad_ipaltname),
     T(fail_bad_urialtname),
     T(fail_wildcard),
+    T(fail_ca_notyetvalid),
+    T(fail_ca_expired),
 
     T(session_cache),
 	
Index: test/openssl.conf
===================================================================
--- test/openssl.conf	(revision 1617)
+++ test/openssl.conf	(working copy)
@@ -2,7 +2,7 @@
 default_ca = neonca
 
 [neonca]
-dir = ./ca
+dir = ${ENV::CADIR}
 database = $dir/index.txt
 new_certs_dir = $dir
 certificate = $dir/cert.pem
@@ -13,17 +13,19 @@
 x509_extensions = issuedExt
 unique_subject = no
 
-[neonca2]
-dir = ./ca2
+# same as neonca1 just +basicConstraints and without certificate to
+# allow creation of the initial self signed certificate
+[neoncainit]
+dir = ${ENV::CADIR}
 database = $dir/index.txt
 new_certs_dir = $dir
-certificate = $dir/cert.pem
 serial = $dir/serial
 private_key = $dir/key.pem
 policy = policy_any
 default_md = md5
 x509_extensions = issuedExt
 unique_subject = no
+basicConstraints = CA:TRUE
 
 [policy_any]
 countryName = optional





More information about the neon mailing list