From 04b8564249b1503756f7d79b43d0e8cb0a89cc83 Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Tue, 15 Sep 2020 12:17:52 +0530 Subject: [PATCH] provisioning: use memcpy instead of strncpy for copying SSID Per WiFi library requirement, SSID can be non-null terminated string if its length goes to 32 bytes (maximum). Use of strncpy in this case, along with compiler optimization level -O2 results in some warnings for potential use of non-null terminated strings. Fix here ensures use of memcpy to copy SSID string upto appropriate desired length. This helps to avoid compiler specific workaround flags added earlier. Closes https://github.com/espressif/esp-idf/issues/5866 Closes IDFGH-3983 --- components/wifi_provisioning/CMakeLists.txt | 6 ------ components/wifi_provisioning/component.mk | 3 --- components/wifi_provisioning/src/handlers.c | 11 ++++++++--- components/wifi_provisioning/src/scheme_softap.c | 7 +++++-- .../provisioning/ble_prov/main/app_prov_handlers.c | 8 ++++++-- .../console_prov/main/app_prov_handlers.c | 8 ++++++-- .../custom_config/main/app_prov_handlers.c | 8 ++++++-- .../provisioning/softap_prov/main/app_prov_handlers.c | 8 ++++++-- 8 files changed, 37 insertions(+), 22 deletions(-) diff --git a/components/wifi_provisioning/CMakeLists.txt b/components/wifi_provisioning/CMakeLists.txt index fd8128fdac..41e09b17f3 100644 --- a/components/wifi_provisioning/CMakeLists.txt +++ b/components/wifi_provisioning/CMakeLists.txt @@ -20,9 +20,3 @@ idf_component_register(SRCS "${srcs}" PRIV_INCLUDE_DIRS src proto-c ../protocomm/proto-c REQUIRES lwip protocomm PRIV_REQUIRES protobuf-c bt mdns json) - -# To avoid warning for strncpy -set_source_files_properties(src/handlers.c src/scheme_softap.c - PROPERTIES COMPILE_FLAGS - -Wno-stringop-truncation -) diff --git a/components/wifi_provisioning/component.mk b/components/wifi_provisioning/component.mk index 1d3fc74745..66a43c3f24 100644 --- a/components/wifi_provisioning/component.mk +++ b/components/wifi_provisioning/component.mk @@ -2,9 +2,6 @@ COMPONENT_SRCDIRS := src proto-c COMPONENT_ADD_INCLUDEDIRS := include COMPONENT_PRIV_INCLUDEDIRS := src proto-c ../protocomm/proto-c/ -# To avoid warning for strncpy in "handlers.c" and "scheme_softap.c" -CPPFLAGS += -Wno-stringop-truncation - ifndef CONFIG_BT_BLUEDROID_ENABLED ifndef CONFIG_BT_NIMBLE_ENABLED COMPONENT_OBJEXCLUDE := src/scheme_ble.o diff --git a/components/wifi_provisioning/src/handlers.c b/components/wifi_provisioning/src/handlers.c index 66f138a275..a1b6deeac7 100644 --- a/components/wifi_provisioning/src/handlers.c +++ b/components/wifi_provisioning/src/handlers.c @@ -105,10 +105,15 @@ static esp_err_t set_config_handler(const wifi_prov_config_set_data_t *req_data, ESP_LOGD(TAG, "Wi-Fi Credentials Received"); - /* Using strncpy allows the max SSID length to be 32 bytes (as per 802.11 standard). + /* Using memcpy allows the max SSID length to be 32 bytes (as per 802.11 standard). * But this doesn't guarantee that the saved SSID will be null terminated, because - * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character) */ - strncpy((char *) wifi_cfg->sta.ssid, req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character). + * Although, this is not a matter for concern because esp_wifi library reads the SSID + * upto 32 bytes in absence of null termination */ + const size_t ssid_len = strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + /* Ensure SSID less than 32 bytes is null terminated */ + memset(wifi_cfg->sta.ssid, 0, sizeof(wifi_cfg->sta.ssid)); + memcpy(wifi_cfg->sta.ssid, req_data->ssid, ssid_len); /* Using strlcpy allows both max passphrase length (63 bytes) and ensures null termination * because size of wifi_cfg->sta.password is 64 bytes (1 extra byte for null character) */ diff --git a/components/wifi_provisioning/src/scheme_softap.c b/components/wifi_provisioning/src/scheme_softap.c index 2b0c3c242b..fe25c1d613 100644 --- a/components/wifi_provisioning/src/scheme_softap.c +++ b/components/wifi_provisioning/src/scheme_softap.c @@ -43,8 +43,11 @@ static esp_err_t start_wifi_ap(const char *ssid, const char *pass) }, }; - strncpy((char *) wifi_config.ap.ssid, ssid, sizeof(wifi_config.ap.ssid)); - wifi_config.ap.ssid_len = strnlen(ssid, sizeof(wifi_config.ap.ssid)); + /* SSID can be a non NULL terminated string if `ap.ssid_len` is specified + * Hence, memcpy is used to support 32 bytes long SSID per 802.11 standard */ + const size_t ssid_len = strnlen(ssid, sizeof(wifi_config.ap.ssid)); + memcpy(wifi_config.ap.ssid, ssid, ssid_len); + wifi_config.ap.ssid_len = ssid_len; if (strlen(pass) == 0) { memset(wifi_config.ap.password, 0, sizeof(wifi_config.ap.password)); diff --git a/examples/provisioning/ble_prov/main/app_prov_handlers.c b/examples/provisioning/ble_prov/main/app_prov_handlers.c index 87cc64f0c1..b5d5d868c8 100644 --- a/examples/provisioning/ble_prov/main/app_prov_handlers.c +++ b/examples/provisioning/ble_prov/main/app_prov_handlers.c @@ -98,12 +98,16 @@ static esp_err_t set_config_handler(const wifi_prov_config_set_data_t *req_data, ESP_LOGI(TAG, "WiFi Credentials Received : \n\tssid %s \n\tpassword %s", req_data->ssid, req_data->password); - /* Using strncpy allows the max SSID length to be 32 bytes (as per 802.11 standard). + /* Using memcpy allows the max SSID length to be 32 bytes (as per 802.11 standard). * But this doesn't guarantee that the saved SSID will be null terminated, because * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character). * Although, this is not a matter for concern because esp_wifi library reads the SSID * upto 32 bytes in absence of null termination */ - strncpy((char *) wifi_cfg->sta.ssid, req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + const size_t ssid_len = strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + /* Ensure SSID less than 32 bytes is null terminated */ + memset(wifi_cfg->sta.ssid, 0, sizeof(wifi_cfg->sta.ssid)); + memcpy(wifi_cfg->sta.ssid, req_data->ssid, ssid_len); + strlcpy((char *) wifi_cfg->sta.password, req_data->password, sizeof(wifi_cfg->sta.password)); return ESP_OK; } diff --git a/examples/provisioning/console_prov/main/app_prov_handlers.c b/examples/provisioning/console_prov/main/app_prov_handlers.c index 850be09c15..26c3ba5f0f 100644 --- a/examples/provisioning/console_prov/main/app_prov_handlers.c +++ b/examples/provisioning/console_prov/main/app_prov_handlers.c @@ -98,12 +98,16 @@ static esp_err_t set_config_handler(const wifi_prov_config_set_data_t *req_data, ESP_LOGI(TAG, "WiFi Credentials Received : \n\tssid %s \n\tpassword %s", req_data->ssid, req_data->password); - /* Using strncpy allows the max SSID length to be 32 bytes (as per 802.11 standard). + /* Using memcpy allows the max SSID length to be 32 bytes (as per 802.11 standard). * But this doesn't guarantee that the saved SSID will be null terminated, because * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character). * Although, this is not a matter for concern because esp_wifi library reads the SSID * upto 32 bytes in absence of null termination */ - strncpy((char *) wifi_cfg->sta.ssid, req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + const size_t ssid_len = strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + /* Ensure SSID less than 32 bytes is null terminated */ + memset(wifi_cfg->sta.ssid, 0, sizeof(wifi_cfg->sta.ssid)); + memcpy(wifi_cfg->sta.ssid, req_data->ssid, ssid_len); + strlcpy((char *) wifi_cfg->sta.password, req_data->password, sizeof(wifi_cfg->sta.password)); return ESP_OK; } diff --git a/examples/provisioning/custom_config/main/app_prov_handlers.c b/examples/provisioning/custom_config/main/app_prov_handlers.c index 490be081c8..067c1128ff 100644 --- a/examples/provisioning/custom_config/main/app_prov_handlers.c +++ b/examples/provisioning/custom_config/main/app_prov_handlers.c @@ -110,12 +110,16 @@ static esp_err_t set_config_handler(const wifi_prov_config_set_data_t *req_data, ESP_LOGI(TAG, "WiFi Credentials Received : \n\tssid %s \n\tpassword %s", req_data->ssid, req_data->password); - /* Using strncpy allows the max SSID length to be 32 bytes (as per 802.11 standard). + /* Using memcpy allows the max SSID length to be 32 bytes (as per 802.11 standard). * But this doesn't guarantee that the saved SSID will be null terminated, because * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character). * Although, this is not a matter for concern because esp_wifi library reads the SSID * upto 32 bytes in absence of null termination */ - strncpy((char *) wifi_cfg->sta.ssid, req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + const size_t ssid_len = strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + /* Ensure SSID less than 32 bytes is null terminated */ + memset(wifi_cfg->sta.ssid, 0, sizeof(wifi_cfg->sta.ssid)); + memcpy(wifi_cfg->sta.ssid, req_data->ssid, ssid_len); + strlcpy((char *) wifi_cfg->sta.password, req_data->password, sizeof(wifi_cfg->sta.password)); return ESP_OK; } diff --git a/examples/provisioning/softap_prov/main/app_prov_handlers.c b/examples/provisioning/softap_prov/main/app_prov_handlers.c index 87cc64f0c1..b5d5d868c8 100644 --- a/examples/provisioning/softap_prov/main/app_prov_handlers.c +++ b/examples/provisioning/softap_prov/main/app_prov_handlers.c @@ -98,12 +98,16 @@ static esp_err_t set_config_handler(const wifi_prov_config_set_data_t *req_data, ESP_LOGI(TAG, "WiFi Credentials Received : \n\tssid %s \n\tpassword %s", req_data->ssid, req_data->password); - /* Using strncpy allows the max SSID length to be 32 bytes (as per 802.11 standard). + /* Using memcpy allows the max SSID length to be 32 bytes (as per 802.11 standard). * But this doesn't guarantee that the saved SSID will be null terminated, because * wifi_cfg->sta.ssid is also 32 bytes long (without extra 1 byte for null character). * Although, this is not a matter for concern because esp_wifi library reads the SSID * upto 32 bytes in absence of null termination */ - strncpy((char *) wifi_cfg->sta.ssid, req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + const size_t ssid_len = strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid)); + /* Ensure SSID less than 32 bytes is null terminated */ + memset(wifi_cfg->sta.ssid, 0, sizeof(wifi_cfg->sta.ssid)); + memcpy(wifi_cfg->sta.ssid, req_data->ssid, ssid_len); + strlcpy((char *) wifi_cfg->sta.password, req_data->password, sizeof(wifi_cfg->sta.password)); return ESP_OK; }