Finally, I got it ! It works with luck because we have 1 bug in Haproxy and 1 error (I suppose) in a OpenSSL compatibility layer.
First point is some OpenSSL macro, in the ssl.h file: #define SSL_set_app_data(s,arg) (SSL_set_ex_data(s,0,(char*)arg)) #define SSL_get_app_data(s) (SSL_get_ex_data(s,0)) These macro uses the function SSL_get_ex_data ans SSL_set_ex_data with the index 0, but this index is not reserved with the function SSL_get_ex_new_index() When I reserve a slot for the cipherlist hash, I have the number 0, this produce a collision an the set_app_data pointer is crushed by the cipherlist data. Luckily, the cipherlist hash is declared in the SSL_CTX space in place of SSL space. The first consequence was a memory leak because the allocated struct is never cleared. The second consequence is an index number > 0. - With this index number > 0, the data doesn't crush app_data, but there stored in a non-reserved storage => various crash cause - When I fix the reservation, The app_data are crushed, and we have systematic segfault. I join two patch. The first which fix the cipher capture must be backported to 1.8, for the second patch wich fix the app data compatibility, I dont known (at least 1.8). I join also the backports for 1.8 (there are trivial backport, with ery minor conflict) Thierry On Sun, 17 Jun 2018 03:01:54 +0200 Thierry FOURNIER <[email protected]> wrote: > Hi, > > When I use SSL requests and the cipherlist hash enabled, HAProxy > randomly crash: > > - segfault > - double free > - munmap_chunk(): invalid pointer > > I think that is a memory crush. > > I read the "cipherlist hash" code, and I put some printf, I do not > detect any memory override. > > When I comment the following line, the bug disappear > > SSL_set_ex_data(ssl, ssl_capture_ptr_index, capture); > > The crash happens with many versions of openssl: > > - 1.0.2j (home build) > - 1.0.1t-1+deb7u4 > - 1.0.1t-1+deb8u8 > - 1.0.2g-1ubuntu4.12 > > cipherlist hash is available from 1.8. The bug appears with current 1.8 > and current 1.9dev. > > I join some files: > > - bug36.build.sh : build script > - bug36.run.sh : run haproxy command > - bug36.request.sh : curl request > - bug36.conf : minimal conf which reproduce the problem > - bug36.pem : ramdom self signed certificate > > Just execute some requests, and the bug is reproduced. > > BR, > Thierry
>From a99fcb5af1a079ab5403f4cf7d2cf9345e4daf03 Mon Sep 17 00:00:00 2001 From: Thierry FOURNIER <[email protected]> Date: Sun, 17 Jun 2018 21:33:01 +0200 Subject: [PATCH 1/2] BUG/MAJOR: Random crash with cipherlist capture The cipher list capture struct is stored in the SSL memory space, but the slot is reserved in the SSL_CTX memory space. This causes ramdom crashes. This patch should be backported to 1.8 --- src/ssl_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 9fb2bb151..599c8c3ec 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -8805,7 +8805,7 @@ static void __ssl_sock_init(void) #if (OPENSSL_VERSION_NUMBER >= 0x1000200fL && !defined OPENSSL_NO_TLSEXT && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER) sctl_ex_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_sctl_free_func); #endif - ssl_capture_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); + ssl_capture_ptr_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); sample_register_fetches(&sample_fetch_keywords); acl_register_keywords(&acl_kws); bind_register_keywords(&bind_kws); -- 2.16.3
>From 54ce55a1203e732d266e72332bfea19d30f45487 Mon Sep 17 00:00:00 2001 From: Thierry FOURNIER <[email protected]> Date: Sun, 17 Jun 2018 21:37:05 +0200 Subject: [PATCH 2/2] BUG/MAJOR: OpenSSL context is stored in non-reserved memory slot We never saw unexplicated crash with SSL, so I suppose that we are luck, or the slot 0 is always reserved. Anyway the usage of the macro SSL_get_app_data() and SSL_set_app_data() seem wrong. This patch change the deprecated functions SSL_get_app_data() and SSL_set_app_data() by the new functions SSL_get_ex_data() and SSL_set_ex_data(), and it reserves the slot in the SSL memory space. For information, this is the two declaration which seems wrong or incomplete in the OpenSSL ssl.h file. We can see the usage of the slot 0 whoch is hardcoded, but never reserved. #define SSL_set_app_data(s,arg) (SSL_set_ex_data(s,0,(char *)arg)) #define SSL_get_app_data(s) (SSL_get_ex_data(s,0)) This patch must be backported at least in 1.8, maybe in other versions. --- src/ssl_sock.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 599c8c3ec..2707a40a9 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -260,6 +260,7 @@ struct ssl_capture { }; struct pool_head *pool_head_ssl_capture = NULL; static int ssl_capture_ptr_index = -1; +static int ssl_app_data_index = -1; #if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0) struct list tlskeys_reference = LIST_HEAD_INIT(tlskeys_reference); @@ -825,7 +826,7 @@ static int ssl_tlsext_ticket_key_cb(SSL *s, unsigned char key_name[16], unsigned int i; int ret = -1; /* error by default */ - conn = SSL_get_app_data(s); + conn = SSL_get_ex_data(s, ssl_app_data_index); ref = objt_listener(conn->target)->bind_conf->keys_ref; HA_RWLOCK_RDLOCK(TLSKEYS_REF_LOCK, &ref->lock); @@ -1389,7 +1390,7 @@ out: void ssl_sock_infocbk(const SSL *ssl, int where, int ret) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); BIO *write_bio; (void)ret; /* shut gcc stupid warning */ @@ -1427,7 +1428,7 @@ int ssl_sock_bind_verifycbk(int ok, X509_STORE_CTX *x_store) int err, depth; ssl = X509_STORE_CTX_get_ex_data(x_store, SSL_get_ex_data_X509_STORE_CTX_idx()); - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); conn->xprt_st |= SSL_SOCK_ST_FL_VERIFY_DONE; @@ -1575,7 +1576,7 @@ void ssl_sock_msgcbk(int write_p, int version, int content_type, const void *buf /* test heartbeat received (write_p is set to 0 for a received record) */ if ((content_type == TLS1_RT_HEARTBEAT) && (write_p == 0)) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); const unsigned char *p = buf; unsigned int payload; @@ -1903,7 +1904,7 @@ static int ssl_sock_generate_certificate_from_conn(struct bind_conf *bind_conf, SSL *ssl) { unsigned int key; - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); conn_get_to_addr(conn); if (conn->flags & CO_FL_ADDR_TO_SET) { @@ -2099,7 +2100,7 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *arg) int allow_early = 0; int i; - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); s = objt_listener(conn->target)->bind_conf; if (s->ssl_conf.early_data) @@ -3876,7 +3877,7 @@ static int sh_ssl_sess_store(unsigned char *s_id, unsigned char *data, int data_ /* SSL callback used when a new session is created while connecting to a server */ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); struct server *s; s = objt_server(conn->target); @@ -4373,7 +4374,7 @@ static int ssl_sock_srv_verifycbk(int ok, X509_STORE_CTX *ctx) return ok; ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); /* We're checking if the provided hostnames match the desired one. The * desired hostname comes from the SNI we presented if any, or if not @@ -4948,7 +4949,7 @@ static int ssl_sock_init(struct connection *conn) } /* set connection pointer */ - if (!SSL_set_app_data(conn->xprt_ctx, conn)) { + if (!SSL_set_ex_data(conn->xprt_ctx, ssl_app_data_index, conn)) { SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; if (may_retry--) { @@ -5007,7 +5008,7 @@ static int ssl_sock_init(struct connection *conn) } /* set connection pointer */ - if (!SSL_set_app_data(conn->xprt_ctx, conn)) { + if (!SSL_set_ex_data(conn->xprt_ctx, ssl_app_data_index, conn)) { SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; if (may_retry--) { @@ -8805,6 +8806,7 @@ static void __ssl_sock_init(void) #if (OPENSSL_VERSION_NUMBER >= 0x1000200fL && !defined OPENSSL_NO_TLSEXT && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER) sctl_ex_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_sctl_free_func); #endif + ssl_app_data_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL); ssl_capture_ptr_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); sample_register_fetches(&sample_fetch_keywords); acl_register_keywords(&acl_kws); -- 2.16.3
>From 73d7e2686d6a09f7f8ddb763dfe96124baa2113a Mon Sep 17 00:00:00 2001 From: Thierry FOURNIER <[email protected]> Date: Sun, 17 Jun 2018 21:33:01 +0200 Subject: [PATCH 1/2] BUG/MAJOR: Random crash with cipherlist capture The cipher list capture struct is stored in the SSL memory space, but the slot is reserved in the SSL_CTX memory space. This causes ramdom crashes. This patch should be backported to 1.8 --- src/ssl_sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 5a003dc67..e48bbec58 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -8964,7 +8964,7 @@ static void __ssl_sock_init(void) #if (OPENSSL_VERSION_NUMBER >= 0x1000200fL && !defined OPENSSL_NO_TLSEXT && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER) sctl_ex_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_sctl_free_func); #endif - ssl_capture_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); + ssl_capture_ptr_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); ssl_pkey_info_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, NULL); sample_register_fetches(&sample_fetch_keywords); acl_register_keywords(&acl_kws); -- 2.16.3
>From af2b771f66d0bf815a816d8812a6eb114d9afeb4 Mon Sep 17 00:00:00 2001 From: Thierry FOURNIER <[email protected]> Date: Sun, 17 Jun 2018 21:37:05 +0200 Subject: [PATCH 2/2] BUG/MAJOR: OpenSSL context is stored in non-reserved memory slot We never saw unexplicated crash with SSL, so I suppose that we are luck, or the slot 0 is always reserved. Anyway the usage of the macro SSL_get_app_data() and SSL_set_app_data() seem wrong. This patch change the deprecated functions SSL_get_app_data() and SSL_set_app_data() by the new functions SSL_get_ex_data() and SSL_set_ex_data(), and it reserves the slot in the SSL memory space. For information, this is the two declaration which seems wrong or incomplete in the OpenSSL ssl.h file. We can see the usage of the slot 0 whoch is hardcoded, but never reserved. #define SSL_set_app_data(s,arg) (SSL_set_ex_data(s,0,(char *)arg)) #define SSL_get_app_data(s) (SSL_get_ex_data(s,0)) This patch must be backported at least in 1.8, maybe in other versions. --- src/ssl_sock.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index e48bbec58..fb12ca87e 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -260,6 +260,7 @@ struct ssl_capture { }; struct pool_head *pool_head_ssl_capture = NULL; static int ssl_capture_ptr_index = -1; +static int ssl_app_data_index = -1; static int ssl_pkey_info_index = -1; @@ -824,7 +825,7 @@ static int ssl_tlsext_ticket_key_cb(SSL *s, unsigned char key_name[16], unsigned int i; int ret = -1; /* error by default */ - conn = SSL_get_app_data(s); + conn = SSL_get_ex_data(s, ssl_app_data_index); ref = objt_listener(conn->target)->bind_conf->keys_ref; HA_RWLOCK_RDLOCK(TLSKEYS_REF_LOCK, &ref->lock); @@ -1388,7 +1389,7 @@ out: void ssl_sock_infocbk(const SSL *ssl, int where, int ret) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); BIO *write_bio; (void)ret; /* shut gcc stupid warning */ @@ -1426,7 +1427,7 @@ int ssl_sock_bind_verifycbk(int ok, X509_STORE_CTX *x_store) int err, depth; ssl = X509_STORE_CTX_get_ex_data(x_store, SSL_get_ex_data_X509_STORE_CTX_idx()); - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); conn->xprt_st |= SSL_SOCK_ST_FL_VERIFY_DONE; @@ -1574,7 +1575,7 @@ void ssl_sock_msgcbk(int write_p, int version, int content_type, const void *buf /* test heartbeat received (write_p is set to 0 for a received record) */ if ((content_type == TLS1_RT_HEARTBEAT) && (write_p == 0)) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); const unsigned char *p = buf; unsigned int payload; @@ -1902,7 +1903,7 @@ static int ssl_sock_generate_certificate_from_conn(struct bind_conf *bind_conf, SSL *ssl) { unsigned int key; - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); conn_get_to_addr(conn); if (conn->flags & CO_FL_ADDR_TO_SET) { @@ -2106,7 +2107,7 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, void *arg) int allow_early = 0; int i; - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); s = objt_listener(conn->target)->bind_conf; if (s->ssl_conf.early_data) @@ -3890,7 +3891,7 @@ static int sh_ssl_sess_store(unsigned char *s_id, unsigned char *data, int data_ /* SSL callback used when a new session is created while connecting to a server */ static int ssl_sess_new_srv_cb(SSL *ssl, SSL_SESSION *sess) { - struct connection *conn = SSL_get_app_data(ssl); + struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index); struct server *s; s = objt_server(conn->target); @@ -4387,7 +4388,7 @@ static int ssl_sock_srv_verifycbk(int ok, X509_STORE_CTX *ctx) return ok; ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); - conn = SSL_get_app_data(ssl); + conn = SSL_get_ex_data(ssl, ssl_app_data_index); /* We're checking if the provided hostnames match the desired one. The * desired hostname comes from the SNI we presented if any, or if not @@ -4962,7 +4963,7 @@ static int ssl_sock_init(struct connection *conn) } /* set connection pointer */ - if (!SSL_set_app_data(conn->xprt_ctx, conn)) { + if (!SSL_set_ex_data(conn->xprt_ctx, ssl_app_data_index, conn)) { SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; if (may_retry--) { @@ -5021,7 +5022,7 @@ static int ssl_sock_init(struct connection *conn) } /* set connection pointer */ - if (!SSL_set_app_data(conn->xprt_ctx, conn)) { + if (!SSL_set_ex_data(conn->xprt_ctx, ssl_app_data_index, conn)) { SSL_free(conn->xprt_ctx); conn->xprt_ctx = NULL; if (may_retry--) { @@ -8964,6 +8965,7 @@ static void __ssl_sock_init(void) #if (OPENSSL_VERSION_NUMBER >= 0x1000200fL && !defined OPENSSL_NO_TLSEXT && !defined OPENSSL_IS_BORINGSSL && !defined LIBRESSL_VERSION_NUMBER) sctl_ex_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_sctl_free_func); #endif + ssl_app_data_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL); ssl_capture_ptr_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, ssl_sock_capture_free_func); ssl_pkey_info_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, NULL); sample_register_fetches(&sample_fetch_keywords); -- 2.16.3
