[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