[PATCH] Fix for client certificate validation does not work
Daniel Dickinson
dovecot at daniel.thecshore.com
Wed Feb 11 07:33:26 UTC 2015
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>
More information about the dovecot
mailing list