From 17b4aa1adc88987f411ae007b7865c59cdf37c1b Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 17 Jul 2018 10:05:32 +0200 Subject: [PATCH] BUG/MINOR: ssl: properly ref-count the tls_keys entries Commit 200b0fa ("MEDIUM: Add support for updating TLS ticket keys via socket") introduced support for updating TLS ticket keys from the CLI, but missed a small corner case : if multiple bind lines reference the same tls_keys file, the same reference is used (as expected), but during the clean shutdown, it will lead to a double free when destroying the bind_conf contexts since none of the lines knows if others still use it. The impact is very low however, mostly a core and/or a message in the system's log upon old process termination. Let's introduce some basic refcounting to prevent this from happening, so that only the last bind_conf frees it. Thanks to Janusz Dziemidowicz and Thierry Fournier for both reporting the same issue with an easy reproducer. This fix needs to be backported from 1.6 to 1.8. --- include/types/ssl_sock.h | 1 + src/ssl_sock.c | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/include/types/ssl_sock.h b/include/types/ssl_sock.h index 8a5b3eeb8..2e02631c5 100644 --- a/include/types/ssl_sock.h +++ b/include/types/ssl_sock.h @@ -59,6 +59,7 @@ struct tls_keys_ref { struct list list; /* Used to chain refs. */ char *filename; int unique_id; /* Each pattern reference have unique id. */ + int refcount; /* number of users of this tls_keys_ref. */ struct tls_sess_key *tlskeys; int tls_ticket_enc_index; __decl_hathreads(HA_RWLOCK_T lock); /* lock used to protect the ref */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index b5547cc9e..3f70b1223 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -4823,7 +4823,7 @@ void ssl_sock_destroy_bind_conf(struct bind_conf *bind_conf) ssl_sock_free_ssl_conf(&bind_conf->ssl_conf); free(bind_conf->ca_sign_file); free(bind_conf->ca_sign_pass); - if (bind_conf->keys_ref) { + if (bind_conf->keys_ref && !--bind_conf->keys_ref->refcount) { free(bind_conf->keys_ref->filename); free(bind_conf->keys_ref->tlskeys); LIST_DEL(&bind_conf->keys_ref->list); @@ -7672,7 +7672,8 @@ static int bind_parse_tls_ticket_keys(char **args, int cur_arg, struct proxy *px } keys_ref = tlskeys_ref_lookup(args[cur_arg + 1]); - if(keys_ref) { + if (keys_ref) { + keys_ref->refcount++; conf->keys_ref = keys_ref; return 0; } @@ -7719,6 +7720,7 @@ static int bind_parse_tls_ticket_keys(char **args, int cur_arg, struct proxy *px i -= 2; keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i % TLS_TICKETS_NO; keys_ref->unique_id = -1; + keys_ref->refcount = 1; HA_RWLOCK_INIT(&keys_ref->lock); conf->keys_ref = keys_ref;