From cde7b7362adc15638c141c249681cbe5d23de663 Mon Sep 17 00:00:00 2001 From: Rahul Tank Date: Tue, 2 Dec 2025 15:37:01 +0530 Subject: [PATCH] fix(protocomm): Add security checks for buffer overflow and incorrect length handling --- .../include/transports/protocomm_ble.h | 5 + .../protocomm/src/simple_ble/simple_ble.c | 9 +- .../protocomm/src/simple_ble/simple_ble.h | 12 ++ .../protocomm/src/transports/protocomm_ble.c | 124 +++++++++++++++--- .../src/transports/protocomm_nimble.c | 84 ++++++++---- 5 files changed, 192 insertions(+), 42 deletions(-) diff --git a/components/protocomm/include/transports/protocomm_ble.h b/components/protocomm/include/transports/protocomm_ble.h index a6758a4cf31..22cd84785ec 100644 --- a/components/protocomm/include/transports/protocomm_ble.h +++ b/components/protocomm/include/transports/protocomm_ble.h @@ -143,6 +143,11 @@ typedef struct protocomm_ble_config { */ uint8_t *ble_addr; + /** + * Flag to keep BLE on + */ + unsigned keep_ble_on:1; + /** * BLE characteristic notify flag */ diff --git a/components/protocomm/src/simple_ble/simple_ble.c b/components/protocomm/src/simple_ble/simple_ble.c index 17ea1f9d25e..bf292da6354 100644 --- a/components/protocomm/src/simple_ble/simple_ble.c +++ b/components/protocomm/src/simple_ble/simple_ble.c @@ -14,6 +14,7 @@ #include #include #include +#include "esp_bt_device.h" #include #include "simple_ble.h" @@ -30,6 +31,11 @@ static esp_bd_addr_t s_cached_remote_bda = {0x0,}; #define adv_config_flag (1 << 0) #define scan_rsp_config_flag (1 << 1) +uint8_t get_keep_ble_on() +{ + return g_ble_cfg_p->keep_ble_on; +} + const uint8_t *simple_ble_get_uuid128(uint16_t handle) { const uint8_t *uuid128_ptr; @@ -250,7 +256,8 @@ esp_err_t simple_ble_start(simple_ble_cfg_t *cfg) } #endif - ret = esp_bluedroid_init(); + esp_bluedroid_config_t bluedroid_cfg = BT_BLUEDROID_INIT_CONFIG_DEFAULT(); + ret = esp_bluedroid_init_with_cfg(&bluedroid_cfg); if (ret) { ESP_LOGE(TAG, "%s init bluetooth failed %d", __func__, ret); return ret; diff --git a/components/protocomm/src/simple_ble/simple_ble.h b/components/protocomm/src/simple_ble/simple_ble.h index 259225de830..dd3858096fa 100644 --- a/components/protocomm/src/simple_ble/simple_ble.h +++ b/components/protocomm/src/simple_ble/simple_ble.h @@ -53,10 +53,22 @@ typedef struct { unsigned ble_sm_sc:1; /** BLE Address */ uint8_t *ble_addr; + /** Flag to keep BLE on */ + unsigned keep_ble_on:1; } simple_ble_cfg_t; +/** + * @brief Get the current BLE keep-on status + * + * This function returns the current value of the `keep_ble_on` flag + * from the global BLE configuration structure. + * + * @return uint8_t Current status of the `keep_ble_on` flag + */ +uint8_t get_keep_ble_on(void); + /** Initialize a simple ble connection * * This function allocates memory and returns a pointer to the diff --git a/components/protocomm/src/transports/protocomm_ble.c b/components/protocomm/src/transports/protocomm_ble.c index 742f453d9c0..2e0f9d44499 100644 --- a/components/protocomm/src/transports/protocomm_ble.c +++ b/components/protocomm/src/transports/protocomm_ble.c @@ -1,10 +1,11 @@ /* - * SPDX-FileCopyrightText: 2018-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2018-2025 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ #include +#include #include #include #include @@ -71,6 +72,21 @@ typedef struct _protocomm_ble { static _protocomm_ble_internal_t *protoble_internal; +static bool protocomm_ble_transport_active(void) +{ + return (protoble_internal != NULL) && (protoble_internal->pc_ble != NULL); +} + +static void protocomm_ble_reset_prepare_write(void) +{ + if (prepare_write_env.prepare_buf) { + free(prepare_write_env.prepare_buf); + prepare_write_env.prepare_buf = NULL; + } + prepare_write_env.prepare_len = 0; + prepare_write_env.handle = 0; +} + // config adv data static esp_ble_adv_data_t adv_config = { .set_scan_rsp = false, @@ -121,6 +137,9 @@ static const uint16_t *uuid128_to_16(const uint8_t *uuid128) static const char *handle_to_handler(uint16_t handle) { + if (protoble_internal == NULL) { + return NULL; + } const uint8_t *uuid128 = simple_ble_get_uuid128(handle); if (!uuid128) { return NULL; @@ -140,6 +159,18 @@ static void transport_simple_ble_read(esp_gatts_cb_event_t event, esp_gatt_if_t static uint16_t max_read_len = 0; esp_gatt_status_t status = ESP_OK; + if (!protocomm_ble_transport_active()) { + read_len = 0; + max_read_len = 0; + read_buf = NULL; + esp_err_t err = esp_ble_gatts_send_response(gatts_if, param->read.conn_id, + param->read.trans_id, ESP_GATT_ERROR, NULL); + if (err != ESP_OK) { + ESP_LOGE(TAG, "Send response error in read"); + } + return; + } + ESP_LOGD(TAG, "Inside read w/ session - %d on param %d %d", param->read.conn_id, param->read.handle, read_len); if (!read_len && !param->read.offset) { @@ -206,7 +237,8 @@ static esp_err_t prepare_write_event_env(esp_gatt_if_t gatts_if, memcpy(prepare_write_env.prepare_buf + param->write.offset, param->write.value, param->write.len); - prepare_write_env.prepare_len += param->write.len; + int next_len = param->write.offset + param->write.len; + prepare_write_env.prepare_len = MAX(prepare_write_env.prepare_len, next_len); prepare_write_env.handle = param->write.handle; } @@ -254,6 +286,17 @@ static void transport_simple_ble_write(esp_gatts_cb_event_t event, esp_gatt_if_t ESP_LOGD(TAG, "Inside write with session - %d on attr handle = %d \nlen = %d, is_prep = %d", param->write.conn_id, param->write.handle, param->write.len, param->write.is_prep); + if (!protocomm_ble_transport_active()) { + ESP_LOGW(TAG, "Ignoring write on inactive protocomm transport"); + protocomm_ble_reset_prepare_write(); + if (param->write.need_rsp) { + esp_ble_gatts_send_response(gatts_if, param->write.conn_id, + param->write.trans_id, ESP_GATT_ERROR, NULL); + } + esp_ble_gatts_close(gatts_if, param->write.conn_id); + return; + } + if (param->write.is_prep) { ret = prepare_write_event_env(gatts_if, param); if (ret != ESP_OK) { @@ -298,6 +341,15 @@ static void transport_simple_ble_exec_write(esp_gatts_cb_event_t event, esp_gatt ssize_t outlen = 0; ESP_LOGD(TAG, "Inside exec_write w/ session - %d", param->exec_write.conn_id); + if (!protocomm_ble_transport_active()) { + ESP_LOGW(TAG, "Ignoring exec write on inactive protocomm transport"); + protocomm_ble_reset_prepare_write(); + esp_ble_gatts_send_response(gatts_if, param->exec_write.conn_id, + param->exec_write.trans_id, ESP_GATT_ERROR, NULL); + esp_ble_gatts_close(gatts_if, param->exec_write.conn_id); + return; + } + if ((param->exec_write.exec_write_flag == ESP_GATT_PREP_WRITE_EXEC) && prepare_write_env.prepare_buf) { @@ -336,12 +388,20 @@ static void transport_simple_ble_disconnect(esp_gatts_cb_event_t event, esp_gatt esp_err_t ret; ESP_LOGD(TAG, "Inside disconnect w/ session - %d", param->disconnect.conn_id); + /* Drop any staged prepare-write data when a connection ends */ + protocomm_ble_reset_prepare_write(); + /* Ignore BLE events received after protocomm layer is stopped */ if (protoble_internal == NULL) { ESP_LOGI(TAG,"Protocomm layer has already stopped"); return; } + if (!protocomm_ble_transport_active()) { + ESP_LOGD(TAG, "Protocomm BLE inactive, ignoring disconnect"); + return; + } + if (protoble_internal->pc_ble->sec && protoble_internal->pc_ble->sec->close_transport_session) { ret = protoble_internal->pc_ble->sec->close_transport_session(protoble_internal->pc_ble->sec_inst, @@ -368,12 +428,20 @@ static void transport_simple_ble_connect(esp_gatts_cb_event_t event, esp_gatt_if esp_err_t ret; ESP_LOGD(TAG, "Inside BLE connect w/ conn_id - %d", param->connect.conn_id); + /* Start each connection with a clean prepare-write buffer */ + protocomm_ble_reset_prepare_write(); + /* Ignore BLE events received after protocomm layer is stopped */ if (protoble_internal == NULL) { ESP_LOGI(TAG,"Protocomm layer has already stopped"); return; } + if (!protocomm_ble_transport_active()) { + ESP_LOGD(TAG, "Protocomm BLE inactive, ignoring connect"); + return; + } + if (protoble_internal->pc_ble->sec && protoble_internal->pc_ble->sec->new_transport_session) { ret = protoble_internal->pc_ble->sec->new_transport_session(protoble_internal->pc_ble->sec_inst, @@ -396,6 +464,9 @@ static void transport_simple_ble_connect(esp_gatts_cb_event_t event, esp_gatt_if static void transport_simple_ble_set_mtu(esp_gatts_cb_event_t event, esp_gatt_if_t gatts_if, esp_ble_gatts_cb_param_t *param) { + if (!protocomm_ble_transport_active()) { + return; + } protoble_internal->gatt_mtu = param->mtu.mtu; return; } @@ -485,6 +556,7 @@ static ssize_t populate_gatt_db(esp_gatts_attr_db_t **gatt_db_generated) static void protocomm_ble_cleanup(void) { + protocomm_ble_reset_prepare_write(); if (protoble_internal) { if (protoble_internal->g_nu_lookup) { for (unsigned i = 0; i < protoble_internal->g_nu_lookup_count; i++) { @@ -608,6 +680,9 @@ esp_err_t protocomm_ble_start(protocomm_t *pc, const protocomm_ble_config_t *con ble_config->ble_bonding = config->ble_bonding; ble_config->ble_sm_sc = config->ble_sm_sc; + /* Set parameter to keep BLE on */ + ble_config->keep_ble_on = config->keep_ble_on; + if (config->ble_addr != NULL) { ble_config->ble_addr = protocomm_ble_addr; } @@ -634,31 +709,44 @@ esp_err_t protocomm_ble_start(protocomm_t *pc, const protocomm_ble_config_t *con esp_err_t protocomm_ble_stop(protocomm_t *pc) { - if ((pc != NULL) && - (protoble_internal != NULL ) && - (pc == protoble_internal->pc_ble)) { - esp_err_t ret = ESP_OK; + if ((pc == NULL) || + (protoble_internal == NULL ) || + (pc != protoble_internal->pc_ble)) { + return ESP_ERR_INVALID_ARG; + } -#ifdef CONFIG_ESP_PROTOCOMM_KEEP_BLE_ON_AFTER_BLE_STOP + esp_err_t ret = ESP_OK; + bool ble_callbacks_active = true; + uint8_t protocomm_keep_ble_on = get_keep_ble_on(); + + protoble_internal->pc_ble = NULL; + + if (protocomm_keep_ble_on) { #ifdef CONFIG_ESP_PROTOCOMM_DISCONNECT_AFTER_BLE_STOP /* Keep BT stack on, but terminate the connection after provisioning */ - ret = simple_ble_disconnect(); - if (ret) { - ESP_LOGE(TAG, "BLE disconnect failed"); - } - simple_ble_deinit(); -#endif // CONFIG_ESP_PROTOCOMM_DISCONNECT_AFTER_BLE_STOP + ret = simple_ble_disconnect(); + if (ret) { + ESP_LOGE(TAG, "BLE disconnect failed"); + } #else + ESP_LOGD(TAG, "Keeping BLE stack running after protocomm stop"); +#endif // CONFIG_ESP_PROTOCOMM_DISCONNECT_AFTER_BLE_STOP + } + else { + /* If flag is not enabled, stop the stack. */ ret = simple_ble_stop(); if (ret) { ESP_LOGE(TAG, "BLE stop failed"); } simple_ble_deinit(); -#endif // CONFIG_ESP_PROTOCOMM_KEEP_BLE_ON_AFTER_BLE_STOP - - protocomm_ble_cleanup(); - return ret; + ble_callbacks_active = false; } - return ESP_ERR_INVALID_ARG; + + protocomm_ble_reset_prepare_write(); + + if (!ble_callbacks_active) { + protocomm_ble_cleanup(); + } + return ret; } diff --git a/components/protocomm/src/transports/protocomm_nimble.c b/components/protocomm/src/transports/protocomm_nimble.c index f9c7acffca2..18518f8416c 100644 --- a/components/protocomm/src/transports/protocomm_nimble.c +++ b/components/protocomm/src/transports/protocomm_nimble.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include @@ -73,6 +74,11 @@ typedef struct _protocomm_ble { } _protocomm_ble_internal_t; static _protocomm_ble_internal_t *protoble_internal; + +static bool protocomm_ble_transport_active(void) +{ + return (protoble_internal != NULL) && (protoble_internal->pc_ble != NULL); +} static struct ble_gap_adv_params adv_params; static char *protocomm_ble_device_name; static struct ble_hs_adv_fields adv_data, resp_data; @@ -283,6 +289,9 @@ simple_ble_gap_event(struct ble_gap_event *event, void *arg) /* Gets `g_nu_lookup name handler` from 128 bit UUID */ static const char *uuid128_to_handler(uint8_t *uuid) { + if (!protocomm_ble_transport_active()) { + return NULL; + } /* Use it to convert 128 bit UUID to 16 bit UUID.*/ uint8_t *uuid16 = uuid + 12; for (int i = 0; i < protoble_internal->g_nu_lookup_count; i++) { @@ -301,6 +310,11 @@ static int gatt_svr_dsc_access(uint16_t conn_handle, uint16_t attr_handle, struct ble_gatt_access_ctxt *ctxt, void *arg) { + if (!protocomm_ble_transport_active()) { + ESP_LOGW(TAG, "Ignoring descriptor access on inactive protocomm transport"); + return BLE_ATT_ERR_UNLIKELY; + } + if (ctxt->op != BLE_GATT_ACCESS_OP_READ_DSC) { ESP_LOGE(TAG, "Invalid operation on Read-only Descriptor"); return BLE_ATT_ERR_UNLIKELY; @@ -329,6 +343,11 @@ gatt_svr_chr_access(uint16_t conn_handle, uint16_t attr_handle, uint16_t data_len = 0; uint16_t data_buf_len = 0; + if (!protocomm_ble_transport_active()) { + ESP_LOGW(TAG, "Ignoring characteristic access on inactive protocomm transport"); + return BLE_ATT_ERR_UNLIKELY; + } + switch (ctxt->op) { case BLE_GATT_ACCESS_OP_READ_CHR: ESP_LOGD(TAG, "Read attempted for characteristic UUID = %s, attr_handle = %d", @@ -593,6 +612,11 @@ static void transport_simple_ble_disconnect(struct ble_gap_event *event, void *a return; } + if (!protocomm_ble_transport_active()) { + ESP_LOGD(TAG, "Protocomm BLE inactive, ignoring disconnect"); + return; + } + if (protoble_internal->pc_ble->sec && protoble_internal->pc_ble->sec->close_transport_session) { ret = @@ -626,6 +650,11 @@ static void transport_simple_ble_connect(struct ble_gap_event *event, void *arg) return; } + if (!protocomm_ble_transport_active()) { + ESP_LOGD(TAG, "Protocomm BLE inactive, ignoring connect"); + return; + } + if (protoble_internal->pc_ble->sec && protoble_internal->pc_ble->sec->new_transport_session) { ret = @@ -649,6 +678,9 @@ static void transport_simple_ble_connect(struct ble_gap_event *event, void *arg) static void transport_simple_ble_set_mtu(struct ble_gap_event *event, void *arg) { + if (!protocomm_ble_transport_active()) { + return; + } protoble_internal->gatt_mtu = event->mtu.value; return; } @@ -1038,37 +1070,43 @@ esp_err_t protocomm_ble_start(protocomm_t *pc, const protocomm_ble_config_t *con esp_err_t protocomm_ble_stop(protocomm_t *pc) { ESP_LOGD(TAG, "protocomm_ble_stop called here..."); - if ((pc != NULL) && - (protoble_internal != NULL ) && - (pc == protoble_internal->pc_ble)) { - esp_err_t ret = ESP_OK; + if ((pc == NULL) || + (protoble_internal == NULL ) || + (pc != protoble_internal->pc_ble)) { + return ESP_ERR_INVALID_ARG; + } - esp_err_t rc = ble_gap_adv_stop(); - if (rc) { - ESP_LOGD(TAG, "Error in stopping advertisement with err code = %d", - rc); - } + esp_err_t ret = ESP_OK; + bool ble_callbacks_active = true; + esp_err_t rc = ble_gap_adv_stop(); + if (rc) { + ESP_LOGD(TAG, "Error in stopping advertisement with err code = %d", + rc); + } + + protoble_internal->pc_ble = NULL; #ifdef CONFIG_ESP_PROTOCOMM_KEEP_BLE_ON_AFTER_BLE_STOP + /* Keep BLE host running after provisioning; leave resources intact */ #ifdef CONFIG_ESP_PROTOCOMM_DISCONNECT_AFTER_BLE_STOP - /* Keep BT stack on, but terminate the connection after provisioning */ - rc = ble_gap_terminate(s_cached_conn_handle, BLE_ERR_REM_USER_CONN_TERM); - if (rc) { - ESP_LOGI(TAG, "Error in terminating connection rc = %d",rc); - } - free_gatt_ble_misc_memory(ble_cfg_p); + rc = ble_gap_terminate(s_cached_conn_handle, BLE_ERR_REM_USER_CONN_TERM); + if (rc) { + ESP_LOGI(TAG, "Error in terminating connection rc = %d", rc); + } #endif // CONFIG_ESP_PROTOCOMM_DISCONNECT_AFTER_BLE_STOP + ESP_LOGD(TAG, "Keeping BLE stack running after protocomm stop"); #else - /* If flag is enabled, don't stop the stack. User application can start a new advertising to perform its BT activities */ - ret = nimble_port_stop(); - if (ret == 0) { - nimble_port_deinit(); - } - free_gatt_ble_misc_memory(ble_cfg_p); + /* Stop BLE stack and tear down GATT metadata */ + ret = nimble_port_stop(); + if (ret == 0) { + nimble_port_deinit(); + } + free_gatt_ble_misc_memory(ble_cfg_p); + ble_callbacks_active = false; #endif // CONFIG_ESP_PROTOCOMM_KEEP_BLE_ON_AFTER_BLE_STOP + if (!ble_callbacks_active) { protocomm_ble_cleanup(); - return ret; } - return ESP_ERR_INVALID_ARG; + return ret; }