diff --git a/components/protocomm/src/common/protocomm.c b/components/protocomm/src/common/protocomm.c index ff4901a0b..b785d8e99 100644 --- a/components/protocomm/src/common/protocomm.c +++ b/components/protocomm/src/common/protocomm.c @@ -52,6 +52,19 @@ void protocomm_delete(protocomm_t *pc) free(it); } + /* Free memory allocated to version string */ + if (pc->ver) { + free((void *)pc->ver); + } + + /* Free memory allocated to security */ + if (pc->sec && pc->sec->cleanup) { + pc->sec->cleanup(); + } + if (pc->pop) { + free(pc->pop); + } + free(pc); } @@ -140,11 +153,14 @@ esp_err_t protocomm_req_handle(protocomm_t *pc, const char *ep_name, uint32_t se const uint8_t *inbuf, ssize_t inlen, uint8_t **outbuf, ssize_t *outlen) { - if ((pc == NULL) || (ep_name == NULL)) { + if (!pc || !ep_name || !outbuf || !outlen) { ESP_LOGE(TAG, "Invalid params %p %p", pc, ep_name); return ESP_ERR_INVALID_ARG; } + *outbuf = NULL; + *outlen = 0; + protocomm_ep_t *ep = search_endpoint(pc, ep_name); if (!ep) { ESP_LOGE(TAG, "No registered endpoint for %s", ep_name); @@ -166,19 +182,23 @@ esp_err_t protocomm_req_handle(protocomm_t *pc, const char *ep_name, uint32_t se } ssize_t dec_inbuf_len = inlen; - pc->sec->decrypt(session_id, inbuf, inlen, dec_inbuf, &dec_inbuf_len); + ret = pc->sec->decrypt(session_id, inbuf, inlen, dec_inbuf, &dec_inbuf_len); + if (ret != ESP_OK) { + ESP_LOGE(TAG, "Decryption of response failed for endpoint %s", ep_name); + free(dec_inbuf); + return ret; + } /* Invoke the request handler */ - uint8_t *plaintext_resp; - ssize_t plaintext_resp_len; + uint8_t *plaintext_resp = NULL; + ssize_t plaintext_resp_len = 0; ret = ep->req_handler(session_id, dec_inbuf, dec_inbuf_len, &plaintext_resp, &plaintext_resp_len, ep->priv_data); if (ret != ESP_OK) { ESP_LOGE(TAG, "Request handler for %s failed", ep_name); - *outbuf = NULL; - *outlen = 0; + free(plaintext_resp); free(dec_inbuf); return ret; } @@ -189,12 +209,20 @@ esp_err_t protocomm_req_handle(protocomm_t *pc, const char *ep_name, uint32_t se uint8_t *enc_resp = (uint8_t *) malloc(plaintext_resp_len); if (!enc_resp) { ESP_LOGE(TAG, "Failed to allocate decrypt buf len %d", inlen); + free(plaintext_resp); return ESP_ERR_NO_MEM; } ssize_t enc_resp_len = plaintext_resp_len; - pc->sec->encrypt(session_id, plaintext_resp, plaintext_resp_len, - enc_resp, &enc_resp_len); + ret = pc->sec->encrypt(session_id, plaintext_resp, plaintext_resp_len, + enc_resp, &enc_resp_len); + + if (ret != ESP_OK) { + ESP_LOGE(TAG, "Encryption of response failed for endpoint %s", ep_name); + free(enc_resp); + free(plaintext_resp); + return ret; + } /* We no more need plaintext response */ free(plaintext_resp); diff --git a/components/protocomm/src/security/security0.c b/components/protocomm/src/security/security0.c index bca4d18c3..a127136a3 100644 --- a/components/protocomm/src/security/security0.c +++ b/components/protocomm/src/security/security0.c @@ -36,6 +36,8 @@ static esp_err_t sec0_session_setup(uint32_t session_id, S0SessionResp *s0resp = (S0SessionResp *) malloc(sizeof(S0SessionResp)); if (!out || !s0resp) { ESP_LOGE(TAG, "Error allocating response"); + free(out); + free(s0resp); return ESP_ERR_NO_MEM; } sec0_payload__init(out); @@ -79,6 +81,7 @@ static esp_err_t sec0_req_handler(const protocomm_security_pop_t *pop, uint32_t } if (req->sec_ver != protocomm_security0.ver) { ESP_LOGE(TAG, "Security version mismatch. Closing connection"); + session_data__free_unpacked(req, NULL); return ESP_ERR_INVALID_ARG; } @@ -86,12 +89,12 @@ static esp_err_t sec0_req_handler(const protocomm_security_pop_t *pop, uint32_t ret = sec0_session_setup(session_id, req, &resp, pop); if (ret != ESP_OK) { ESP_LOGE(TAG, "Session setup error %d", ret); + session_data__free_unpacked(req, NULL); return ESP_FAIL; } - session_data__free_unpacked(req, NULL); - resp.sec_ver = req->sec_ver; + session_data__free_unpacked(req, NULL); *outlen = session_data__get_packed_size(&resp); *outbuf = (uint8_t *) malloc(*outlen); diff --git a/components/protocomm/src/security/security1.c b/components/protocomm/src/security/security1.c index 76b9a25f5..36d99f0a2 100644 --- a/components/protocomm/src/security/security1.c +++ b/components/protocomm/src/security/security1.c @@ -38,8 +38,9 @@ static const char* TAG = "security1"; #define PUBLIC_KEY_LEN 32 #define SZ_RANDOM 16 -#define SESSION_STATE_1 1 /* Session in state 1 */ -#define SESSION_STATE_SETUP 2 /* Session setup successful */ +#define SESSION_STATE_CMD0 0 /* Session is not setup */ +#define SESSION_STATE_CMD1 1 /* Session is not setup */ +#define SESSION_STATE_DONE 2 /* Session setup successful */ typedef struct session { /* Session data */ @@ -82,22 +83,12 @@ static esp_err_t handle_session_command1(uint32_t session_id, uint8_t check_buf[PUBLIC_KEY_LEN]; int mbed_err; - if (!cur_session) { - ESP_LOGE(TAG, "Data on session endpoint without session establishment"); + if (cur_session->state != SESSION_STATE_CMD1) { + ESP_LOGE(TAG, "Invalid state of session %d (expected %d)", SESSION_STATE_CMD1, cur_session->state); return ESP_ERR_INVALID_STATE; } - if (session_id != cur_session->id) { - ESP_LOGE(TAG, "Invalid session"); - return ESP_ERR_INVALID_STATE; - } - - if (!in) { - ESP_LOGE(TAG, "Empty session data"); - return ESP_ERR_INVALID_ARG; - } - - /* Initialise crypto context */ + /* Initialize crypto context */ mbedtls_aes_init(&cur_session->ctx_aes); memset(cur_session->stb, 0, sizeof(cur_session->stb)); cur_session->nc_off = 0; @@ -109,6 +100,7 @@ static esp_err_t handle_session_command1(uint32_t session_id, sizeof(cur_session->sym_key)*8); if (mbed_err != 0) { ESP_LOGE(TAG, "Failure at mbedtls_aes_setkey_enc with error code : -0x%x", -mbed_err); + mbedtls_aes_free(&cur_session->ctx_aes); return ESP_FAIL; } @@ -118,6 +110,7 @@ static esp_err_t handle_session_command1(uint32_t session_id, in->sc1->client_verify_data.data, check_buf); if (mbed_err != 0) { ESP_LOGE(TAG, "Failure at mbedtls_aes_crypt_ctr with error code : -0x%x", -mbed_err); + mbedtls_aes_free(&cur_session->ctx_aes); return ESP_FAIL; } @@ -127,19 +120,30 @@ static esp_err_t handle_session_command1(uint32_t session_id, if (mbedtls_ssl_safer_memcmp(check_buf, cur_session->device_pubkey, sizeof(cur_session->device_pubkey)) != 0) { ESP_LOGE(TAG, "Key mismatch. Close connection"); + mbedtls_aes_free(&cur_session->ctx_aes); return ESP_FAIL; } Sec1Payload *out = (Sec1Payload *) malloc(sizeof(Sec1Payload)); - sec1_payload__init(out); SessionResp1 *out_resp = (SessionResp1 *) malloc(sizeof(SessionResp1)); - session_resp1__init(out_resp); + if (!out || !out_resp) { + ESP_LOGE(TAG, "Error allocating memory for response1"); + free(out); + free(out_resp); + mbedtls_aes_free(&cur_session->ctx_aes); + return ESP_ERR_NO_MEM; + } + sec1_payload__init(out); + session_resp1__init(out_resp); out_resp->status = STATUS__Success; uint8_t *outbuf = (uint8_t *) malloc(PUBLIC_KEY_LEN); if (!outbuf) { ESP_LOGE(TAG, "Error allocating ciphertext buffer"); + free(out); + free(out_resp); + mbedtls_aes_free(&cur_session->ctx_aes); return ESP_ERR_NO_MEM; } @@ -149,6 +153,10 @@ static esp_err_t handle_session_command1(uint32_t session_id, cur_session->client_pubkey, outbuf); if (mbed_err != 0) { ESP_LOGE(TAG, "Failure at mbedtls_aes_crypt_ctr with error code : -0x%x", -mbed_err); + free(outbuf); + free(out); + free(out_resp); + mbedtls_aes_free(&cur_session->ctx_aes); return ESP_FAIL; } @@ -163,8 +171,8 @@ static esp_err_t handle_session_command1(uint32_t session_id, resp->proto_case = SESSION_DATA__PROTO_SEC1; resp->sec1 = out; - ESP_LOGD(TAG, "Session successful"); - + cur_session->state = SESSION_STATE_DONE; + ESP_LOGD(TAG, "Secure session established successfully"); return ESP_OK; } @@ -172,32 +180,21 @@ static esp_err_t handle_session_command0(uint32_t session_id, SessionData *req, SessionData *resp, const protocomm_security_pop_t *pop) { + ESP_LOGD(TAG, "Request to handle setup0_command"); Sec1Payload *in = (Sec1Payload *) req->sec1; esp_err_t ret; int mbed_err; - if (!cur_session) { - ESP_LOGE(TAG, "Data on session endpoint without session establishment"); + if (cur_session->state != SESSION_STATE_CMD0) { + ESP_LOGE(TAG, "Invalid state of session %d (expected %d)", SESSION_STATE_CMD0, cur_session->state); return ESP_ERR_INVALID_STATE; } - if (session_id != cur_session->id) { - ESP_LOGE(TAG, "Invalid session"); - return ESP_ERR_INVALID_STATE; - } - - if (!in) { - ESP_LOGE(TAG, "Empty session data"); - return ESP_ERR_INVALID_ARG; - } - if (in->sc0->client_pubkey.len != PUBLIC_KEY_LEN) { ESP_LOGE(TAG, "Invalid public key length"); return ESP_ERR_INVALID_ARG; } - cur_session->state = SESSION_STATE_1; - 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)); @@ -315,8 +312,10 @@ static esp_err_t handle_session_command0(uint32_t session_id, Sec1Payload *out = (Sec1Payload *) malloc(sizeof(Sec1Payload)); SessionResp0 *out_resp = (SessionResp0 *) malloc(sizeof(SessionResp0)); if (!out || !out_resp) { - ESP_LOGE(TAG, "Error allocating memory for response"); - ret = ESP_FAIL; + ESP_LOGE(TAG, "Error allocating memory for response0"); + ret = ESP_ERR_NO_MEM; + free(out); + free(out_resp); goto exit_cmd0; } @@ -338,6 +337,8 @@ static esp_err_t handle_session_command0(uint32_t session_id, resp->proto_case = SESSION_DATA__PROTO_SEC1; resp->sec1 = out; + cur_session->state = SESSION_STATE_CMD1; + ESP_LOGD(TAG, "Session setup phase1 done"); ret = ESP_OK; @@ -361,6 +362,21 @@ static esp_err_t sec1_session_setup(uint32_t session_id, Sec1Payload *in = (Sec1Payload *) req->sec1; esp_err_t ret; + if (!cur_session) { + ESP_LOGE(TAG, "Invalid session context data"); + return ESP_ERR_INVALID_ARG; + } + + if (session_id != cur_session->id) { + ESP_LOGE(TAG, "Invalid session ID : %d (expected %d)", session_id, cur_session->id); + return ESP_ERR_INVALID_STATE; + } + + if (!in) { + ESP_LOGE(TAG, "Empty session data"); + return ESP_ERR_INVALID_ARG; + } + switch (in->msg) { case SEC1_MSG_TYPE__Session_Command0: ret = handle_session_command0(session_id, req, resp, pop); @@ -411,30 +427,30 @@ static void sec1_session_setup_cleanup(uint32_t session_id, SessionData *resp) return; } -static esp_err_t sec1_init() +static esp_err_t sec1_close_session(uint32_t session_id) { - return ESP_OK; -} - -static esp_err_t sec1_cleanup() -{ - if (cur_session) { - ESP_LOGD(TAG, "Closing current session with id %u", cur_session->id); - mbedtls_aes_free(&cur_session->ctx_aes); - bzero(cur_session, sizeof(session_t)); - free(cur_session); - cur_session = NULL; + if (!cur_session || cur_session->id != session_id) { + ESP_LOGE(TAG, "Attempt to close invalid session"); + return ESP_ERR_INVALID_ARG; } + + if (cur_session->state == SESSION_STATE_DONE) { + /* Free AES context data */ + mbedtls_aes_free(&cur_session->ctx_aes); + } + + bzero(cur_session, sizeof(session_t)); + free(cur_session); + cur_session = NULL; return ESP_OK; } static esp_err_t sec1_new_session(uint32_t session_id) { - if (cur_session && cur_session->id != session_id) { + if (cur_session) { + /* Only one session is allowed at a time */ ESP_LOGE(TAG, "Closing old session with id %u", cur_session->id); - sec1_cleanup(); - } else if (cur_session && cur_session->id == session_id) { - return ESP_OK; + sec1_close_session(cur_session->id); } cur_session = (session_t *) calloc(1, sizeof(session_t)); @@ -447,16 +463,17 @@ static esp_err_t sec1_new_session(uint32_t session_id) return ESP_OK; } -static esp_err_t sec1_close_session(uint32_t session_id) +static esp_err_t sec1_init() { - if (!cur_session || cur_session->id != session_id) { - ESP_LOGE(TAG, "Attempt to close invalid session"); - return ESP_ERR_INVALID_ARG; - } + return ESP_OK; +} - bzero(cur_session, sizeof(session_t)); - free(cur_session); - cur_session = NULL; +static esp_err_t sec1_cleanup() +{ + if (cur_session) { + ESP_LOGD(TAG, "Closing current session with id %u", cur_session->id); + sec1_close_session(cur_session->id); + } return ESP_OK; } @@ -469,8 +486,15 @@ static esp_err_t sec1_decrypt(uint32_t session_id, } if (!cur_session || cur_session->id != session_id) { + ESP_LOGE(TAG, "Session with ID %d not found", session_id); return ESP_ERR_INVALID_STATE; } + + if (cur_session->state != SESSION_STATE_DONE) { + ESP_LOGE(TAG, "Secure session not established"); + return ESP_ERR_INVALID_STATE; + } + *outlen = inlen; int ret = mbedtls_aes_crypt_ctr(&cur_session->ctx_aes, inlen, &cur_session->nc_off, cur_session->rand, cur_session->stb, inbuf, outbuf); @@ -497,6 +521,7 @@ static esp_err_t sec1_req_handler(const protocomm_security_pop_t *pop, uint32_t } if (req->sec_ver != protocomm_security1.ver) { ESP_LOGE(TAG, "Security version mismatch. Closing connection"); + session_data__free_unpacked(req, NULL); return ESP_ERR_INVALID_ARG; } @@ -504,12 +529,12 @@ static esp_err_t sec1_req_handler(const protocomm_security_pop_t *pop, uint32_t ret = sec1_session_setup(session_id, req, &resp, pop); if (ret != ESP_OK) { ESP_LOGE(TAG, "Session setup error %d", ret); + session_data__free_unpacked(req, NULL); return ESP_FAIL; } - session_data__free_unpacked(req, NULL); - resp.sec_ver = req->sec_ver; + session_data__free_unpacked(req, NULL); *outlen = session_data__get_packed_size(&resp); *outbuf = (uint8_t *) malloc(*outlen); diff --git a/components/protocomm/src/transports/protocomm_console.c b/components/protocomm/src/transports/protocomm_console.c index c7cf8c921..5c268bc49 100644 --- a/components/protocomm/src/transports/protocomm_console.c +++ b/components/protocomm/src/transports/protocomm_console.c @@ -121,10 +121,6 @@ static void protocomm_console_task(void *arg) } } - if (pc_console->sec && pc_console->sec->cleanup) { - pc_console->sec->cleanup(); - } - pc_console = NULL; esp_console_deinit();