Hi list, I'm currently workibng on dynamic load of certificates, and I see somme minor bug. There are a few Patches.
The first of them add some useful traces for debuging SSL (typically explain the reason of an handshake failure). The second and the third are 2 minor bug with return code not controlled. The fourth is the main patch. It remove some entries tore twice in the SNI tree. This bug is really minor. I valid my patches with a classic single certificate, and with dual certificate (DSA/RSA). I can't test dual or three certificates with ECDSA. My patches can be applied on the current master branch, and there are easy to apply on the 1.6 branch. Anyone can test the mode DSA + RSA + ECDSA ? Thanks Thierry
>From e789c15a217d4bfc8ed7fac080cd17236c06c654 Mon Sep 17 00:00:00 2001 From: "Thierry FOURNIER / OZON.IO" <[email protected]> Date: Mon, 10 Oct 2016 11:59:50 +0200 Subject: [PATCH 1/4] MINOR: ssl: add debug traces X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Add some debug trace when haproxy is configured in debug & verbose mode. This is useful for openssl tests. Typically, the error "SSL handshake failure" can be caused by a lot of protocol error. This patch details the encountered error. For exemple: OpenSSL error 0x1408a0c1: ssl3_get_client_hello: no shared cipher Note that my compilator (gcc-4.7) refuse to considers the fucntion ssl_sock_dump_errors() as inline. The condition "if" ensure that the content of the function is not executed in normal case. It should be a pity to call a function just for testing its execution condition, so I use the macro "forceinline". --- src/ssl_sock.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 1dfafb3..67093a2 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -200,6 +200,27 @@ struct ocsp_cbk_arg { }; /* + * This function gives the detail of the SSL error. It is used only + * if the debug mode and the verbose mode are activated. It dump all + * the SSL error until the stack was empty. + */ +static forceinline void ssl_sock_dump_errors(struct connection *conn) +{ + unsigned long ret; + + if (unlikely(global.mode & MODE_DEBUG)) { + while(1) { + ret = ERR_get_error(); + if (ret == 0) + return; + fprintf(stderr, "fd[%04x] OpenSSL error[0x%lx] %s: %s\n", + (unsigned short)conn->t.sock.fd, ret, + ERR_func_error_string(ret), ERR_reason_error_string(ret)); + } + } +} + +/* * This function returns the number of seconds elapsed * since the Epoch, 1970-01-01 00:00:00 +0000 (UTC) and the * date presented un ASN1_GENERALIZEDTIME. @@ -1014,6 +1035,7 @@ int ssl_sock_bind_verifycbk(int ok, X509_STORE_CTX *x_store) } if (objt_listener(conn->target)->bind_conf->ca_ignerr & (1ULL << err)) { + ssl_sock_dump_errors(conn); ERR_clear_error(); return 1; } @@ -1027,6 +1049,7 @@ int ssl_sock_bind_verifycbk(int ok, X509_STORE_CTX *x_store) /* check if certificate error needs to be ignored */ if (objt_listener(conn->target)->bind_conf->crt_ignerr & (1ULL << err)) { + ssl_sock_dump_errors(conn); ERR_clear_error(); return 1; } @@ -3543,6 +3566,7 @@ reneg_ok: out_error: /* Clear openssl global errors stack */ + ssl_sock_dump_errors(conn); ERR_clear_error(); /* free resumed session if exists */ @@ -3620,6 +3644,7 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun conn->flags |= CO_FL_ERROR; /* Clear openssl global errors stack */ + ssl_sock_dump_errors(conn); ERR_clear_error(); } goto read0; @@ -3654,6 +3679,7 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun return done; out_error: /* Clear openssl global errors stack */ + ssl_sock_dump_errors(conn); ERR_clear_error(); conn->flags |= CO_FL_ERROR; @@ -3750,6 +3776,7 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl out_error: /* Clear openssl global errors stack */ + ssl_sock_dump_errors(conn); ERR_clear_error(); conn->flags |= CO_FL_ERROR; @@ -3775,6 +3802,7 @@ static void ssl_sock_shutw(struct connection *conn, int clean) /* no handshake was in progress, try a clean ssl shutdown */ if (clean && (SSL_shutdown(conn->xprt_ctx) <= 0)) { /* Clear openssl global errors stack */ + ssl_sock_dump_errors(conn); ERR_clear_error(); } @@ -6022,6 +6050,9 @@ static void __ssl_sock_init(void) #ifndef OPENSSL_NO_DH ssl_dh_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, NULL); #endif + + /* Load SSL string for the verbose & debug mode. */ + ERR_load_SSL_strings(); } __attribute__((destructor)) -- 1.7.10.4
>From 7ef42f1897facc87a82113a7b5cda7a8ea82e69d Mon Sep 17 00:00:00 2001 From: "Thierry FOURNIER / OZON.IO" <[email protected]> Date: Fri, 14 Oct 2016 00:49:21 +0200 Subject: [PATCH 2/4] BUILD/CLEANUP: ssl: Check BIO_reset() return code X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 The BIO_reset function can fails, and the error is not processed. This patch just take in account the return code of the BIO_reset() function. --- src/ssl_sock.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 67093a2..bec8b28 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1831,7 +1831,11 @@ static int ssl_sock_load_crt_file_into_ckch(const char *path, struct cert_key_an } /* Seek back to beginning of file */ - BIO_reset(in); + if (BIO_reset(in) == -1) { + memprintf(err, "%san error occurs while reading the file '%s'.\n", + err && *err ? *err : "", path); + goto end; + } /* Read Certificate */ ckch->cert = PEM_read_bio_X509_AUX(in, NULL, NULL, NULL); -- 1.7.10.4
>From 6806f2e93b73b8bd35b5f019f36d86f5c5acf1c1 Mon Sep 17 00:00:00 2001 From: "Thierry FOURNIER / OZON.IO" <[email protected]> Date: Thu, 6 Oct 2016 10:35:29 +0200 Subject: [PATCH 3/4] BUG/MINOR: ssl: Check malloc return code X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 If malloc() can't allocate memory and return NULL, a segfaut will raises. This patch should be backported in the 1.6 and 1.5 version. --- src/ssl_sock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index bec8b28..7dc2e59 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1704,6 +1704,8 @@ static int ssl_sock_add_cert_sni(SSL_CTX *ctx, struct bind_conf *s, char *name, int j, len; len = strlen(name); sc = malloc(sizeof(struct sni_ctx) + len + 1); + if (!sc) + return order; for (j = 0; j < len; j++) sc->name.key[j] = tolower(name[j]); sc->name.key[len] = 0; -- 1.7.10.4
>From 43fb75ed3f7badb4d93abcbf4e1aa90638f39773 Mon Sep 17 00:00:00 2001 From: "Thierry FOURNIER / OZON.IO" <[email protected]> Date: Thu, 6 Oct 2016 10:56:48 +0200 Subject: [PATCH 4/4] BUG/MINOR: ssl: prevent multiple entries for the same certificate X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Today, the certificate are indexed int he SNI tree using their CN and the list of thier AltNames. So, Some certificates have the same names in the CN and one of the AltNames entries. Typically Let's Encrypt duplicate the the DNS name in the CN and the AltName. This patch prevent the creation of identical entries in the trees. It checks the same DNS name and the same SSL context. If the same certificate is registered two time it will be duplicated. This patch should be backported in the 1.6 and 1.5 version. --- src/ssl_sock.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 7dc2e59..1d08355 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1688,6 +1688,7 @@ static int ssl_sock_add_cert_sni(SSL_CTX *ctx, struct bind_conf *s, char *name, { struct sni_ctx *sc; int wild = 0, neg = 0; + struct ebmb_node *node; if (*name == '!') { neg = 1; @@ -1703,12 +1704,27 @@ static int ssl_sock_add_cert_sni(SSL_CTX *ctx, struct bind_conf *s, char *name, if (*name) { int j, len; len = strlen(name); + for (j = 0; j < len && j < trash.size; j++) + trash.str[j] = tolower(name[j]); + if (j >= trash.size) + return order; + trash.str[j] = 0; + + /* Check for duplicates. */ + if (wild) + node = ebst_lookup(&s->sni_w_ctx, trash.str); + else + node = ebst_lookup(&s->sni_ctx, trash.str); + for (; node; node = ebmb_next_dup(node)) { + sc = ebmb_entry(node, struct sni_ctx, name); + if (sc->ctx == ctx && sc->neg == neg) + return order; + } + sc = malloc(sizeof(struct sni_ctx) + len + 1); if (!sc) return order; - for (j = 0; j < len; j++) - sc->name.key[j] = tolower(name[j]); - sc->name.key[len] = 0; + memcpy(sc->name.key, trash.str, len + 1); sc->ctx = ctx; sc->order = order++; sc->neg = neg; -- 1.7.10.4
