From eed4636870750b79f058c30dc63a57f069017066 Mon Sep 17 00:00:00 2001 From: Rahul Tank Date: Tue, 20 Jan 2026 12:12:04 +0530 Subject: [PATCH] fix(nimble): Address security vulnerabilties and NVS corruption issues. Security Issues Fixed: 1. Buffer Overflow Prevention (ble_store_nvs.c) - Changed all sprintf() calls to snprintf() in get_nvs_key_string() to prevent potential buffer overflow when generating NVS key strings - Changed index parameter type from int8_t to int in ble_nvs_delete_value() to handle values > 127 correctly 2. Bounds Checking (ble_store_nvs.c) - Added bounds checking in populate_db_from_nvs() before each memcpy operation to prevent buffer overflow when NVS contains more entries than RAM arrays can hold (due to config mismatch or NVS corruption) 3. Array Index Validation (ble_store_nvs.c) - Fixed potential array index -1 access in ble_nvs_restore_sec_keys() by adding checks before accessing [num - 1] indices - Fixed loop bounds to use actual populated count instead of max config value 4. NVS Data Size Validation (ble_store_nvs.c) - Added get_expected_size_for_obj_type() helper function - Added size validation in get_nvs_db_value() and get_nvs_peer_record() to detect corrupted or version-mismatched NVS data before reading 5. Type Safety Fix (ble_store_config_conf.c) - Fixed critical typo: && to & for ble_store_config_num_csfcs - Added missing union members (csfc, ead) in ble_store_config_conf_export() 6. Thread Safety (ble_gattc_cache.c) - Replaced static buffer with thread-local storage (__thread) in getKeyname() - Changed sprintf() to snprintf() with explicit size limit - Increased buffer size from 16 to 20 bytes for safety margin --- nimble/host/src/ble_gattc_cache.c | 17 ++- .../store/config/src/ble_store_config_conf.c | 8 +- nimble/host/store/config/src/ble_store_nvs.c | 130 ++++++++++++++++-- 3 files changed, 133 insertions(+), 22 deletions(-) diff --git a/nimble/host/src/ble_gattc_cache.c b/nimble/host/src/ble_gattc_cache.c index 037c01861..e0d38075a 100644 --- a/nimble/host/src/ble_gattc_cache.c +++ b/nimble/host/src/ble_gattc_cache.c @@ -96,15 +96,20 @@ print_addr(ble_addr_t addr) } } +/* Increased buffer size to safely hold "key" + 12 hex chars + null + margin */ +#define GATT_CACHE_KEY_NAME_MAX_LEN 20 + +/* Thread-local storage for key name to avoid static buffer thread safety issues */ +static __thread char tls_key_buffer[GATT_CACHE_KEY_NAME_MAX_LEN]; + char *getKeyname(const ble_addr_t *addr) { - static char buffer[16]; - sprintf(buffer, "%s%02X%02X%02X%02X%02X%02X", - cache_key, - addr->val[5], addr->val[4], addr->val[3], - addr->val[2], addr->val[1], addr->val[0]); + snprintf(tls_key_buffer, GATT_CACHE_KEY_NAME_MAX_LEN, "%s%02X%02X%02X%02X%02X%02X", + cache_key, + addr->val[5], addr->val[4], addr->val[3], + addr->val[2], addr->val[1], addr->val[0]); - return buffer; + return tls_key_buffer; } static int diff --git a/nimble/host/store/config/src/ble_store_config_conf.c b/nimble/host/store/config/src/ble_store_config_conf.c index 181ee04d1..09dd0e4c7 100644 --- a/nimble/host/store/config/src/ble_store_config_conf.c +++ b/nimble/host/store/config/src/ble_store_config_conf.c @@ -144,7 +144,7 @@ ble_store_config_conf_set(int argc, char **argv, char *val) val, ble_store_config_csfcs, sizeof *ble_store_config_csfcs, - &&ble_store_config_num_csfcs); + &ble_store_config_num_csfcs); return rc; } #endif @@ -180,6 +180,12 @@ ble_store_config_conf_export(void (*func)(char *name, char *val), char sec[BLE_STORE_CONFIG_SEC_SET_ENCODE_SZ]; char cccd[BLE_STORE_CONFIG_CCCD_SET_ENCODE_SZ]; char rpa_rec[BLE_STORE_CONFIG_RPA_REC_SET_ENCODE_SZ]; +#if MYNEWT_VAL(BLE_STORE_MAX_CSFCS) + char csfc[BLE_STORE_CONFIG_CSFC_SET_ENCODE_SZ]; +#endif +#if MYNEWT_VAL(ENC_ADV_DATA) + char ead[BLE_STORE_CONFIG_EAD_SET_ENCODE_SZ]; +#endif } buf; ble_store_config_serialize_arr(ble_store_config_our_secs, diff --git a/nimble/host/store/config/src/ble_store_nvs.c b/nimble/host/store/config/src/ble_store_nvs.c index 7b076b994..3f7512bea 100644 --- a/nimble/host/store/config/src/ble_store_nvs.c +++ b/nimble/host/store/config/src/ble_store_nvs.c @@ -60,25 +60,25 @@ static void get_nvs_key_string(int obj_type, int index, char *key_string) { if (obj_type == BLE_STORE_OBJ_TYPE_PEER_DEV_REC) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_PEER_RECORDS_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_PEER_RECORDS_KEY, index); } else { if (obj_type == BLE_STORE_OBJ_TYPE_PEER_SEC) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_PEER_SEC_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_PEER_SEC_KEY, index); } else if (obj_type == BLE_STORE_OBJ_TYPE_OUR_SEC) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_OUR_SEC_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_OUR_SEC_KEY, index); #if MYNEWT_VAL(ENC_ADV_DATA) } else if (obj_type == BLE_STORE_OBJ_TYPE_ENC_ADV_DATA) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_EAD_SEC_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_EAD_SEC_KEY, index); #endif } else if (obj_type == BLE_STORE_OBJ_TYPE_LOCAL_IRK) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_LOCAL_IRK_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_LOCAL_IRK_KEY, index); } else if (obj_type == BLE_STORE_OBJ_TYPE_PEER_ADDR){ - sprintf(key_string, "%s_%d", NIMBLE_NVS_RPA_RECORDS_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_RPA_RECORDS_KEY, index); } else if (obj_type == BLE_STORE_OBJ_TYPE_CCCD) { - sprintf(key_string, "%s_%d", NIMBLE_NVS_CCCD_SEC_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_CCCD_SEC_KEY, index); } else { - sprintf(key_string, "%s_%d", NIMBLE_NVS_CSFC_SEC_KEY, index); + snprintf(key_string, NIMBLE_NVS_STR_NAME_MAX_LEN, "%s_%d", NIMBLE_NVS_CSFC_SEC_KEY, index); } } } @@ -149,6 +149,14 @@ get_nvs_peer_record(char *key_string, struct ble_hs_dev_records *p_dev_rec) goto end; } + /* Validate that NVS data size matches expected struct size */ + if (required_size != sizeof(struct ble_hs_dev_records)) { + ESP_LOGE(TAG, "NVS data size mismatch for peer record: expected %d, got %d", + (int)sizeof(struct ble_hs_dev_records), (int)required_size); + err = ESP_ERR_NVS_INVALID_LENGTH; + goto end; + } + err = nvs_get_blob(nimble_handle, key_string, p_dev_rec, &required_size); @@ -158,11 +166,35 @@ end: } #endif +static size_t +get_expected_size_for_obj_type(int obj_type) +{ + switch (obj_type) { + case BLE_STORE_OBJ_TYPE_CCCD: + return sizeof(struct ble_store_value_cccd); + case BLE_STORE_OBJ_TYPE_CSFC: + return sizeof(struct ble_store_value_csfc); +#if MYNEWT_VAL(ENC_ADV_DATA) + case BLE_STORE_OBJ_TYPE_ENC_ADV_DATA: + return sizeof(struct ble_store_value_ead); +#endif + case BLE_STORE_OBJ_TYPE_LOCAL_IRK: + return sizeof(struct ble_store_value_local_irk); + case BLE_STORE_OBJ_TYPE_PEER_ADDR: + return sizeof(struct ble_store_value_rpa_rec); + case BLE_STORE_OBJ_TYPE_PEER_SEC: + case BLE_STORE_OBJ_TYPE_OUR_SEC: + default: + return sizeof(struct ble_store_value_sec); + } +} + static int get_nvs_db_value(int obj_type, char *key_string, union ble_store_value *val) { esp_err_t err; size_t required_size = 0; + size_t expected_size; nvs_handle_t nimble_handle; err = nvs_open(NIMBLE_NVS_NAMESPACE, NVS_READWRITE, &nimble_handle); @@ -178,6 +210,15 @@ get_nvs_db_value(int obj_type, char *key_string, union ble_store_value *val) goto end; } + /* Validate that NVS data size matches expected struct size */ + expected_size = get_expected_size_for_obj_type(obj_type); + if (required_size != expected_size) { + ESP_LOGE(TAG, "NVS data size mismatch for obj_type %d: expected %d, got %d", + obj_type, (int)expected_size, (int)required_size); + err = ESP_ERR_NVS_INVALID_LENGTH; + goto end; + } + if (obj_type == BLE_STORE_OBJ_TYPE_CCCD) { err = nvs_get_blob(nimble_handle, key_string, &val->cccd, &required_size); @@ -307,7 +348,7 @@ get_nvs_db_attribute(int obj_type, bool empty, void *value, int num_value) * -1 on NVS memory access failure */ static int -ble_nvs_delete_value(int obj_type, int8_t index) +ble_nvs_delete_value(int obj_type, int index) { esp_err_t err; nvs_handle_t nimble_handle; @@ -463,9 +504,13 @@ populate_db_from_nvs(int obj_type, void *dst, int *db_num) esp_err_t err; int i; + int max_entries; char key_string[NIMBLE_NVS_STR_NAME_MAX_LEN]; - for (i = 1; i <= get_nvs_max_obj_value(obj_type); i++) { + /* Get the maximum number of entries allowed for this object type */ + max_entries = get_nvs_max_obj_value(obj_type); + + for (i = 1; i <= max_entries; i++) { get_nvs_key_string(obj_type, i, key_string); #if MYNEWT_VAL(BLE_HOST_BASED_PRIVACY) @@ -491,6 +536,11 @@ populate_db_from_nvs(int obj_type, void *dst, int *db_num) /* NVS index has data, fill up the ram db with it */ if (obj_type == BLE_STORE_OBJ_TYPE_PEER_DEV_REC) { + /* Bounds check before writing to RAM array */ + if (*db_num >= max_entries) { + ESP_LOGW(TAG, "Peer dev records: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "Peer dev records filled from NVS index = %d", i); memcpy(db_item, &p_dev_rec, sizeof(struct ble_hs_dev_records)); db_item += sizeof(struct ble_hs_dev_records); @@ -499,34 +549,64 @@ populate_db_from_nvs(int obj_type, void *dst, int *db_num) #endif { if (obj_type == BLE_STORE_OBJ_TYPE_CCCD) { + /* Bounds check before writing to RAM array */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_CCCDS)) { + ESP_LOGW(TAG, "CCCD: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "CCCD in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.cccd, sizeof(struct ble_store_value_cccd)); db_item += sizeof(struct ble_store_value_cccd); (*db_num)++; } else if (obj_type == BLE_STORE_OBJ_TYPE_CSFC) { + /* Bounds check before writing to RAM array */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_CSFCS)) { + ESP_LOGW(TAG, "CSFC: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "CSFC in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.csfc, sizeof(struct ble_store_value_csfc)); db_item += sizeof(struct ble_store_value_csfc); (*db_num)++; #if MYNEWT_VAL(ENC_ADV_DATA) } else if (obj_type == BLE_STORE_OBJ_TYPE_ENC_ADV_DATA) { + /* Bounds check before writing to RAM array */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_EADS)) { + ESP_LOGW(TAG, "EAD: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "EAD in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.ead, sizeof(struct ble_store_value_ead)); db_item += sizeof(struct ble_store_value_ead); (*db_num)++; #endif } else if(obj_type == BLE_STORE_OBJ_TYPE_LOCAL_IRK) { + /* Bounds check before writing to RAM array */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { + ESP_LOGW(TAG, "Local IRK: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "Local IRK in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.local_irk, sizeof(struct ble_store_value_local_irk)); db_item += sizeof(struct ble_store_value_local_irk); (*db_num)++; } else if(obj_type == BLE_STORE_OBJ_TYPE_PEER_ADDR) { + /* Bounds check before writing to RAM array */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { + ESP_LOGW(TAG, "RPA_REC: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "RPA_REC in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.rpa_rec, sizeof(struct ble_store_value_rpa_rec)); db_item += sizeof(struct ble_store_value_rpa_rec); (*db_num)++; } else { + /* Bounds check before writing to RAM array (sec type) */ + if (*db_num >= MYNEWT_VAL(BLE_STORE_MAX_BONDS)) { + ESP_LOGW(TAG, "SEC: RAM array full, skipping NVS index %d", i); + continue; + } ESP_LOGD(TAG, "KEY in RAM is filled up from NVS index = %d", i); memcpy(db_item, &cur.sec, sizeof(struct ble_store_value_sec)); db_item += sizeof(struct ble_store_value_sec); @@ -563,11 +643,21 @@ ble_nvs_restore_sec_keys(void) return err; } - for (int i = 0; i < MYNEWT_VAL(BLE_STORE_MAX_BONDS) - 1; i++) { - if ((ble_store_config_our_secs[i].bond_count > ble_store_config_our_secs[i+1].bond_count) - || (ble_store_config_peer_secs[i].bond_count > ble_store_config_peer_secs[i+1].bond_count)) { + /* Only check for out-of-order entries if we have valid data */ + if (ble_store_config_num_our_secs > 1 || ble_store_config_num_peer_secs > 1) { + /* Use actual populated count, not max config value */ + int our_check_limit = (ble_store_config_num_our_secs > 1) ? (ble_store_config_num_our_secs - 1) : 0; + int peer_check_limit = (ble_store_config_num_peer_secs > 1) ? (ble_store_config_num_peer_secs - 1) : 0; + int check_limit = (our_check_limit > peer_check_limit) ? our_check_limit : peer_check_limit; + + for (int i = 0; i < check_limit; i++) { + if ((i < our_check_limit && + ble_store_config_our_secs[i].bond_count > ble_store_config_our_secs[i+1].bond_count) || + (i < peer_check_limit && + ble_store_config_peer_secs[i].bond_count > ble_store_config_peer_secs[i+1].bond_count)) { flag = 1; break; + } } } @@ -580,8 +670,18 @@ ble_nvs_restore_sec_keys(void) sizeof(struct ble_store_value_sec), ble_store_config_compare_bond_count); } - ble_store_config_our_bond_count = ble_store_config_our_secs[ble_store_config_num_our_secs - 1].bond_count; - ble_store_config_peer_bond_count = ble_store_config_peer_secs[ble_store_config_num_peer_secs - 1].bond_count; + /* Only access array if we have valid entries to prevent index -1 access */ + if (ble_store_config_num_our_secs > 0) { + ble_store_config_our_bond_count = ble_store_config_our_secs[ble_store_config_num_our_secs - 1].bond_count; + } else { + ble_store_config_our_bond_count = 0; + } + + if (ble_store_config_num_peer_secs > 0) { + ble_store_config_peer_bond_count = ble_store_config_peer_secs[ble_store_config_num_peer_secs - 1].bond_count; + } else { + ble_store_config_peer_bond_count = 0; + } ESP_LOGD(TAG, "ble_store_config_peer_secs restored %d bonds", ble_store_config_num_peer_secs);