From 53e93339474c1ffee7fb282ed1d6183f890ee469 Mon Sep 17 00:00:00 2001 From: Jon Shallow Date: Wed, 10 Jul 2024 13:11:02 +0100 Subject: [PATCH] Locking: Make sure all app callbacks are properly locked. Recursive lock issues happen for some of the app callbacks that then call a libcoap public API that tries to lock up the CoAP context. All libcoap callbacks into app code should now be fixed. --- src/coap_block.c | 29 ++++++++++++-------- src/coap_cache.c | 2 +- src/coap_gnutls.c | 67 +++++++++++++++++++++++++-------------------- src/coap_mbedtls.c | 32 ++++++++++++---------- src/coap_openssl.c | 55 +++++++++++++++++++++++-------------- src/coap_resource.c | 5 ++-- src/coap_tinydtls.c | 16 +++++++---- src/coap_wolfssl.c | 42 ++++++++++++++++------------ 8 files changed, 145 insertions(+), 103 deletions(-) diff --git a/src/coap_block.c b/src/coap_block.c index bc0fc3e7cf..0e13ba146c 100644 --- a/src/coap_block.c +++ b/src/coap_block.c @@ -667,8 +667,9 @@ coap_add_data_large_internal(coap_session_t *session, assert(pdu); if (pdu->data) { coap_log_warn("coap_add_data_large: PDU already contains data\n"); - if (release_func) - release_func(session, app_ptr); + if (release_func) { + coap_lock_callback(session->context, release_func(session, app_ptr)); + } return 0; } @@ -865,8 +866,9 @@ coap_add_data_large_internal(coap_session_t *session, if (!coap_add_data(pdu, rem, &data[block.num * chunk])) goto fail; } - if (release_func) - release_func(session, app_ptr); + if (release_func) { + coap_lock_callback(session->context, release_func(session, app_ptr)); + } } else if ((have_block_defined && length > chunk) || (ssize_t)length > avail) { /* Only add in lg_xmit if more than one block needs to be handled */ size_t rem; @@ -1065,8 +1067,9 @@ coap_add_data_large_internal(coap_session_t *session, if (!coap_add_data(pdu, length, data)) goto fail; - if (release_func) - release_func(session, app_ptr); + if (release_func) { + coap_lock_callback(session->context, release_func(session, app_ptr)); + } } return 1; @@ -1074,7 +1077,7 @@ coap_add_data_large_internal(coap_session_t *session, if (lg_xmit) { coap_block_delete_lg_xmit(session, lg_xmit); } else if (release_func) { - release_func(session, app_ptr); + coap_lock_callback(session->context, release_func(session, app_ptr)); } return 0; } @@ -1109,8 +1112,9 @@ coap_add_data_large_request_lkd(coap_session_t *session, * E.g. Reliable and CSM not in yet for checking block support */ if (coap_client_delay_first(session) == 0) { - if (release_func) - release_func(session, app_ptr); + if (release_func) { + coap_lock_callback(session->context, release_func(session, app_ptr)); + } return 0; } return coap_add_data_large_internal(session, NULL, pdu, NULL, NULL, -1, 0, @@ -1249,8 +1253,9 @@ coap_add_data_large_response_lkd(coap_resource_t *resource, return 1; error: - if (release_func) - release_func(session, app_ptr); + if (release_func) { + coap_lock_callback(session->context, release_func(session, app_ptr)); + } error_released: #if COAP_ERROR_PHRASE_LENGTH > 0 coap_add_data(response, @@ -2334,7 +2339,7 @@ coap_block_delete_lg_xmit(coap_session_t *session, return; if (lg_xmit->release_func) { - lg_xmit->release_func(session, lg_xmit->app_ptr); + coap_lock_callback(session->context, lg_xmit->release_func(session, lg_xmit->app_ptr)); } if (lg_xmit->pdu.token) { coap_free_type(COAP_PDU_BUF, lg_xmit->pdu.token - lg_xmit->pdu.max_hdr_size); diff --git a/src/coap_cache.c b/src/coap_cache.c index 3bcb862a9a..0f5ceb1db9 100644 --- a/src/coap_cache.c +++ b/src/coap_cache.c @@ -292,7 +292,7 @@ coap_delete_cache_entry(coap_context_t *ctx, coap_cache_entry_t *cache_entry) { } coap_delete_cache_key(cache_entry->cache_key); if (cache_entry->callback && cache_entry->app_data) { - cache_entry->callback(cache_entry->app_data); + coap_lock_callback(ctx, cache_entry->callback(cache_entry->app_data)); } coap_free_type(COAP_CACHE_ENTRY, cache_entry); } diff --git a/src/coap_gnutls.c b/src/coap_gnutls.c index 17d21b724f..9053e68268 100644 --- a/src/coap_gnutls.c +++ b/src/coap_gnutls.c @@ -593,10 +593,10 @@ psk_client_callback(gnutls_session_t g_session, lhint.length = temp.length; lhint.s = temp.s; - cpsk_info = - setup_data->validate_ih_call_back(&lhint, - c_session, - setup_data->ih_call_back_arg); + coap_lock_callback_ret(cpsk_info, c_session->context, + setup_data->validate_ih_call_back(&lhint, + c_session, + setup_data->ih_call_back_arg)); if (cpsk_info == NULL) return -1; @@ -753,13 +753,15 @@ check_rpk_cert(coap_gnutls_context_t *g_context, G_CHECK(gnutls_pubkey_export(pcert.pubkey, GNUTLS_X509_FMT_DER, der, &size), "gnutls_pubkey_export"); gnutls_pcert_deinit(&pcert); - if (!g_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN, - der, - size, - c_session, - 0, - 1, - g_context->setup_data.cn_call_back_arg)) { + coap_lock_callback_ret(ret, c_session->context, + g_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN, + der, + size, + c_session, + 0, + 1, + g_context->setup_data.cn_call_back_arg)); + if (!ret) { return 0; } } @@ -910,13 +912,15 @@ cert_verify_gnutls(gnutls_session_t g_session) { G_CHECK(gnutls_x509_crt_export(cert, GNUTLS_X509_FMT_DER, der, &size), "gnutls_x509_crt_export"); gnutls_x509_crt_deinit(cert); - if (!g_context->setup_data.validate_cn_call_back(OUTPUT_CERT_NAME, - der, - size, - c_session, - 0, - cert_is_trusted, - g_context->setup_data.cn_call_back_arg)) { + coap_lock_callback_ret(ret, c_session->context, + g_context->setup_data.validate_cn_call_back(OUTPUT_CERT_NAME, + der, + size, + c_session, + 0, + cert_is_trusted, + g_context->setup_data.cn_call_back_arg)); + if (!ret) { alert = GNUTLS_A_ACCESS_DENIED; goto fail; } @@ -1528,10 +1532,12 @@ post_client_hello_gnutls_psk(gnutls_session_t g_session) { /* * New SNI request */ - const coap_dtls_spsk_info_t *new_entry = - c_session->context->spsk_setup_data.validate_sni_call_back(name, - c_session, - c_session->context->spsk_setup_data.sni_call_back_arg); + const coap_dtls_spsk_info_t *new_entry; + + coap_lock_callback_ret(new_entry, c_session->context, + c_session->context->spsk_setup_data.validate_sni_call_back(name, + c_session, + c_session->context->spsk_setup_data.sni_call_back_arg)); if (!new_entry) { G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL, GNUTLS_A_UNRECOGNIZED_NAME)); @@ -1642,9 +1648,11 @@ post_client_hello_gnutls_pki(gnutls_session_t g_session) { /* * New SNI request */ - coap_dtls_key_t *new_entry = - g_context->setup_data.validate_sni_call_back(name, - g_context->setup_data.sni_call_back_arg); + coap_dtls_key_t *new_entry; + + coap_lock_callback_ret(new_entry, c_session->context, + g_context->setup_data.validate_sni_call_back(name, + g_context->setup_data.sni_call_back_arg)); if (!new_entry) { G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL, GNUTLS_A_UNRECOGNIZED_NAME)); @@ -1660,11 +1668,10 @@ post_client_hello_gnutls_pki(gnutls_session_t g_session) { g_context->pki_sni_entry_list[i].pki_key = *new_entry; sni_setup_data = g_context->setup_data; sni_setup_data.pki_key = *new_entry; - if ((ret = setup_pki_credentials( - &g_context->pki_sni_entry_list[i].pki_credentials, - g_session, - g_context, - &sni_setup_data, COAP_DTLS_ROLE_SERVER)) < 0) { + if ((ret = setup_pki_credentials(&g_context->pki_sni_entry_list[i].pki_credentials, + g_session, + g_context, + &sni_setup_data, COAP_DTLS_ROLE_SERVER)) < 0) { int keep_ret = ret; G_ACTION(gnutls_alert_send(g_session, GNUTLS_AL_FATAL, GNUTLS_A_BAD_CERTIFICATE)); diff --git a/src/coap_mbedtls.c b/src/coap_mbedtls.c index 21dd035527..bdcf8645e8 100644 --- a/src/coap_mbedtls.c +++ b/src/coap_mbedtls.c @@ -474,13 +474,17 @@ cert_verify_callback_mbedtls(void *data, mbedtls_x509_crt *crt, *flags &= ~MBEDTLS_X509_BADCERT_CN_MISMATCH; } if (setup_data->validate_cn_call_back) { - if (!setup_data->validate_cn_call_back(cn, - crt->raw.p, - crt->raw.len, - c_session, - depth, - *flags == 0, - setup_data->cn_call_back_arg)) { + int ret; + + coap_lock_callback_ret(ret, c_session->context, + setup_data->validate_cn_call_back(cn, + crt->raw.p, + crt->raw.len, + c_session, + depth, + *flags == 0, + setup_data->cn_call_back_arg)); + if (!ret) { *flags |= MBEDTLS_X509_BADCERT_CN_MISMATCH; } } @@ -897,9 +901,9 @@ pki_sni_callback(void *p_info, mbedtls_ssl_context *ssl, coap_dtls_key_t *new_entry; pki_sni_entry *pki_sni_entry_list; - new_entry = - m_context->setup_data.validate_sni_call_back(name, - m_context->setup_data.sni_call_back_arg); + coap_lock_callback_ret(new_entry, c_session->context, + m_context->setup_data.validate_sni_call_back(name, + m_context->setup_data.sni_call_back_arg)); if (!new_entry) { mbedtls_free(name); return -1; @@ -975,10 +979,10 @@ psk_sni_callback(void *p_info, mbedtls_ssl_context *ssl, const coap_dtls_spsk_info_t *new_entry; psk_sni_entry *psk_sni_entry_list; - new_entry = - c_session->context->spsk_setup_data.validate_sni_call_back(name, - c_session, - c_session->context->spsk_setup_data.sni_call_back_arg); + coap_lock_callback_ret(new_entry, c_session->context, + c_session->context->spsk_setup_data.validate_sni_call_back(name, + c_session, + c_session->context->spsk_setup_data.sni_call_back_arg)); if (!new_entry) { mbedtls_free(name); return -1; diff --git a/src/coap_openssl.c b/src/coap_openssl.c index 23e5e1c639..95a3cb5c85 100644 --- a/src/coap_openssl.c +++ b/src/coap_openssl.c @@ -762,10 +762,10 @@ coap_dtls_psk_client_callback(SSL *ssl, lhint.s = temp.s; lhint.length = temp.length; - cpsk_info = - setup_data->validate_ih_call_back(&lhint, - c_session, - setup_data->ih_call_back_arg); + coap_lock_callback_ret(cpsk_info, c_session->context, + setup_data->validate_ih_call_back(&lhint, + c_session, + setup_data->ih_call_back_arg)); if (cpsk_info == NULL) return 0; @@ -2410,12 +2410,15 @@ tls_verify_call_back(int preverify_ok, X509_STORE_CTX *ctx) { int length = i2d_X509(x509, NULL); uint8_t *base_buf; uint8_t *base_buf2 = base_buf = OPENSSL_malloc(length); + int ret; /* base_buf2 gets moved to the end */ i2d_X509(x509, &base_buf2); - if (!setup_data->validate_cn_call_back(cn, base_buf, length, session, - depth, preverify_ok, - setup_data->cn_call_back_arg)) { + coap_lock_callback_ret(ret, session->context, + setup_data->validate_cn_call_back(cn, base_buf, length, session, + depth, preverify_ok, + setup_data->cn_call_back_arg)); + if (!ret) { if (depth == 0) { X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REJECTED); } else { @@ -2564,8 +2567,11 @@ tls_server_name_call_back(SSL *ssl, if (i == context->sni_count) { SSL_CTX *ctx; coap_dtls_pki_t sni_setup_data; - coap_dtls_key_t *new_entry = setup_data->validate_sni_call_back(sni, - setup_data->sni_call_back_arg); + coap_dtls_key_t *new_entry; + + coap_lock_callback_ret(new_entry, session->context, + setup_data->validate_sni_call_back(sni, + setup_data->sni_call_back_arg)); if (!new_entry) { return SSL_TLSEXT_ERR_ALERT_FATAL; } @@ -2661,10 +2667,12 @@ psk_tls_server_name_call_back(SSL *ssl, } if (i == o_context->psk_sni_count) { SSL_CTX *ctx; - const coap_dtls_spsk_info_t *new_entry = - setup_data->validate_sni_call_back(sni, - c_session, - setup_data->sni_call_back_arg); + const coap_dtls_spsk_info_t *new_entry; + + coap_lock_callback_ret(new_entry, c_session->context, + setup_data->validate_sni_call_back(sni, + c_session, + setup_data->sni_call_back_arg)); if (!new_entry) { return SSL_TLSEXT_ERR_ALERT_FATAL; } @@ -2880,8 +2888,11 @@ tls_client_hello_call_back(SSL *ssl, /* * New SNI request */ - coap_dtls_key_t *new_entry = setup_data->validate_sni_call_back(sni, - setup_data->sni_call_back_arg); + coap_dtls_key_t *new_entry; + + coap_lock_callback_ret(new_entry, session->context, + setup_data->validate_sni_call_back(sni, + setup_data->sni_call_back_arg)); if (!new_entry) { *al = SSL_AD_UNRECOGNIZED_NAME; return SSL_CLIENT_HELLO_ERROR; @@ -3005,10 +3016,13 @@ psk_tls_client_hello_call_back(SSL *ssl, * New SNI request */ psk_sni_entry *tmp_entry; - const coap_dtls_spsk_info_t *new_entry = setup_data->validate_sni_call_back( - sni, - c_session, - setup_data->sni_call_back_arg); + const coap_dtls_spsk_info_t *new_entry; + + coap_lock_callback_ret(new_entry, c_session->context, + setup_data->validate_sni_call_back( + sni, + c_session, + setup_data->sni_call_back_arg)); if (!new_entry) { *al = SSL_AD_UNRECOGNIZED_NAME; return SSL_CLIENT_HELLO_ERROR; @@ -3019,7 +3033,8 @@ psk_tls_client_hello_call_back(SSL *ssl, (o_context->psk_sni_count+1)*sizeof(sni_entry)); if (tmp_entry) { o_context->psk_sni_entry_list = tmp_entry; - o_context->psk_sni_entry_list[o_context->psk_sni_count].sni = + o_context->psk_sni_entry_list[o_context->psk_sni_count] + .sni = OPENSSL_strdup(sni); if (o_context->psk_sni_entry_list[o_context->psk_sni_count].sni) { o_context->psk_sni_entry_list[o_context->psk_sni_count].psk_info = diff --git a/src/coap_resource.c b/src/coap_resource.c index e10cd5a7d0..6c71d271b8 100644 --- a/src/coap_resource.c +++ b/src/coap_resource.c @@ -471,8 +471,9 @@ coap_free_resource(coap_resource_t *resource) { resource->context->resource_deleted(resource->context, resource->uri_path, resource->context->observe_user_data); - if (resource->context->release_userdata && resource->user_data) - resource->context->release_userdata(resource->user_data); + if (resource->context->release_userdata && resource->user_data) { + coap_lock_callback(resource->context, resource->context->release_userdata(resource->user_data)); + } /* delete registered attributes */ LL_FOREACH_SAFE(resource->link_attr, attr, tmp) coap_delete_attr(attr); diff --git a/src/coap_tinydtls.c b/src/coap_tinydtls.c index ca842a8145..d2489c50d2 100644 --- a/src/coap_tinydtls.c +++ b/src/coap_tinydtls.c @@ -459,10 +459,10 @@ get_psk_info(struct dtls_context_t *dtls_context, lhint.length = id_len; lhint.s = id; - cpsk_info = - setup_cdata->validate_ih_call_back(&lhint, - coap_session, - setup_cdata->ih_call_back_arg); + coap_lock_callback_ret(cpsk_info, coap_session->context, + setup_cdata->validate_ih_call_back(&lhint, + coap_session, + setup_cdata->ih_call_back_arg)); if (cpsk_info) { psk_identity = &cpsk_info->identity; coap_session_refresh_psk_identity(coap_session, &cpsk_info->identity); @@ -619,6 +619,8 @@ verify_ecdsa_key(struct dtls_context_t *dtls_context COAP_UNUSED, size_t key_size) { coap_tiny_context_t *t_context = (coap_tiny_context_t *)dtls_get_app_data(dtls_context); + int ret; + if (t_context && t_context->setup_data.validate_cn_call_back) { /* Need to build asn.1 certificate - code taken from tinydtls */ uint8 *p; @@ -647,8 +649,10 @@ verify_ecdsa_key(struct dtls_context_t *dtls_context COAP_UNUSED, &remote_addr, dtls_session->ifindex); if (!c_session) return -3; - if (!t_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN, - buf, p-buf, c_session, 0, 1, t_context->setup_data.cn_call_back_arg)) { + coap_lock_callback_ret(ret, t_context->coap_context, + t_context->setup_data.validate_cn_call_back(COAP_DTLS_RPK_CERT_CN, + buf, p-buf, c_session, 0, 1, t_context->setup_data.cn_call_back_arg)); + if (!ret) { return -1; } } diff --git a/src/coap_wolfssl.c b/src/coap_wolfssl.c index e62cade802..7bd10beb4f 100644 --- a/src/coap_wolfssl.c +++ b/src/coap_wolfssl.c @@ -574,10 +574,10 @@ coap_dtls_psk_client_callback(WOLFSSL *ssl, lhint.s = temp.s; lhint.length = temp.length; - cpsk_info = - setup_data->validate_ih_call_back(&lhint, - c_session, - setup_data->ih_call_back_arg); + coap_lock_callback_ret(cpsk_info, c_session->context, + setup_data->validate_ih_call_back(&lhint, + c_session, + setup_data->ih_call_back_arg)); if (cpsk_info == NULL) return 0; @@ -1512,12 +1512,15 @@ tls_verify_call_back(int preverify_ok, WOLFSSL_X509_STORE_CTX *ctx) { if (length > 0) { uint8_t *base_buf; uint8_t *base_buf2 = base_buf = wolfssl_malloc(length); + int ret; /* base_buf2 gets moved to the end */ wolfSSL_i2d_X509(x509, &base_buf2); - if (!setup_data->validate_cn_call_back(cn, base_buf, length, session, - depth, preverify_ok, - setup_data->cn_call_back_arg)) { + coap_lock_callback_ret(ret, session->context, + setup_data->validate_cn_call_back(cn, base_buf, length, session, + depth, preverify_ok, + setup_data->cn_call_back_arg)); + if (!ret) { if (depth == 0) { wolfSSL_X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REJECTED); } else { @@ -1564,8 +1567,9 @@ tls_server_name_call_back(WOLFSSL *ssl, if (!sni || !sni[0]) { sni = ""; } - new_entry = setup_data->validate_sni_call_back(sni, - setup_data->sni_call_back_arg); + coap_lock_callback_ret(new_entry, session->context, + setup_data->validate_sni_call_back(sni, + setup_data->sni_call_back_arg)); if (!new_entry) { return fatal_return; } @@ -1610,15 +1614,17 @@ psk_tls_server_name_call_back(WOLFSSL *ssl, if (!sni || !sni[0]) { sni = ""; } - new_entry = setup_data->validate_sni_call_back(sni, - c_session, - setup_data->sni_call_back_arg); - coap_session_refresh_psk_key(c_session, - &new_entry->key); - snprintf(lhint, sizeof(lhint), "%.*s", - (int)new_entry->hint.length, - new_entry->hint.s); - wolfSSL_use_psk_identity_hint(ssl, lhint); + coap_lock_callback_ret(new_entry, c_session->context, + setup_data->validate_sni_call_back(sni, + c_session, + setup_data->sni_call_back_arg)); + if (new_entry) { + coap_session_refresh_psk_key(c_session, &new_entry->key); + snprintf(lhint, sizeof(lhint), "%.*s", + (int)new_entry->hint.length, + new_entry->hint.s); + wolfSSL_use_psk_identity_hint(ssl, lhint); + } } if (w_context->psk_pki_enabled & IS_PSK) {