protocomm security : memory leaks fixed

List of changes:
* Corner case exceptions are properly handled to ensure release of memory occupied by security infrastructure
* fixed erroneous cleanup of security instance by protocomm_console

diff --git a/components/protocomm/src/common/protocomm.c b/components/protocomm/src/common/protocomm.c
index ff4901a0ba3cef90e498547f77bd26aaf87395fb..b785d8e992c1192ac11b10b5950754ddfef353e9 100644 (file)
--- 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 bca4d18c374cd938c47113075693c1789ca3d998..a127136a3ca966d6c3393aff1c7d47d4e8a59d52 100644 (file)
--- 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 76b9a25f5ee7dc1c827f363aee23028fa1879a2c..36d99f0a29dd611ceb19d679bd1424565f3b752e 100644 (file)
--- 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");
-        return ESP_ERR_INVALID_STATE;
-    }
-
-    if (session_id != cur_session->id) {
-        ESP_LOGE(TAG, "Invalid session");
+    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 (!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");
-        return ESP_ERR_INVALID_STATE;
-    }
-
-    if (session_id != cur_session->id) {
-        ESP_LOGE(TAG, "Invalid session");
+    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 (!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;
-}
+    if (!cur_session || cur_session->id != session_id) {
+        ESP_LOGE(TAG, "Attempt to close invalid session");
+        return ESP_ERR_INVALID_ARG;
+    }
 
-static esp_err_t sec1_cleanup()
-{
-    if (cur_session) {
-        ESP_LOGD(TAG, "Closing current session with id %u", cur_session->id);
+    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;
     }
+
+    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 c7cf8c9211f8e0e3e74d344e0afe0f6e904fac44..5c268bc49a32fd463582b279c383e1b3f3612ad3 100644 (file)
--- 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();