From 68947b594411f8e8a94047c561a7991fdc5e140f Mon Sep 17 00:00:00 2001 From: Aditya Patwardhan Date: Mon, 3 Oct 2022 09:02:04 +0530 Subject: [PATCH 1/4] protocommm/esp_srp: Fix small issues reported by coverity. --- components/protocomm/src/crypto/srp6a/esp_srp.c | 3 ++- components/protocomm/src/crypto/srp6a/esp_srp_mpi.c | 3 +++ .../protocomm/src/crypto/srp6a/include/esp_srp.h | 2 +- components/protocomm/src/security/security2.c | 12 +++++++++--- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/components/protocomm/src/crypto/srp6a/esp_srp.c b/components/protocomm/src/crypto/srp6a/esp_srp.c index 79159bc41f7..36fbc083422 100644 --- a/components/protocomm/src/crypto/srp6a/esp_srp.c +++ b/components/protocomm/src/crypto/srp6a/esp_srp.c @@ -388,7 +388,7 @@ esp_err_t esp_srp_set_salt_verifier(esp_srp_handle_t *hd, const char *salt, int return ESP_FAIL; } -esp_err_t esp_srp_get_session_key(esp_srp_handle_t *hd, char *bytes_A, int len_A, char **bytes_key, int *len_key) +esp_err_t esp_srp_get_session_key(esp_srp_handle_t *hd, char *bytes_A, int len_A, char **bytes_key, uint16_t *len_key) { esp_mpi_t *u = NULL; esp_mpi_t *vu = NULL; @@ -524,6 +524,7 @@ esp_err_t esp_srp_exchange_proofs(esp_srp_handle_t *hd, char *username, uint16_t ESP_LOG_BUFFER_HEX_LEVEL(TAG, (char *)digest, sizeof(digest), ESP_LOG_DEBUG); if (memcmp(bytes_user_proof, digest, SHA512_HASH_SZ) != 0) { + free(s); return ESP_FAIL; } diff --git a/components/protocomm/src/crypto/srp6a/esp_srp_mpi.c b/components/protocomm/src/crypto/srp6a/esp_srp_mpi.c index c02cbab0007..2cb60e4ee97 100644 --- a/components/protocomm/src/crypto/srp6a/esp_srp_mpi.c +++ b/components/protocomm/src/crypto/srp6a/esp_srp_mpi.c @@ -26,6 +26,7 @@ esp_mpi_t *esp_mpi_new_from_hex(const char *hex) int ret = mbedtls_mpi_read_string(a, 16, hex); if (ret != 0) { printf("mbedtls_mpi_read_string() failed, returned %x\n", ret); + esp_mpi_free(a); return NULL; } return a; @@ -41,6 +42,7 @@ esp_mpi_t *esp_mpi_new_from_bin(const char *str, int str_len) int ret = mbedtls_mpi_read_binary(a, (unsigned char *)str, str_len); if (ret != 0) { printf("mbedtls_mpi_read_binary() failed, returned %x\n", ret); + esp_mpi_free(a); return NULL; } return a; @@ -81,6 +83,7 @@ char *esp_mpi_to_bin(esp_mpi_t *bn, int *len) int ret = mbedtls_mpi_write_binary(bn, (unsigned char *)p, *len); if (ret != 0) { printf("mbedtls_mpi_read_string() failed, returned %x\n", ret); + free(p); return NULL; } return p; diff --git a/components/protocomm/src/crypto/srp6a/include/esp_srp.h b/components/protocomm/src/crypto/srp6a/include/esp_srp.h index 388fab7acbc..972704f038c 100644 --- a/components/protocomm/src/crypto/srp6a/include/esp_srp.h +++ b/components/protocomm/src/crypto/srp6a/include/esp_srp.h @@ -88,7 +88,7 @@ esp_err_t esp_srp_srv_pubkey_from_salt_verifier(esp_srp_handle_t *hd, char **byt /* Returns bytes_key * *bytes_key MUST NOT BE FREED BY THE CALLER */ -esp_err_t esp_srp_get_session_key(esp_srp_handle_t *hd, char *bytes_A, int len_A, char **bytes_key, int *len_key); +esp_err_t esp_srp_get_session_key(esp_srp_handle_t *hd, char *bytes_A, int len_A, char **bytes_key, uint16_t *len_key); /* Exchange proofs * Returns 1 if user's proof is ok. Also 1 when is returned, bytes_host_proof contains our proof. diff --git a/components/protocomm/src/security/security2.c b/components/protocomm/src/security/security2.c index e13d2a7d090..aff063da653 100644 --- a/components/protocomm/src/security/security2.c +++ b/components/protocomm/src/security/security2.c @@ -90,6 +90,11 @@ static esp_err_t handle_session_command0(session_t *cur_session, return ESP_ERR_INVALID_ARG; } + if (sv == NULL) { + ESP_LOGE(TAG, "Invalid security params"); + return ESP_ERR_INVALID_ARG; + } + cur_session->username_len = in->sc0->client_username.len; cur_session->username = calloc(cur_session->username_len, sizeof(char)); if (!cur_session->username) { @@ -129,7 +134,7 @@ static esp_err_t handle_session_command0(session_t *cur_session, cur_session->verifier_len = sv->verifier_len; ESP_LOGI(TAG, "Using salt and verifier to generate public key..."); - if (sv != NULL && sv->salt != NULL && sv->salt_len != 0 && sv->verifier != NULL && sv->verifier_len != 0) { + if (sv->salt != NULL && sv->salt_len != 0 && sv->verifier != NULL && sv->verifier_len != 0) { if (esp_srp_set_salt_verifier(cur_session->srp_hd, cur_session->salt, cur_session->salt_len, cur_session->verifier, cur_session->verifier_len) != ESP_OK) { ESP_LOGE(TAG, "Failed to set salt and verifier!"); return ESP_FAIL; @@ -141,9 +146,8 @@ static esp_err_t handle_session_command0(session_t *cur_session, } hexdump("Device Public Key", device_pubkey, device_pubkey_len); - if (esp_srp_get_session_key(cur_session->srp_hd, cur_session->client_pubkey, cur_session->client_pubkey_len, - &cur_session->session_key, (int *)&cur_session->session_key_len) != ESP_OK) { + &cur_session->session_key, &cur_session->session_key_len) != ESP_OK) { ESP_LOGE(TAG, "Failed to generate device session key!"); return ESP_FAIL; } @@ -225,6 +229,7 @@ static esp_err_t handle_session_command1(session_t *cur_session, mbed_err = mbedtls_gcm_setkey(&cur_session->ctx_gcm, MBEDTLS_CIPHER_ID_AES, (unsigned char *)cur_session->session_key, AES_GCM_KEY_LEN); if (mbed_err != 0) { ESP_LOGE(TAG, "Failure at mbedtls_gcm_setkey_enc with error code : -0x%x", -mbed_err); + free(device_proof); mbedtls_gcm_free(&cur_session->ctx_gcm); return ESP_FAIL; } @@ -233,6 +238,7 @@ static esp_err_t handle_session_command1(session_t *cur_session, S2SessionResp1 *out_resp = (S2SessionResp1 *) malloc(sizeof(S2SessionResp1)); if (!out || !out_resp) { ESP_LOGE(TAG, "Error allocating memory for response1"); + free(device_proof); free(out); free(out_resp); mbedtls_gcm_free(&cur_session->ctx_gcm); From acc3dc8bd2b8843740eb2d730da17e49f324dde8 Mon Sep 17 00:00:00 2001 From: Aditya Patwardhan Date: Mon, 3 Oct 2022 10:50:02 +0530 Subject: [PATCH 2/4] protocomm/esp_srp: Allocate memory for username only when the verification is successful --- components/protocomm/src/security/security2.c | 41 +++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/components/protocomm/src/security/security2.c b/components/protocomm/src/security/security2.c index aff063da653..19e33f8ee2a 100644 --- a/components/protocomm/src/security/security2.c +++ b/components/protocomm/src/security/security2.c @@ -48,8 +48,6 @@ typedef struct session { uint16_t salt_len; char *verifier; uint16_t verifier_len; - char *client_pubkey; - uint16_t client_pubkey_len; char *session_key; uint16_t session_key_len; uint8_t iv[AES_GCM_IV_SIZE]; @@ -95,23 +93,11 @@ static esp_err_t handle_session_command0(session_t *cur_session, return ESP_ERR_INVALID_ARG; } - cur_session->username_len = in->sc0->client_username.len; - cur_session->username = calloc(cur_session->username_len, sizeof(char)); - if (!cur_session->username) { - ESP_LOGE(TAG, "Failed to allocate memory!"); - return ESP_ERR_NO_MEM; - } - memcpy(cur_session->username, in->sc0->client_username.data, in->sc0->client_username.len); - ESP_LOGD(TAG, "Username: %.*s", cur_session->username_len, cur_session->username); - cur_session->client_pubkey = calloc(PUBLIC_KEY_LEN, sizeof(char)); - if (!cur_session->client_pubkey ) { - ESP_LOGE(TAG, "Failed to allocate memory!"); - return ESP_ERR_NO_MEM; - } - memcpy(cur_session->client_pubkey, in->sc0->client_pubkey.data, PUBLIC_KEY_LEN); - cur_session->client_pubkey_len = PUBLIC_KEY_LEN; - hexdump("Client Public Key", cur_session->client_pubkey, PUBLIC_KEY_LEN); + ESP_LOGD(TAG, "Username: %.*s", in->sc0->client_username.len, in->sc0->client_username.data); + + + hexdump("Client Public Key", (char *) in->sc0->client_pubkey.data, PUBLIC_KEY_LEN); /* Initialize mu srp context */ cur_session->srp_hd = calloc(1, sizeof(esp_srp_handle_t)); @@ -122,6 +108,7 @@ static esp_err_t handle_session_command0(session_t *cur_session, if (esp_srp_init(cur_session->srp_hd, ESP_NG_3072) != ESP_OK) { ESP_LOGE(TAG, "Failed to initialise security context!"); + free(cur_session->srp_hd); return ESP_FAIL; } @@ -137,27 +124,30 @@ static esp_err_t handle_session_command0(session_t *cur_session, if (sv->salt != NULL && sv->salt_len != 0 && sv->verifier != NULL && sv->verifier_len != 0) { if (esp_srp_set_salt_verifier(cur_session->srp_hd, cur_session->salt, cur_session->salt_len, cur_session->verifier, cur_session->verifier_len) != ESP_OK) { ESP_LOGE(TAG, "Failed to set salt and verifier!"); + free(cur_session->srp_hd); return ESP_FAIL; } if (esp_srp_srv_pubkey_from_salt_verifier(cur_session->srp_hd, &device_pubkey, &device_pubkey_len) != ESP_OK) { ESP_LOGE(TAG, "Failed to device public key!"); + free(cur_session->srp_hd); return ESP_FAIL; } } hexdump("Device Public Key", device_pubkey, device_pubkey_len); - if (esp_srp_get_session_key(cur_session->srp_hd, cur_session->client_pubkey, cur_session->client_pubkey_len, + if (esp_srp_get_session_key(cur_session->srp_hd, (char *) in->sc0->client_pubkey.data, PUBLIC_KEY_LEN, &cur_session->session_key, &cur_session->session_key_len) != ESP_OK) { ESP_LOGE(TAG, "Failed to generate device session key!"); + free(cur_session->srp_hd); return ESP_FAIL; } hexdump("Session Key", cur_session->session_key, cur_session->session_key_len); - Sec2Payload *out = (Sec2Payload *) malloc(sizeof(Sec2Payload)); S2SessionResp0 *out_resp = (S2SessionResp0 *) malloc(sizeof(S2SessionResp0)); if (!out || !out_resp) { ESP_LOGE(TAG, "Error allocating memory for response0"); + free(cur_session->srp_hd); free(out); free(out_resp); return ESP_ERR_NO_MEM; @@ -182,6 +172,15 @@ static esp_err_t handle_session_command0(session_t *cur_session, resp->proto_case = SESSION_DATA__PROTO_SEC2; resp->sec2 = out; + cur_session->username_len = in->sc0->client_username.len; + cur_session->username = malloc(cur_session->username_len); + if (!cur_session->username) { + ESP_LOGE(TAG, "Failed to allocate memory!"); + free(cur_session->srp_hd); + return ESP_ERR_NO_MEM; + } + memcpy(cur_session->username, in->sc0->client_username.data, in->sc0->client_username.len); + cur_session->state = SESSION_STATE_CMD1; ESP_LOGD(TAG, "Session setup phase1 done"); @@ -213,6 +212,7 @@ static esp_err_t handle_session_command1(session_t *cur_session, if (esp_srp_exchange_proofs(cur_session->srp_hd, cur_session->username, cur_session->username_len, (char * ) in->sc1->client_proof.data, device_proof) != ESP_OK) { ESP_LOGE(TAG, "Failed to authenticate client proof!"); + free(device_proof); return ESP_FAIL; } hexdump("Device proof", device_proof, CLIENT_PROOF_LEN); @@ -347,7 +347,6 @@ static esp_err_t sec2_close_session(protocomm_security_handle_t handle, uint32_t } free(cur_session->username); - free(cur_session->client_pubkey); if (cur_session->srp_hd) { esp_srp_free(cur_session->srp_hd); From 7cb55e6f0b638fb8da06a3f74f47e53ff0ca046a Mon Sep 17 00:00:00 2001 From: Aditya Patwardhan Date: Fri, 7 Oct 2022 14:51:01 +0530 Subject: [PATCH 3/4] pytest_wifi_prov_mgr.py: Update example test to enable sec1 and sec2 testing --- .../wifi_prov_mgr/pytest_wifi_prov_mgr.py | 37 +++++++++++++++---- .../wifi_prov_mgr/sdkconfig.ci.security1 | 1 + 2 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 examples/provisioning/wifi_prov_mgr/sdkconfig.ci.security1 diff --git a/examples/provisioning/wifi_prov_mgr/pytest_wifi_prov_mgr.py b/examples/provisioning/wifi_prov_mgr/pytest_wifi_prov_mgr.py index cea11131c20..45704df07f6 100644 --- a/examples/provisioning/wifi_prov_mgr/pytest_wifi_prov_mgr.py +++ b/examples/provisioning/wifi_prov_mgr/pytest_wifi_prov_mgr.py @@ -25,11 +25,7 @@ esp_prov.config_throw_except = True -@pytest.mark.esp32 -@pytest.mark.generic -@pytest.mark.xfail(reason='Runner unable to connect to target over Bluetooth', run=False) -def test_examples_wifi_prov_mgr(dut: Dut) -> None: - +def test_wifi_prov_mgr(dut: Dut, sec_ver: int) -> None: # Check if BT memory is released before provisioning starts dut.expect('wifi_prov_scheme_ble: BT memory released', timeout=60) @@ -40,14 +36,22 @@ def test_examples_wifi_prov_mgr(dut: Dut) -> None: logging.info('Starting Provisioning') verbose = False protover = 'v1.1' - secver = 1 - pop = 'abcd1234' provmode = 'ble' ap_ssid = 'myssid' ap_password = 'mypassword' logging.info('Getting security') - security = esp_prov.get_security(secver, pop, verbose) + if (sec_ver == 1): + pop = 'abcd1234' + sec2_username = None + sec2_password = None + security = esp_prov.get_security(sec_ver, sec2_username, sec2_password, pop, verbose) + elif (sec_ver == 2): + pop = None + sec2_username = 'wifiprov' + sec2_password = 'abcd1234' + security = esp_prov.get_security(sec_ver, sec2_username, sec2_password, pop, verbose) + if security is None: raise RuntimeError('Failed to get security') @@ -85,3 +89,20 @@ def test_examples_wifi_prov_mgr(dut: Dut) -> None: # Check if BTDM memory is released after provisioning finishes dut.expect('wifi_prov_scheme_ble: BTDM memory released', timeout=30) + + +@pytest.mark.esp32 +@pytest.mark.generic +@pytest.mark.parametrize('config', ['security1',], indirect=True) +@pytest.mark.xfail(reason='Runner unable to connect to target over Bluetooth', run=False) +def test_examples_wifi_prov_mgr_sec1(dut: Dut) -> None: + + test_wifi_prov_mgr(dut, 1) + + +@pytest.mark.esp32 +@pytest.mark.generic +@pytest.mark.xfail(reason='Runner unable to connect to target over Bluetooth', run=False) +def test_examples_wifi_prov_mgr_sec2(dut: Dut) -> None: + + test_wifi_prov_mgr(dut, 2) diff --git a/examples/provisioning/wifi_prov_mgr/sdkconfig.ci.security1 b/examples/provisioning/wifi_prov_mgr/sdkconfig.ci.security1 new file mode 100644 index 00000000000..760cddbe95f --- /dev/null +++ b/examples/provisioning/wifi_prov_mgr/sdkconfig.ci.security1 @@ -0,0 +1 @@ +CONFIG_EXAMPLE_PROV_SECURITY_VERSION_1=y From 6328afdce66237cf43d8cc7a3769b4f8d45abdc8 Mon Sep 17 00:00:00 2001 From: Aditya Patwardhan Date: Fri, 7 Oct 2022 18:25:47 +0530 Subject: [PATCH 4/4] wifi_provisioning/manager.c: Fix small bug introduced in recent MR. --- components/wifi_provisioning/src/manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/wifi_provisioning/src/manager.c b/components/wifi_provisioning/src/manager.c index 343c9865dfa..27adb38141b 100644 --- a/components/wifi_provisioning/src/manager.c +++ b/components/wifi_provisioning/src/manager.c @@ -1485,7 +1485,7 @@ esp_err_t wifi_prov_mgr_start_provisioning(wifi_prov_security_t security, const /* Initialize app data */ if (security == WIFI_PROV_SECURITY_0) { prov_ctx->mgr_info.capabilities.no_sec = true; - } else + } #endif #ifdef CONFIG_ESP_PROTOCOMM_SUPPORT_SECURITY_VERSION_1 if (security == WIFI_PROV_SECURITY_1) { @@ -1504,7 +1504,7 @@ esp_err_t wifi_prov_mgr_start_provisioning(wifi_prov_security_t security, const } else { prov_ctx->mgr_info.capabilities.no_pop = true; } - } else + } #endif #ifdef CONFIG_ESP_PROTOCOMM_SUPPORT_SECURITY_VERSION_2 if (security == WIFI_PROV_SECURITY_2) {