diff --git a/include/haproxy/ssl_ocsp-t.h b/include/haproxy/ssl_ocsp-t.h index 028d6fa09..34e55d48b 100644 --- a/include/haproxy/ssl_ocsp-t.h +++ b/include/haproxy/ssl_ocsp-t.h @@ -47,7 +47,8 @@ struct certificate_ocsp { struct ebmb_node key; unsigned char key_data[OCSP_MAX_CERTID_ASN1_LENGTH]; unsigned int key_length; - int refcount; + int refcount_store; /* Number of ckch_store that reference this certificate_ocsp */ + int refcount; /* Number of actual references to this certificate_ocsp (SSL_CTXs mostly) */ struct buffer response; long expire; X509 *issuer; diff --git a/include/haproxy/ssl_ocsp.h b/include/haproxy/ssl_ocsp.h index 54a1b8831..8a4197cf3 100644 --- a/include/haproxy/ssl_ocsp.h +++ b/include/haproxy/ssl_ocsp.h @@ -36,6 +36,7 @@ int ssl_sock_get_ocsp_arg_kt_index(int evp_keytype); int ssl_sock_ocsp_stapling_cbk(SSL *ssl, void *arg); void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp); +void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp); int ssl_sock_load_ocsp_response(struct buffer *ocsp_response, struct certificate_ocsp *ocsp, diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c index e7ada0252..299ca925b 100644 --- a/src/ssl_ckch.c +++ b/src/ssl_ckch.c @@ -734,8 +734,27 @@ void ssl_sock_free_cert_key_and_chain_contents(struct ckch_data *data) X509_free(data->ocsp_issuer); data->ocsp_issuer = NULL; - OCSP_CERTID_free(data->ocsp_cid); - data->ocsp_cid = NULL; + + /* We need to properly remove the reference to the corresponding + * certificate_ocsp structure if it exists (which it should). + */ +#if ((defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP) && !defined OPENSSL_IS_BORINGSSL) + if (data->ocsp_cid) { + struct certificate_ocsp *ocsp = NULL; + unsigned char certid[OCSP_MAX_CERTID_ASN1_LENGTH] = {}; + unsigned int certid_length = 0; + + if (ssl_ocsp_build_response_key(data->ocsp_cid, (unsigned char*)certid, &certid_length) >= 0) { + HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); + ocsp = (struct certificate_ocsp *)ebmb_lookup(&cert_ocsp_tree, certid, OCSP_MAX_CERTID_ASN1_LENGTH); + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); + ssl_sock_free_ocsp(ocsp); + } + + OCSP_CERTID_free(data->ocsp_cid); + data->ocsp_cid = NULL; + } +#endif } /* diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c index f3e2812a2..61375d5dd 100644 --- a/src/ssl_ocsp.c +++ b/src/ssl_ocsp.c @@ -382,11 +382,50 @@ int ssl_sock_update_ocsp_response(struct buffer *ocsp_response, char **err) #if !defined OPENSSL_IS_BORINGSSL +/* + * Must be called under ocsp_tree_lock lock. + */ +static void ssl_sock_free_ocsp_data(struct certificate_ocsp *ocsp) +{ + ebmb_delete(&ocsp->key); + eb64_delete(&ocsp->next_update); + X509_free(ocsp->issuer); + ocsp->issuer = NULL; + sk_X509_pop_free(ocsp->chain, X509_free); + ocsp->chain = NULL; + chunk_destroy(&ocsp->response); + if (ocsp->uri) { + ha_free(&ocsp->uri->area); + ha_free(&ocsp->uri); + } + free(ocsp); +} + /* * Decrease the refcount of the struct ocsp_response and frees it if it's not * used anymore. Also removes it from the tree if free'd. */ void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp) +{ + if (!ocsp) + return; + + HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); + ocsp->refcount_store--; + if (ocsp->refcount_store <= 0) { + eb64_delete(&ocsp->next_update); + /* Might happen if some ongoing requests kept using an SSL_CTX + * that referenced this OCSP response after the corresponding + * ckch_store was deleted or changed (via cli commands for + * instance). + */ + if (ocsp->refcount <= 0) + ssl_sock_free_ocsp_data(ocsp); + } + HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); +} + +void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp) { if (!ocsp) return; @@ -394,19 +433,15 @@ void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp) HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); ocsp->refcount--; if (ocsp->refcount <= 0) { - ebmb_delete(&ocsp->key); eb64_delete(&ocsp->next_update); - X509_free(ocsp->issuer); - ocsp->issuer = NULL; - sk_X509_pop_free(ocsp->chain, X509_free); - ocsp->chain = NULL; - chunk_destroy(&ocsp->response); - if (ocsp->uri) { - ha_free(&ocsp->uri->area); - ha_free(&ocsp->uri); - } + /* Might happen if some ongoing requests kept using an SSL_CTX + * that referenced this OCSP response after the corresponding + * ckch_store was deleted or changed (via cli commands for + * instance). + */ + if (ocsp->refcount_store <= 0) + ssl_sock_free_ocsp_data(ocsp); - free(ocsp); } HA_SPIN_UNLOCK(OCSP_LOCK, &ocsp_tree_lock); } @@ -626,13 +661,13 @@ void ssl_sock_ocsp_free_func(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int id ocsp_arg = ptr; if (ocsp_arg->is_single) { - ssl_sock_free_ocsp(ocsp_arg->s_ocsp); + ssl_sock_free_ocsp_instance(ocsp_arg->s_ocsp); ocsp_arg->s_ocsp = NULL; } else { int i; for (i = 0; i < SSL_SOCK_NUM_KEYTYPES; i++) { - ssl_sock_free_ocsp(ocsp_arg->m_ocsp[i]); + ssl_sock_free_ocsp_instance(ocsp_arg->m_ocsp[i]); ocsp_arg->m_ocsp[i] = NULL; } } @@ -908,7 +943,7 @@ static int ssl_ocsp_task_schedule() } REGISTER_POST_CHECK(ssl_ocsp_task_schedule); -void ssl_sock_free_ocsp(struct certificate_ocsp *ocsp); +void ssl_sock_free_ocsp_instance(struct certificate_ocsp *ocsp); void ssl_destroy_ocsp_update_task(void) { @@ -930,7 +965,7 @@ void ssl_destroy_ocsp_update_task(void) task_destroy(ocsp_update_task); ocsp_update_task = NULL; - ssl_sock_free_ocsp(ssl_ocsp_task_ctx.cur_ocsp); + ssl_sock_free_ocsp_instance(ssl_ocsp_task_ctx.cur_ocsp); ssl_ocsp_task_ctx.cur_ocsp = NULL; if (ssl_ocsp_task_ctx.hc) { @@ -1189,7 +1224,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context, /* Reinsert the entry into the update list so that it can be updated later */ ssl_ocsp_update_insert(ocsp); /* Release the reference kept on the updated ocsp response. */ - ssl_sock_free_ocsp(ctx->cur_ocsp); + ssl_sock_free_ocsp_instance(ctx->cur_ocsp); ctx->cur_ocsp = NULL; HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); @@ -1299,7 +1334,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context, ++ctx->cur_ocsp->num_failure; ssl_ocsp_update_insert_after_error(ctx->cur_ocsp); /* Release the reference kept on the updated ocsp response. */ - ssl_sock_free_ocsp(ctx->cur_ocsp); + ssl_sock_free_ocsp_instance(ctx->cur_ocsp); ctx->cur_ocsp = NULL; } if (hc) @@ -1328,7 +1363,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context, if (hc) httpclient_stop_and_destroy(hc); /* Release the reference kept on the updated ocsp response. */ - ssl_sock_free_ocsp(ctx->cur_ocsp); + ssl_sock_free_ocsp_instance(ctx->cur_ocsp); HA_SPIN_LOCK(OCSP_LOCK, &ocsp_tree_lock); /* Set next_wakeup to the new first entry of the tree */ eb = eb64_first(&ocsp_update_tree); @@ -1416,7 +1451,15 @@ static int cli_parse_update_ocsp_response(char **args, char *payload, struct app update_once = (ocsp->next_update.node.leaf_p == NULL); eb64_delete(&ocsp->next_update); - /* Insert the entry at the beginning of the update tree. */ + /* Insert the entry at the beginning of the update tree. + * We don't need to increase the reference counter on the + * certificate_ocsp structure because we would not have a way to + * decrease it afterwards since this update operation is asynchronous. + * If the corresponding entry were to be destroyed before the update can + * be performed, which is pretty unlikely, it would not be such a + * problem because that would mean that the OCSP response is not + * actually used. + */ ocsp->next_update.key = 0; eb64_insert(&ocsp_update_tree, &ocsp->next_update); ocsp->update_once = update_once; @@ -1655,6 +1698,14 @@ static int cli_io_handler_show_ocspresponse(struct appctx *appctx) #endif } +static void cli_release_show_ocspresponse(struct appctx *appctx) +{ + struct show_ocspresp_cli_ctx *ctx = appctx->svcctx; + + if (ctx) + ssl_sock_free_ocsp_instance(ctx->ocsp); +} + /* Check if the ckch_store and the entry does have the same configuration */ int ocsp_update_check_cfg_consistency(struct ckch_store *store, struct crtlist_entry *entry, char *crt_path, char **err) { @@ -1915,7 +1966,7 @@ smp_fetch_ssl_ocsp_success_cnt(const struct arg *args, struct sample *smp, const static struct cli_kw_list cli_kws = {{ },{ { { "set", "ssl", "ocsp-response", NULL }, "set ssl ocsp-response : update a certificate's OCSP Response from a base64-encode DER", cli_parse_set_ocspresponse, NULL }, - { { "show", "ssl", "ocsp-response", NULL },"show ssl ocsp-response [[text|base64] id] : display the IDs of the OCSP responses used in memory, or the details of a single OCSP response (in text or base64 format)", cli_parse_show_ocspresponse, cli_io_handler_show_ocspresponse, NULL }, + { { "show", "ssl", "ocsp-response", NULL },"show ssl ocsp-response [[text|base64] id] : display the IDs of the OCSP responses used in memory, or the details of a single OCSP response (in text or base64 format)", cli_parse_show_ocspresponse, cli_io_handler_show_ocspresponse, cli_release_show_ocspresponse }, { { "show", "ssl", "ocsp-updates", NULL }, "show ssl ocsp-updates : display information about the next 'nb' ocsp responses that will be updated automatically", cli_parse_show_ocsp_updates, cli_io_handler_show_ocsp_updates, cli_release_show_ocsp_updates }, #if ((defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP) && !defined OPENSSL_IS_BORINGSSL) { { "update", "ssl", "ocsp-response", NULL }, "update ssl ocsp-response : send ocsp request and update stored ocsp response", cli_parse_update_ocsp_response, NULL, NULL }, diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 85998ae4e..f1ee08365 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -1124,6 +1124,7 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data * struct buffer *ocsp_uri = get_trash_chunk(); char *err = NULL; size_t path_len; + int inc_refcount_store = 0; x = data->cert; if (!x) @@ -1159,8 +1160,10 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data * if (!issuer) goto out; - if (!data->ocsp_cid) + if (!data->ocsp_cid) { data->ocsp_cid = OCSP_cert_to_id(0, x, issuer); + inc_refcount_store = 1; + } if (!data->ocsp_cid) goto out; @@ -1187,6 +1190,9 @@ static int ssl_sock_load_ocsp(const char *path, SSL_CTX *ctx, struct ckch_data * #endif SSL_CTX_get_tlsext_status_cb(ctx, &callback); + if (inc_refcount_store) + iocsp->refcount_store++; + if (!callback) { struct ocsp_cbk_arg *cb_arg; EVP_PKEY *pkey;