dovecot - Feb 2015 - [PATCH] Fix for client certificate validation does not work

Hi all,

As I reported earlier (with a typo in the work [BUG]) client
certification validation *does not* work even if you do everything
exactly according to all documentation and attempts at helpful advice.

I have seen this issue with both startssl.com and self-signed
certificates, and based on what I've seen from searching the web, this
is a problem that has gotten little attention because most people don't
bother, but are more than willing to give out useless advice on how to
make it work.

Furthermore the issue does NOT occur with the cyrus-imap mail server, so
it is definitely a server-side issue.

The actual issue is that the code for calling OpenSSL that constructs
the client certificate validation is in fact WRONG.

I don't have a perfect patch as I was mostly interested in getting it
working for my needs and didn't bother with constructing the list of CA
names to send to the client, preferring to let OpenSSL handle all that
sort of thing.

What it comes down to is that the code, which probably worked at one
point, was not correctly updated at some point and since then client
side certificate validation has been BROKEN.

I have patched against 2.2.9, however I have seen this problem in the
versions in both Debian Wheezy and Debian Jessie as well.

As you will see from the patch (which is an attachment as people tend to
complain that patches get mangled when you inline them, and even if I
have a good client I've gotten heck because the receiver didn't.

Regards,

Daniel

-------------- next part --------------
Index: dovecot-2.2.9/src/login-common/ssl-proxy-openssl.c
==================================================================---
dovecot-2.2.9.orig/src/login-common/ssl-proxy-openssl.c	2015-02-11
00:31:24.986198000 -0500
+++ dovecot-2.2.9/src/login-common/ssl-proxy-openssl.c	2015-02-11
00:32:19.262198000 -0500
@@ -951,54 +951,25 @@
 	return strstr(cert, "PRIVATE KEY---") != NULL;
 }
 
-static void load_ca(X509_STORE *store, const char *ca,
-		    STACK_OF(X509_NAME) **xnames_r)
+static void load_ca(SSL_CTX *ssl_ctx, const char *ca)
 {
-	/* mostly just copy&pasted from X509_load_cert_crl_file() */
-	STACK_OF(X509_INFO) *inf;
-	X509_INFO *itmp;
-	X509_NAME *xname;
-	BIO *bio;
-	int i;
-
-	bio = BIO_new_mem_buf(t_strdup_noconst(ca), strlen(ca));
-	if (bio == NULL)
-		i_fatal("BIO_new_mem_buf() failed");
-	inf = PEM_X509_INFO_read_bio(bio, NULL, NULL, NULL);
-	if (inf == NULL)
-		i_fatal("Couldn't parse ssl_ca: %s", ssl_last_error());
-	BIO_free(bio);
-
-	if (xnames_r != NULL) {
-		*xnames_r = sk_X509_NAME_new_null();
-		if (*xnames_r == NULL)
-			i_fatal_status(FATAL_OUTOFMEM, "sk_X509_NAME_new_null() failed");
-	}
-	for(i = 0; i < sk_X509_INFO_num(inf); i++) {
-		itmp = sk_X509_INFO_value(inf, i);
-		if(itmp->x509) {
-			X509_STORE_add_cert(store, itmp->x509);
-			xname = X509_get_subject_name(itmp->x509);
-			if (xname != NULL && xnames_r != NULL) {
-				xname = X509_NAME_dup(xname);
-				if (xname == NULL)
-					i_fatal_status(FATAL_OUTOFMEM, "X509_NAME_dup() failed");
-				sk_X509_NAME_push(*xnames_r, xname);
-			}
-		}
-		if(itmp->crl)
-			X509_STORE_add_crl(store, itmp->crl);
+	struct stat statbuf;
+	int ret = 0;
+	stat(ca, &statbuf);
+	
+	if (S_ISDIR(statbuf.st_mode)) {
+		ret = SSL_CTX_load_verify_locations(ssl_ctx, NULL, ca);
+	} else {
+		ret = SSL_CTX_load_verify_locations(ssl_ctx, ca, NULL);
+	}
+	if (!ret) {
+		i_fatal("SSL_CTX_load_verify_locations() failed: %s",
ssl_last_error());
 	}
-	sk_X509_INFO_pop_free(inf, X509_INFO_free);
 }
 
-static STACK_OF(X509_NAME) *
-ssl_proxy_ctx_init(SSL_CTX *ssl_ctx, const struct master_service_ssl_settings
*set,
-		   bool load_xnames)
+static void
+ssl_proxy_ctx_init(SSL_CTX *ssl_ctx, const struct master_service_ssl_settings
*set)
 {
-	X509_STORE *store;
-	STACK_OF(X509_NAME) *xnames = NULL;
-
 	/* enable all SSL workarounds, except empty fragments as it
 	   makes SSL more vulnerable against attacks */
 	SSL_CTX_set_options(ssl_ctx, SSL_OP_ALL &
@@ -1010,12 +981,10 @@
 
 	if (*set->ssl_ca != '\0') {
 		/* set trusted CA certs */
-		store = SSL_CTX_get_cert_store(ssl_ctx);
-		load_ca(store, set->ssl_ca, load_xnames ? &xnames : NULL);
+		load_ca(ssl_ctx, set->ssl_ca);
 	}
 	ssl_proxy_ctx_set_crypto_params(ssl_ctx, set);
 	SSL_CTX_set_info_callback(ssl_ctx, ssl_info_callback);
-	return xnames;
 }
 
 static void
@@ -1068,7 +1037,7 @@
 }
 
 static void
-ssl_proxy_ctx_verify_client(SSL_CTX *ssl_ctx, STACK_OF(X509_NAME) *ca_names)
+ssl_proxy_ctx_verify_client(SSL_CTX *ssl_ctx)
 {
 #if OPENSSL_VERSION_NUMBER >= 0x00907000L
 	X509_STORE *store;
@@ -1079,8 +1048,6 @@
 #endif
 	SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER | SSL_VERIFY_CLIENT_ONCE,
 			   ssl_verify_client_cert);
-	/* set list of CA names that are sent to client */
-	SSL_CTX_set_client_CA_list(ssl_ctx, ca_names);
 }
 
 static const char *ssl_proxy_get_use_certificate_error(const char *cert)
@@ -1277,7 +1244,7 @@
 	ctx->ctx = ssl_ctx = SSL_CTX_new(SSLv23_server_method());
 	if (ssl_ctx == NULL)
 		i_fatal("SSL_CTX_new() failed");
-	xnames = ssl_proxy_ctx_init(ssl_ctx, ssl_set, ctx->verify_client_cert);
+	ssl_proxy_ctx_init(ssl_ctx, ssl_set);
 
 	if (SSL_CTX_set_cipher_list(ssl_ctx, ctx->cipher_list) != 1) {
 		i_fatal("Can't set cipher list to '%s': %s",
@@ -1303,7 +1270,7 @@
 	ssl_proxy_ctx_use_key(ctx->ctx, ssl_set);
 
 	if (ctx->verify_client_cert)
-		ssl_proxy_ctx_verify_client(ctx->ctx, xnames);
+		ssl_proxy_ctx_verify_client(ctx->ctx);
 
 	hash_table_insert(ssl_servers, ctx, ctx);
 	return ctx;
@@ -1343,12 +1310,10 @@
 ssl_proxy_init_client(const struct login_settings *login_set,
 		      const struct master_service_ssl_settings *ssl_set)
 {
-	STACK_OF(X509_NAME) *xnames;
-
 	if ((ssl_client_ctx = SSL_CTX_new(SSLv23_client_method())) == NULL)
 		i_fatal("SSL_CTX_new() failed");
-	xnames = ssl_proxy_ctx_init(ssl_client_ctx, ssl_set, TRUE);
-	ssl_proxy_ctx_verify_client(ssl_client_ctx, xnames);
+	ssl_proxy_ctx_init(ssl_client_ctx, ssl_set);
+	ssl_proxy_ctx_verify_client(ssl_client_ctx);
 
 	ssl_proxy_client_ctx_set_client_cert(ssl_client_ctx, login_set);
 }
Index: dovecot-2.2.9/src/lib-ssl-iostream/iostream-openssl-context.c
==================================================================---
dovecot-2.2.9.orig/src/lib-ssl-iostream/iostream-openssl-context.c	2015-02-11
00:31:24.986198000 -0500
+++ dovecot-2.2.9/src/lib-ssl-iostream/iostream-openssl-context.c	2015-02-11
00:31:24.986198000 -0500
@@ -11,6 +11,7 @@
 #include <openssl/ssl.h>
 #include <openssl/err.h>
 #include <openssl/rand.h>
+#include <sys/stat.h>
 
 #if !defined(OPENSSL_NO_ECDH) && OPENSSL_VERSION_NUMBER >=
0x10000000L
 #  define HAVE_ECDH
@@ -222,50 +223,26 @@
 	return ret;
 }
 
-static int load_ca(X509_STORE *store, const char *ca,
-		   STACK_OF(X509_NAME) **xnames_r)
+static int load_ca(SSL_CTX *ssl_ctx, const char *ca)
 {
-	/* mostly just copy&pasted from X509_load_cert_crl_file() */
-	STACK_OF(X509_INFO) *inf;
-	STACK_OF(X509_NAME) *xnames;
-	X509_INFO *itmp;
-	X509_NAME *xname;
-	BIO *bio;
-	int i;
-
-	bio = BIO_new_mem_buf(t_strdup_noconst(ca), strlen(ca));
-	if (bio == NULL)
-		i_fatal("BIO_new_mem_buf() failed");
-	inf = PEM_X509_INFO_read_bio(bio, NULL, NULL, NULL);
-	BIO_free(bio);
-
-	if (inf == NULL)
+	struct stat statbuf;
+	int ret = 0;
+	stat(ca, &statbuf);
+	if S_ISDIR(statbuf.st_mode) {
+		ret = SSL_CTX_load_verify_location(ssl_ctx, NULL, ca);
+	} else {
+		ret = SSL_CTX_load_verify_location(ssl_ctx, ca, NULL);
+	}	
+	if (!ret) {
 		return -1;
-
-	xnames = sk_X509_NAME_new_null();
-	if (xnames == NULL)
-		i_fatal("sk_X509_NAME_new_null() failed");
-	for(i = 0; i < sk_X509_INFO_num(inf); i++) {
-		itmp = sk_X509_INFO_value(inf, i);
-		if(itmp->x509) {
-			X509_STORE_add_cert(store, itmp->x509);
-			xname = X509_get_subject_name(itmp->x509);
-			if (xname != NULL)
-				xname = X509_NAME_dup(xname);
-			if (xname != NULL)
-				sk_X509_NAME_push(xnames, xname);
-		}
-		if(itmp->crl)
-			X509_STORE_add_crl(store, itmp->crl);
+	} else {
+		return 0;
 	}
-	sk_X509_INFO_pop_free(inf, X509_INFO_free);
-	*xnames_r = xnames;
-	return 0;
+
 }
 
 static void
-ssl_iostream_ctx_verify_remote_cert(struct ssl_iostream_context *ctx,
-				    STACK_OF(X509_NAME) *ca_names)
+ssl_iostream_ctx_verify_remote_cert(struct ssl_iostream_context *ctx)
 {
 #if OPENSSL_VERSION_NUMBER >= 0x00907000L
 	X509_STORE *store;
@@ -274,8 +251,6 @@
 	X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK |
 			     X509_V_FLAG_CRL_CHECK_ALL);
 #endif
-
-	SSL_CTX_set_client_CA_list(ctx->ssl_ctx, ca_names);
 }
 
 static struct ssl_iostream_settings *
@@ -320,18 +295,17 @@
 			     const char **error_r)
 {
 	X509_STORE *store;
-	STACK_OF(X509_NAME) *xnames = NULL;
 	const char *ca_file, *ca_dir;
 	bool have_ca = FALSE;
 
 	if (set->ca != NULL) {
 		store = SSL_CTX_get_cert_store(ctx->ssl_ctx);
-		if (load_ca(store, set->ca, &xnames) < 0) {
+		if (load_ca(ctx->ssl_ctx, set->ca) < 0) {
 			*error_r = t_strdup_printf("Couldn't parse ssl_ca: %s",
 						   openssl_iostream_error());
 			return -1;
 		}
-		ssl_iostream_ctx_verify_remote_cert(ctx, xnames);
+		ssl_iostream_ctx_verify_remote_cert(ctx);
 		have_ca = TRUE;
 	}
 	ca_file = set->ca_file == NULL || *set->ca_file == '\0' ?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://dovecot.org/pipermail/dovecot/attachments/20150211/ae10a357/attachment-0001.sig>

what problem with startssl.com? lots of people use them for a long
time with no problem

On 2/11/15, Daniel Dickinson <dovecot at daniel.thecshore.com>
wrote:
> Hi all,
>
> As I reported earlier (with a typo in the work [BUG]) client
> certification validation *does not* work even if you do everything
> exactly according to all documentation and attempts at helpful advice.
>
> I have seen this issue with both startssl.com and self-signed
> certificates, and based on what I've seen from searching the web, this
> is a problem that has gotten little attention because most people don't
> bother, but are more than willing to give out useless advice on how to
> make it work.
>
> Furthermore the issue does NOT occur with the cyrus-imap mail server, so
> it is definitely a server-side issue.
>
> The actual issue is that the code for calling OpenSSL that constructs
> the client certificate validation is in fact WRONG.
>
> I don't have a perfect patch as I was mostly interested in getting it
> working for my needs and didn't bother with constructing the list of CA
> names to send to the client, preferring to let OpenSSL handle all that
> sort of thing.
>
> What it comes down to is that the code, which probably worked at one
> point, was not correctly updated at some point and since then client
> side certificate validation has been BROKEN.
>
> I have patched against 2.2.9, however I have seen this problem in the
> versions in both Debian Wheezy and Debian Jessie as well.
>
> As you will see from the patch (which is an attachment as people tend to
> complain that patches get mangled when you inline them, and even if I
> have a good client I've gotten heck because the receiver didn't.
>
> Regards,
>
> Daniel
>
>

Ok, the patch doesn't actually fix the bug.  It appeared to do so in
that after running the server with the patch applied client certificate
validation succeeded, however, it appears this bug is actually
intermittent as, even with the patched package, the server is now
complaining that the client has not provided a valid SSL certificate.

This is definitely not true as the certificates, and in general
verification of the same client-side certificates work, even with the
same Thunderbird client, with postfix, exim, and cyrus-imapd.

In short dovecot has some bug that causes verification of certificates
presented by the client to fail, however the bug is not easy to debug as
sometimes config changes work, but later, running the same config,
things fail again.

There appears to be some sort of caching even across client and server
restarts that is coming into play and confusing the issue.

Anyone know of SSL caching issues with Window 8.1, particular
Thunderbird on that platform?

Also why is this bug only affecting dovecot?  There is some strangeness
going on here, and, from web searching for the same issue, it appears
others have run into the same issue and had no success in resolving it,
despite also doing everything according to documentation.

With dovecot 2.2.9 from Ubuntu (i.e. not patched version):
The relevant config bits from dovecot -n are:

auth_mechanisms = login plain digest-md5 cram-md5
auth_ssl_require_client_cert = yes
ssl = required
ssl_ca = </path/to/ca.pem
ssl_cert = </path/to/cert.pem
ssl_key = </path/to/key.pem
ssl_protocols = !SSLv2 !SSLv3
ssl_require_crl = no (yes or no makes no difference)
ssl_verify_client_cert = yes

Oddly this doesn't appear in dovecot -n, though set

disable_plaintext_auth = yes

Client fails whether configured for encrypted passwords or not
Client fails whether using ssl = yes (imaps) on 993 or STARTTLS on 143

And yes I have followed the correct ordering of the CA followed by CRL
(and tried without CRL as well).

In addition I have tried CA + intermediate and CA + crl + intermediate +
crl for startssl.com certificates as well as the above test with
self-signed CA (root CA + crl or just root CA).

I have also just confirmed (by connecting and sending mail on port 587
with STARTTLS and SSL required, with client certificate validation
required by postfix) that using the same certificate, same client, same
mail server, that SMTP AUTH + verification of client certificates succeeds.

Since I've had the same client using the same certificates work with all
three of postfix (SMTP), exim (SMTP), and cyrus-imapd (IMAP), there is
not doubt that there is an issue with dovecot's handling of this scenario.

I know I'm harping on this 'it works elsewhere' theme, but this bug
has
been ignored for ages because of the assumption that user is doing
something wrong, and that is simply not the case, or if it is, dovecot
is very, very bad at indicating what the actual problem is.

Line-ending are *nix line endings.

Anything else that you want to suggest along with how to make dovecot
report what the actual problem is if there is some other magic formulae
to invoke?

If I've missed something it's by no means obvious or explained clearly
anywhere.

Regards,

Daniel

On 2015-02-11 2:33 AM, Daniel Dickinson wrote:
> Hi all,
> 
> As I reported earlier (with a typo in the work [BUG]) client
> certification validation *does not* work even if you do everything
> exactly according to all documentation and attempts at helpful advice.
> 
> I have seen this issue with both startssl.com and self-signed
> certificates, and based on what I've seen from searching the web, this
> is a problem that has gotten little attention because most people don't
> bother, but are more than willing to give out useless advice on how to
> make it work.
> 
> Furthermore the issue does NOT occur with the cyrus-imap mail server, so
> it is definitely a server-side issue.
> 
> The actual issue is that the code for calling OpenSSL that constructs
> the client certificate validation is in fact WRONG.
> 
> I don't have a perfect patch as I was mostly interested in getting it
> working for my needs and didn't bother with constructing the list of CA
> names to send to the client, preferring to let OpenSSL handle all that
> sort of thing.
> 
> What it comes down to is that the code, which probably worked at one
> point, was not correctly updated at some point and since then client
> side certificate validation has been BROKEN.
> 
> I have patched against 2.2.9, however I have seen this problem in the
> versions in both Debian Wheezy and Debian Jessie as well.
> 
> As you will see from the patch (which is an attachment as people tend to
> complain that patches get mangled when you inline them, and even if I
> have a good client I've gotten heck because the receiver didn't.
> 
> Regards,
> 
> Daniel
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://dovecot.org/pipermail/dovecot/attachments/20150212/7a8ce77d/attachment-0001.sig>