From 76719b9c379b662efbc8ed54297d2ddd88ad60f3 Mon Sep 17 00:00:00 2001 From: Anurag Kar Date: Mon, 10 Jun 2019 17:49:30 +0530 Subject: [PATCH 1/4] Wi-Fi Provisioning : Bugfix in copying SSID and Passphrase These changes guarantee that the SSID and Passphrase received via protocomm are NULL terminated and size limited to their standard lengths. List of changes: * Corrected length of passphrase field in wifi_prov_config_set_data_t structure * Performing length checks on SSID, passphrase and bssid, when populating wifi_prov_config_set_data_t structure with received credentials --- .../include/wifi_provisioning/wifi_config.h | 2 +- .../wifi_provisioning/src/wifi_config.c | 41 ++++++++++++++----- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/components/wifi_provisioning/include/wifi_provisioning/wifi_config.h b/components/wifi_provisioning/include/wifi_provisioning/wifi_config.h index 4058540ad..2fa64448d 100644 --- a/components/wifi_provisioning/include/wifi_provisioning/wifi_config.h +++ b/components/wifi_provisioning/include/wifi_provisioning/wifi_config.h @@ -76,7 +76,7 @@ typedef struct { */ typedef struct { char ssid[33]; /*!< SSID of the AP to which the slave is to be connected */ - char password[65]; /*!< Password of the AP */ + char password[64]; /*!< Password of the AP */ char bssid[6]; /*!< BSSID of the AP */ uint8_t channel; /*!< Channel of the AP */ } wifi_prov_config_set_data_t; diff --git a/components/wifi_provisioning/src/wifi_config.c b/components/wifi_provisioning/src/wifi_config.c index 09ccc37d5..93e3e8564 100644 --- a/components/wifi_provisioning/src/wifi_config.c +++ b/components/wifi_provisioning/src/wifi_config.c @@ -151,15 +151,36 @@ static esp_err_t cmd_set_config_handler(WiFiConfigPayload *req, wifi_prov_config_set_data_t req_data; memset(&req_data, 0, sizeof(req_data)); - memcpy(req_data.ssid, req->cmd_set_config->ssid.data, - req->cmd_set_config->ssid.len); - memcpy(req_data.password, req->cmd_set_config->passphrase.data, - req->cmd_set_config->passphrase.len); - memcpy(req_data.bssid, req->cmd_set_config->bssid.data, - req->cmd_set_config->bssid.len); - req_data.channel = req->cmd_set_config->channel; - if (h->set_config_handler(&req_data, &h->ctx) == ESP_OK) { - resp_payload->status = STATUS__Success; + + /* Check arguments provided in protobuf packet: + * - SSID / Passphrase string length must be within the standard limits + * - BSSID must either be NULL or have length equal to that imposed by the standard + * If any of these conditions are not satisfied, don't invoke the handler and + * send error status without closing connection */ + resp_payload->status = STATUS__InvalidArgument; + if (req->cmd_set_config->bssid.len != 0 && + req->cmd_set_config->bssid.len != sizeof(req_data.bssid)) { + ESP_LOGD(TAG, "Received invalid BSSID"); + } else if (req->cmd_set_config->ssid.len >= sizeof(req_data.ssid)) { + ESP_LOGD(TAG, "Received invalid SSID"); + } else if (req->cmd_set_config->passphrase.len >= sizeof(req_data.password)) { + ESP_LOGD(TAG, "Received invalid Passphrase"); + } else { + /* The received SSID and Passphrase are not NULL terminated so + * we memcpy over zeroed out arrays. Above length checks ensure + * that there is atleast 1 extra byte for null termination */ + memcpy(req_data.ssid, req->cmd_set_config->ssid.data, + req->cmd_set_config->ssid.len); + memcpy(req_data.password, req->cmd_set_config->passphrase.data, + req->cmd_set_config->passphrase.len); + memcpy(req_data.bssid, req->cmd_set_config->bssid.data, + req->cmd_set_config->bssid.len); + req_data.channel = req->cmd_set_config->channel; + if (h->set_config_handler(&req_data, &h->ctx) == ESP_OK) { + resp_payload->status = STATUS__Success; + } else { + resp_payload->status = STATUS__InternalError; + } } resp->payload_case = WI_FI_CONFIG_PAYLOAD__PAYLOAD_RESP_SET_CONFIG; @@ -188,7 +209,7 @@ static esp_err_t cmd_apply_config_handler(WiFiConfigPayload *req, if (h->apply_config_handler(&h->ctx) == ESP_OK) { resp_payload->status = STATUS__Success; } else { - resp_payload->status = STATUS__InvalidArgument; + resp_payload->status = STATUS__InternalError; } resp->payload_case = WI_FI_CONFIG_PAYLOAD__PAYLOAD_RESP_APPLY_CONFIG; From accef886a935c9cc4c661d7055a08f2cffe4bafd Mon Sep 17 00:00:00 2001 From: Anurag Kar Date: Mon, 10 Jun 2019 17:23:53 +0530 Subject: [PATCH 2/4] Provisioning Examples : Bugfix in copying Wi-Fi SSID and Passphrase --- .../provisioning/ble_prov/main/app_prov_handlers.c | 12 ++++++++---- .../console_prov/main/app_prov_handlers.c | 12 ++++++++---- examples/provisioning/custom_config/main/app_prov.c | 4 ++-- .../custom_config/main/app_prov_handlers.c | 12 ++++++++---- examples/provisioning/softap_prov/main/app_prov.c | 4 ++-- .../softap_prov/main/app_prov_handlers.c | 12 ++++++++---- 6 files changed, 36 insertions(+), 20 deletions(-) diff --git a/examples/provisioning/ble_prov/main/app_prov_handlers.c b/examples/provisioning/ble_prov/main/app_prov_handlers.c index 4a0c0d99a..05dc895dc 100644 --- a/examples/provisioning/ble_prov/main/app_prov_handlers.c +++ b/examples/provisioning/ble_prov/main/app_prov_handlers.c @@ -98,10 +98,14 @@ 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); - memcpy((char *) wifi_cfg->sta.ssid, req_data->ssid, - strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid))); - memcpy((char *) wifi_cfg->sta.password, req_data->password, - strnlen(req_data->password, sizeof(wifi_cfg->sta.password))); + + /* Using strncpy 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)); + 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 49c29739f..e3fa61496 100644 --- a/examples/provisioning/console_prov/main/app_prov_handlers.c +++ b/examples/provisioning/console_prov/main/app_prov_handlers.c @@ -98,10 +98,14 @@ 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); - memcpy((char *) wifi_cfg->sta.ssid, req_data->ssid, - strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid))); - memcpy((char *) wifi_cfg->sta.password, req_data->password, - strnlen(req_data->password, sizeof(wifi_cfg->sta.password))); + + /* Using strncpy 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)); + 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.c b/examples/provisioning/custom_config/main/app_prov.c index 6ed3df19a..1bd773386 100644 --- a/examples/provisioning/custom_config/main/app_prov.c +++ b/examples/provisioning/custom_config/main/app_prov.c @@ -327,13 +327,13 @@ 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 = strlen(ssid); + wifi_config.ap.ssid_len = strnlen(ssid, sizeof(wifi_config.ap.ssid)); if (strlen(pass) == 0) { memset(wifi_config.ap.password, 0, sizeof(wifi_config.ap.password)); wifi_config.ap.authmode = WIFI_AUTH_OPEN; } else { - strncpy((char *) wifi_config.ap.password, pass, sizeof(wifi_config.ap.password)); + strlcpy((char *) wifi_config.ap.password, pass, sizeof(wifi_config.ap.password)); wifi_config.ap.authmode = WIFI_AUTH_WPA_WPA2_PSK; } diff --git a/examples/provisioning/custom_config/main/app_prov_handlers.c b/examples/provisioning/custom_config/main/app_prov_handlers.c index c67eeebca..2f320890e 100644 --- a/examples/provisioning/custom_config/main/app_prov_handlers.c +++ b/examples/provisioning/custom_config/main/app_prov_handlers.c @@ -110,10 +110,14 @@ 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); - memcpy((char *) wifi_cfg->sta.ssid, req_data->ssid, - strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid))); - memcpy((char *) wifi_cfg->sta.password, req_data->password, - strnlen(req_data->password, sizeof(wifi_cfg->sta.password))); + + /* Using strncpy 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)); + 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.c b/examples/provisioning/softap_prov/main/app_prov.c index 79c1e0da0..23e030415 100644 --- a/examples/provisioning/softap_prov/main/app_prov.c +++ b/examples/provisioning/softap_prov/main/app_prov.c @@ -314,13 +314,13 @@ 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 = strlen(ssid); + wifi_config.ap.ssid_len = strnlen(ssid, sizeof(wifi_config.ap.ssid)); if (strlen(pass) == 0) { memset(wifi_config.ap.password, 0, sizeof(wifi_config.ap.password)); wifi_config.ap.authmode = WIFI_AUTH_OPEN; } else { - strncpy((char *) wifi_config.ap.password, pass, sizeof(wifi_config.ap.password)); + strlcpy((char *) wifi_config.ap.password, pass, sizeof(wifi_config.ap.password)); wifi_config.ap.authmode = WIFI_AUTH_WPA_WPA2_PSK; } diff --git a/examples/provisioning/softap_prov/main/app_prov_handlers.c b/examples/provisioning/softap_prov/main/app_prov_handlers.c index 4a0c0d99a..05dc895dc 100644 --- a/examples/provisioning/softap_prov/main/app_prov_handlers.c +++ b/examples/provisioning/softap_prov/main/app_prov_handlers.c @@ -98,10 +98,14 @@ 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); - memcpy((char *) wifi_cfg->sta.ssid, req_data->ssid, - strnlen(req_data->ssid, sizeof(wifi_cfg->sta.ssid))); - memcpy((char *) wifi_cfg->sta.password, req_data->password, - strnlen(req_data->password, sizeof(wifi_cfg->sta.password))); + + /* Using strncpy 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)); + strlcpy((char *) wifi_cfg->sta.password, req_data->password, sizeof(wifi_cfg->sta.password)); return ESP_OK; } From dab432d7ffc7414e85b8860a9c920d80dba98a4a Mon Sep 17 00:00:00 2001 From: Anurag Kar Date: Tue, 28 May 2019 14:41:49 +0530 Subject: [PATCH 3/4] Protocomm : Minor fixes List of changes: * protocomm_httpd : Reset session_id static variable on start and stop * security1 : Typo in checking failed dynamic allocation --- components/protocomm/src/security/security1.c | 2 +- components/protocomm/src/transports/protocomm_httpd.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/components/protocomm/src/security/security1.c b/components/protocomm/src/security/security1.c index 36d99f0a2..56a5e8f31 100644 --- a/components/protocomm/src/security/security1.c +++ b/components/protocomm/src/security/security1.c @@ -198,7 +198,7 @@ static esp_err_t handle_session_command0(uint32_t session_id, mbedtls_ecdh_context *ctx_server = malloc(sizeof(mbedtls_ecdh_context)); mbedtls_entropy_context *entropy = malloc(sizeof(mbedtls_entropy_context)); mbedtls_ctr_drbg_context *ctr_drbg = malloc(sizeof(mbedtls_ctr_drbg_context)); - if (!ctx_server || !ctx_server || !ctr_drbg) { + if (!ctx_server || !entropy || !ctr_drbg) { ESP_LOGE(TAG, "Failed to allocate memory for mbedtls context"); free(ctx_server); free(entropy); diff --git a/components/protocomm/src/transports/protocomm_httpd.c b/components/protocomm/src/transports/protocomm_httpd.c index b4653b9a5..1a20c4442 100644 --- a/components/protocomm/src/transports/protocomm_httpd.c +++ b/components/protocomm/src/transports/protocomm_httpd.c @@ -50,9 +50,7 @@ static esp_err_t common_post_handler(httpd_req_t *req) if (pc_httpd->sec && pc_httpd->sec->close_transport_session) { ret = pc_httpd->sec->close_transport_session(session_id); if (ret != ESP_OK) { - ESP_LOGE(TAG, "Failed to close session with ID: %d", session_id); - ret = ESP_FAIL; - goto out; + ESP_LOGW(TAG, "Error closing session with ID: %d", session_id); } } session_id = PROTOCOMM_NO_SESSION_ID; @@ -241,6 +239,7 @@ esp_err_t protocomm_httpd_start(protocomm_t *pc, const protocomm_httpd_config_t pc->add_endpoint = protocomm_httpd_add_endpoint; pc->remove_endpoint = protocomm_httpd_remove_endpoint; pc_httpd = pc; + session_id = PROTOCOMM_NO_SESSION_ID; return ESP_OK; } @@ -256,6 +255,7 @@ esp_err_t protocomm_httpd_stop(protocomm_t *pc) } pc_httpd->priv = NULL; pc_httpd = NULL; + session_id = PROTOCOMM_NO_SESSION_ID; return ESP_OK; } return ESP_ERR_INVALID_ARG; From 57c7a25bb3fe3e9c5b12e21e0cbed69731d1e801 Mon Sep 17 00:00:00 2001 From: Anurag Kar Date: Wed, 19 Jun 2019 13:39:55 +0530 Subject: [PATCH 4/4] protocomm_ble : Bugfix for unbound memcpy on prepare write buffer Closes https://github.com/espressif/esp-idf/issues/3633 --- .../protocomm/src/transports/protocomm_ble.c | 74 +++++++++++-------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/components/protocomm/src/transports/protocomm_ble.c b/components/protocomm/src/transports/protocomm_ble.c index 5e65aa95d..5d97c8086 100644 --- a/components/protocomm/src/transports/protocomm_ble.c +++ b/components/protocomm/src/transports/protocomm_ble.c @@ -131,40 +131,56 @@ static void transport_simple_ble_read(esp_gatts_cb_event_t event, esp_gatt_if_t static esp_err_t prepare_write_event_env(esp_gatt_if_t gatts_if, esp_ble_gatts_cb_param_t *param) { - ESP_LOGD(TAG, "prepare write, handle = %d, value len = %d", - param->write.handle, param->write.len); + ESP_LOGD(TAG, "prepare write, handle = %d, value len = %d, offset = %d", + param->write.handle, param->write.len, param->write.offset); esp_gatt_status_t status = ESP_GATT_OK; - if (prepare_write_env.prepare_buf == NULL) { - prepare_write_env.prepare_buf = (uint8_t *) malloc(PREPARE_BUF_MAX_SIZE * sizeof(uint8_t)); - if (prepare_write_env.prepare_buf == NULL) { - ESP_LOGE(TAG, "%s , failed tp allocate preparebuf", __func__); - status = ESP_GATT_NO_RESOURCES; - } - /* prepare_write_env.prepare_len = 0; */ + + /* Ensure that write data is not larger than max attribute length */ + if (param->write.offset > PREPARE_BUF_MAX_SIZE) { + status = ESP_GATT_INVALID_OFFSET; + } else if ((param->write.offset + param->write.len) > PREPARE_BUF_MAX_SIZE) { + status = ESP_GATT_INVALID_ATTR_LEN; } else { - if (param->write.offset > PREPARE_BUF_MAX_SIZE) { - status = ESP_GATT_INVALID_OFFSET; - } else if ((param->write.offset + param->write.len) > PREPARE_BUF_MAX_SIZE) { - status = ESP_GATT_INVALID_ATTR_LEN; + /* If prepare buffer is not allocated, then allocate it */ + if (prepare_write_env.prepare_buf == NULL) { + prepare_write_env.prepare_len = 0; + prepare_write_env.prepare_buf = (uint8_t *) malloc(PREPARE_BUF_MAX_SIZE * sizeof(uint8_t)); + if (prepare_write_env.prepare_buf == NULL) { + ESP_LOGE(TAG, "%s , failed to allocate prepare buf", __func__); + status = ESP_GATT_NO_RESOURCES; + } } } - memcpy(prepare_write_env.prepare_buf + param->write.offset, - param->write.value, - param->write.len); - prepare_write_env.prepare_len += param->write.len; - prepare_write_env.handle = param->write.handle; + + /* If prepare buffer is allocated copy incoming data into it */ + if (status == ESP_GATT_OK) { + memcpy(prepare_write_env.prepare_buf + param->write.offset, + param->write.value, + param->write.len); + prepare_write_env.prepare_len += param->write.len; + prepare_write_env.handle = param->write.handle; + } + + /* Send write response if needed */ if (param->write.need_rsp) { - esp_gatt_rsp_t gatt_rsp = {0}; - gatt_rsp.attr_value.len = param->write.len; - gatt_rsp.attr_value.handle = param->write.handle; - gatt_rsp.attr_value.offset = param->write.offset; - gatt_rsp.attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE; - if (gatt_rsp.attr_value.len && param->write.value) { - memcpy(gatt_rsp.attr_value.value, param->write.value, param->write.len); + esp_err_t response_err; + /* If data was successfully appended to prepare buffer + * only then have it reflected in the response */ + if (status == ESP_GATT_OK) { + esp_gatt_rsp_t gatt_rsp = {0}; + gatt_rsp.attr_value.len = param->write.len; + gatt_rsp.attr_value.handle = param->write.handle; + gatt_rsp.attr_value.offset = param->write.offset; + gatt_rsp.attr_value.auth_req = ESP_GATT_AUTH_REQ_NONE; + if (gatt_rsp.attr_value.len && param->write.value) { + memcpy(gatt_rsp.attr_value.value, param->write.value, param->write.len); + } + response_err = esp_ble_gatts_send_response(gatts_if, + param->write.conn_id, param->write.trans_id, status, &gatt_rsp); + } else { + response_err = esp_ble_gatts_send_response(gatts_if, + param->write.conn_id, param->write.trans_id, status, NULL); } - esp_err_t response_err = esp_ble_gatts_send_response(gatts_if, param->write.conn_id, - param->write.trans_id, status, - &gatt_rsp); if (response_err != ESP_OK) { ESP_LOGE(TAG, "Send response error in prep write"); } @@ -186,7 +202,7 @@ static void transport_simple_ble_write(esp_gatts_cb_event_t event, esp_gatt_if_t ssize_t outlen = 0; esp_err_t ret; - ESP_LOGD(TAG, "Inside write with session - %d on attr handle - %d \nLen -%d IS Prep - %d", + 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 (param->write.is_prep) {