Corrected memory error on security protocol finish

Mika Leppänen · Mika Leppänen · commit 553fbb18648b · 2019-02-25T09:33:00.000+02:00
Security protocols accessed freed data, when incoming data arrived
to FINISH state. This is now corrected so that incoming data frames
are ignored on FINISH state.
diff --git a/source/Security/kmp/kmp_api.c b/source/Security/kmp/kmp_api.c
@@ -37,16 +37,17 @@
 #define TRACE_GROUP "kmap"
 
 struct kmp_api_s {
-    void                         *app_data_ptr;         /**< Opaque pointer for application data */
-    kmp_api_create_confirm       *create_conf;          /**< KMP-CREATE.confirm callback */
-    kmp_api_create_indication    *create_ind;           /**< KMP-CREATE.indication callback */
-    kmp_api_finished_indication  *finished_ind;         /**< KMP-FINISHED.indication callback */
-    kmp_api_finished             *finished;             /**< Finished i.e. ready to be deleted callback */
-    kmp_type_e                   type;                  /**< KMP type */
-    kmp_addr_t                   *addr;                 /**< Supplicant EUI-64, Relay IP address, Relay port */
-    kmp_service_t                *service;              /**< KMP service */
-    bool                         timer_start_pending;   /**< Timer is pending to start */
-    sec_prot_t                   sec_prot;              /**< Security protocol interface */
+    void                         *app_data_ptr;          /**< Opaque pointer for application data */
+    kmp_api_create_confirm       *create_conf;           /**< KMP-CREATE.confirm callback */
+    kmp_api_create_indication    *create_ind;            /**< KMP-CREATE.indication callback */
+    kmp_api_finished_indication  *finished_ind;          /**< KMP-FINISHED.indication callback */
+    kmp_api_finished             *finished;              /**< Finished i.e. ready to be deleted callback */
+    kmp_type_e                   type;                   /**< KMP type */
+    kmp_addr_t                   *addr;                  /**< Supplicant EUI-64, Relay IP address, Relay port */
+    kmp_service_t                *service;               /**< KMP service */
+    bool                         timer_start_pending :1; /**< Timer is pending to start */
+    bool                         receive_disable :1;     /**< Receiving disabled, do not route messages anymore */
+    sec_prot_t                   sec_prot;               /**< Security protocol interface */
 };
 
 typedef struct {
@@ -88,6 +89,7 @@ static void kmp_sec_prot_timer_stop(sec_prot_t *prot);
 static void kmp_sec_prot_state_machine_call(sec_prot_t *prot);
 static void kmp_sec_prot_eui64_addr_get(sec_prot_t *prot, uint8_t *local_eui64,  uint8_t *remote_eui64);
 static sec_prot_t *kmp_sec_prot_by_type_get(sec_prot_t *prot, uint8_t type);
+static void kmp_sec_prot_receive_disable(sec_prot_t *prot);
 
 #define kmp_api_get_from_prot(prot) (kmp_api_t *)(((uint8_t *)prot) - offsetof(kmp_api_t, sec_prot));
 
@@ -126,6 +128,7 @@ kmp_api_t *kmp_api_create(kmp_service_t *service, kmp_type_e type)
     kmp->addr = 0;
     kmp->service = service;
     kmp->timer_start_pending = false;
+    kmp->receive_disable = false;
 
     memset(&kmp->sec_prot, 0, sec_size);
 
@@ -140,6 +143,7 @@ kmp_api_t *kmp_api_create(kmp_service_t *service, kmp_type_e type)
     kmp->sec_prot.state_machine_call = kmp_sec_prot_state_machine_call;
     kmp->sec_prot.addr_get = kmp_sec_prot_eui64_addr_get;
     kmp->sec_prot.type_get = kmp_sec_prot_by_type_get;
+    kmp->sec_prot.receive_disable = kmp_sec_prot_receive_disable;
 
     if (sec_prot->init(&kmp->sec_prot) < 0) {
         ns_dyn_mem_free(kmp);
@@ -279,6 +283,12 @@ static sec_prot_t *kmp_sec_prot_by_type_get(sec_prot_t *prot, uint8_t type)
     return &kmp_by_type->sec_prot;
 }
 
+static void kmp_sec_prot_receive_disable(sec_prot_t *prot)
+{
+    kmp_api_t *kmp = kmp_api_get_from_prot(prot);
+    kmp->receive_disable = true;
+}
+
 void kmp_api_delete(kmp_api_t *kmp)
 {
     if (kmp->sec_prot.delete) {
@@ -426,6 +436,11 @@ int8_t kmp_service_msg_if_receive(kmp_service_t *service, kmp_type_e type, const
         return -1;
     }
 
+    // Security protocol has disables message receiving
+    if (kmp->receive_disable) {
+        return -1;
+    }
+
     int8_t ret = kmp->sec_prot.receive(&kmp->sec_prot, pdu, size);
     return ret;
 }
diff --git a/source/Security/protocols/fwh_sec_prot/supp_fwh_sec_prot.c b/source/Security/protocols/fwh_sec_prot/supp_fwh_sec_prot.c
@@ -47,6 +47,7 @@ typedef enum {
     FWH_STATE_CREATE_IND = SEC_STATE_CREATE_IND,
     FWH_STATE_MESSAGE_1 = SEC_STATE_FIRST,
     FWH_STATE_MESSAGE_3,
+    FWH_STATE_MESSAGE_3_RETRY_WAIT,
     FWH_STATE_CREATE_RESP_SUPP_RETRY,
     FWH_STATE_FINISH = SEC_STATE_FINISH,
     FWH_STATE_FINISHED = SEC_STATE_FINISHED
@@ -76,15 +77,9 @@ typedef struct {
     void                          *recv_pdu;                   /**< received pdu */
     uint16_t                      recv_size;                   /**< received pdu size */
     uint64_t                      recv_replay_cnt;             /**< received replay counter */
+    bool                          msg3_retry_wait :1;          /**< Waiting for Message 3 retry */
 } fwh_sec_prot_int_t;
 
-static const trickle_params_t fwh_trickle_params = {
-    .Imin = 50,            /* 5000ms; ticks are 100ms */
-    .Imax = 150,           /* 15000ms */
-    .k = 0,                /* infinity - no consistency checking */
-    .TimerExpirations = 4
-};
-
 static uint16_t supp_fwh_sec_prot_size(void);
 static int8_t supp_fwh_sec_prot_init(sec_prot_t *prot);
 
@@ -141,6 +136,7 @@ static int8_t supp_fwh_sec_prot_init(sec_prot_t *prot)
     sec_prot_state_set(prot, &data->common, FWH_STATE_INIT);
 
     data->common.ticks = 30 * 10; // 30 seconds
+    data->msg3_retry_wait = false;
 
     uint8_t eui64[8] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08};
     sec_prot_lib_nonce_init(data->snonce, eui64, 1000);
@@ -173,7 +169,7 @@ static int8_t supp_fwh_sec_prot_receive(sec_prot_t *prot, void *pdu, uint16_t si
         // Get message
         data->recv_msg = supp_fwh_sec_prot_message_get(&data->recv_eapol_pdu, prot->sec_keys);
         if (data->recv_msg != FWH_MESSAGE_UNKNOWN) {
-            tr_info("4WH: recv %s", data->recv_msg == FWH_MESSAGE_1 ? "Message 1" : "Message 2");
+            tr_info("4WH: recv %s", data->recv_msg == FWH_MESSAGE_1 ? "Message 1" : "Message 3");
 
             // Call state machine
             data->recv_pdu = pdu;
@@ -278,7 +274,7 @@ static int8_t supp_fwh_sec_prot_message_send(sec_prot_t *prot, fwh_sec_prot_msg_
 static void supp_fwh_sec_prot_timer_timeout(sec_prot_t *prot, uint16_t ticks)
 {
     fwh_sec_prot_int_t *data = fwh_sec_prot_get(prot);
-    sec_prot_timer_timeout_handle(prot, &data->common, &fwh_trickle_params, ticks);
+    sec_prot_timer_timeout_handle(prot, &data->common, NULL, ticks);
 }
 
 static void supp_fwh_sec_prot_state_machine(sec_prot_t *prot)
@@ -379,19 +375,28 @@ static void supp_fwh_sec_prot_state_machine(sec_prot_t *prot)
             break;
 
         case FWH_STATE_FINISH:
-            tr_info("4WH: finish");
+            if (data->msg3_retry_wait) {
+                tr_info("4WH: Message 3 retry timeout");
+                sec_prot_state_set(prot, &data->common, FWH_STATE_FINISHED);
+                return;
+            }
+            data->msg3_retry_wait = true;
+
+            tr_info("4WH: finish, wait Message 3 retry");
 
             // KMP-FINISHED.indication
             sec_prot_keys_ptk_write(prot->sec_keys, data->new_ptk);
             sec_prot_keys_ptk_eui_64_write(prot->sec_keys, data->remote_eui64);
             prot->finished_ind(prot, sec_prot_result_get(&data->common), prot->sec_keys);
-            sec_prot_state_set(prot, &data->common, FWH_STATE_FINISHED);
+
+            data->common.ticks = 60 * 10; // 60 seconds
+            sec_prot_state_set(prot, &data->common, FWH_STATE_MESSAGE_3_RETRY_WAIT);
             break;
 
-        case FWH_STATE_FINISHED:
+        case FWH_STATE_MESSAGE_3_RETRY_WAIT:
             if (sec_prot_result_timeout_check(&data->common)) {
-                prot->timer_stop(prot);
-                prot->finished(prot);
+                tr_info("4WH: Message 3 retry timeout");
+                sec_prot_state_set(prot, &data->common, FWH_STATE_FINISHED);
             } else {
                 if (data->recv_msg != FWH_MESSAGE_3) {
                     return;
@@ -415,25 +420,15 @@ static void supp_fwh_sec_prot_state_machine(sec_prot_t *prot)
                 supp_fwh_sec_prot_recv_replay_counter_store(prot);
                 supp_fwh_sec_prot_security_replay_counter_update(prot);
 
-                tr_info("4WH: start again");
+                tr_info("4WH: send Message 4 again");
 
-                // Send KMP-CREATE.indication
-                prot->create_ind(prot);
-                sec_prot_state_set(prot, &data->common, FWH_STATE_CREATE_RESP_SUPP_RETRY);
+                supp_fwh_sec_prot_message_send(prot, FWH_MESSAGE_4);
             }
             break;
 
-        // Special case for second receiving of 4WH message 3
-        case FWH_STATE_CREATE_RESP_SUPP_RETRY:
-            if (sec_prot_result_ok_check(&data->common)) {
-                // Send 4WH message 4
-                supp_fwh_sec_prot_message_send(prot, FWH_MESSAGE_4);
-                data->common.ticks = 30 * 10; // 30 seconds
-                sec_prot_state_set(prot, &data->common, FWH_STATE_FINISH);
-            } else {
-                // Ready to be deleted
-                sec_prot_state_set(prot, &data->common, FWH_STATE_FINISHED);
-            }
+        case FWH_STATE_FINISHED:
+            prot->timer_stop(prot);
+            prot->finished(prot);
             break;
 
         default:
diff --git a/source/Security/protocols/sec_prot.h b/source/Security/protocols/sec_prot.h
@@ -207,6 +207,16 @@ typedef void sec_prot_eui64_addr_get(sec_prot_t *prot, uint8_t *local_eui64, uin
  */
 typedef sec_prot_t *sec_prot_by_type_get(sec_prot_t *prot, uint8_t type);
 
+/**
+ * sec_prot_receive_disable disables receiving of messages
+ *
+ * \param prot protocol
+ *
+ * \return security protocol or NULL
+ *
+ */
+typedef void sec_prot_receive_disable(sec_prot_t *prot);
+
 // Security protocol data
 struct sec_prot_s {
     sec_prot_create_request       *create_req;           /**< Create request */
@@ -232,6 +242,7 @@ struct sec_prot_s {
 
     sec_prot_eui64_addr_get       *addr_get;             /**< Gets EUI-64 addresses */
     sec_prot_by_type_get          *type_get;             /**< Gets security protocol by type */
+    sec_prot_receive_disable      *receive_disable;      /**< Disable receiving of messages */
 
     sec_prot_keys_t               *sec_keys;             /**< Security keys storage pointer */
     uint8_t                       header_size;           /**< Header size */
diff --git a/source/Security/protocols/sec_prot_lib.c b/source/Security/protocols/sec_prot_lib.c
@@ -54,7 +54,7 @@ void sec_prot_init(sec_prot_common_t *data)
 
 void sec_prot_timer_timeout_handle(sec_prot_t *prot, sec_prot_common_t *data, const trickle_params_t *trickle_params, uint16_t ticks)
 {
-    if (data->trickle_running) {
+    if (data->trickle_running && trickle_params) {
         bool running = trickle_running(&data->trickle_timer, trickle_params);
 
         // Checks for trickle timer expiration */
@@ -112,6 +112,10 @@ void sec_prot_state_set(sec_prot_t *prot, sec_prot_common_t *data, uint8_t state
             // Wait for timeout
             data->trickle_running = false;
             data->ticks = SEC_FINISHED_TIMEOUT;
+
+            // Disables receiving of messages when state machine sets SEC_STATE_FINISHED
+            prot->receive_disable(prot);
+
             // Clear result
             sec_prot_result_set(data, SEC_RESULT_OK);
             break;
@@ -290,6 +294,7 @@ int8_t sec_prot_lib_pmkid_calc(const uint8_t *pmk, const uint8_t *auth_eui64, co
         return -1;
     }
 
+    tr_info("PMKID %s EUI-64 %s %s", trace_array(pmkid, PMKID_LEN), trace_array(auth_eui64, 8), trace_array(supp_eui64, 8));
     return 0;
 }
 
@@ -311,6 +316,7 @@ int8_t sec_prot_lib_ptkid_calc(const uint8_t *ptk, const uint8_t *auth_eui64, co
         return -1;
     }
 
+    tr_info("PTKID %s EUI-64 %s %s", trace_array(ptkid, PTKID_LEN), trace_array(auth_eui64, 8), trace_array(supp_eui64, 8));
     return 0;
 }
 
@@ -471,11 +477,15 @@ int8_t sec_prot_lib_pmkid_generate(sec_prot_t *prot, uint8_t *pmkid, bool is_aut
         prot->addr_get(prot, local_eui64, remote_eui64);
     }
 
+    int8_t ret;
+
     if (is_auth) {
-        return sec_prot_lib_pmkid_calc(pmk, local_eui64, remote_eui64, pmkid);
+        ret = sec_prot_lib_pmkid_calc(pmk, local_eui64, remote_eui64, pmkid);
     } else {
-        return sec_prot_lib_pmkid_calc(pmk, remote_eui64, local_eui64, pmkid);
+        ret = sec_prot_lib_pmkid_calc(pmk, remote_eui64, local_eui64, pmkid);
     }
+
+    return ret;
 }
 
 int8_t sec_prot_lib_ptkid_generate(sec_prot_t *prot, uint8_t *ptkid, bool is_auth)
@@ -492,11 +502,15 @@ int8_t sec_prot_lib_ptkid_generate(sec_prot_t *prot, uint8_t *ptkid, bool is_aut
         return -1;
     }
 
+    int8_t ret;
+
     if (is_auth) {
         return sec_prot_lib_ptkid_calc(ptk, local_eui64, remote_eui64, ptkid);
     } else {
         return sec_prot_lib_ptkid_calc(ptk, remote_eui64, local_eui64, ptkid);
     }
+
+    return ret;
 }
 
 int8_t sec_prot_lib_gtkhash_generate(uint8_t *gtk, uint8_t *gtk_hash)

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ void sec_prot_init(sec_prot_common_t *data)`
`54`	`54`
`55`	`55`	`void sec_prot_timer_timeout_handle(sec_prot_t prot, sec_prot_common_t data, const trickle_params_t *trickle_params, uint16_t ticks)`
`56`	`56`	`{`
`57`		`- if (data->trickle_running) {`
	`57`	`+ if (data->trickle_running && trickle_params) {`
`58`	`58`	`bool running = trickle_running(&data->trickle_timer, trickle_params);`
`59`	`59`
`60`	`60`	`// Checks for trickle timer expiration */`
`@@ -112,6 +112,10 @@ void sec_prot_state_set(sec_prot_t prot, sec_prot_common_t data, uint8_t state`
`112`	`112`	`// Wait for timeout`
`113`	`113`	`data->trickle_running = false;`
`114`	`114`	`data->ticks = SEC_FINISHED_TIMEOUT;`
	`115`	`+`
	`116`	`+ // Disables receiving of messages when state machine sets SEC_STATE_FINISHED`
	`117`	`+ prot->receive_disable(prot);`
	`118`	`+`
`115`	`119`	`// Clear result`
`116`	`120`	`sec_prot_result_set(data, SEC_RESULT_OK);`
`117`	`121`	`break;`
`@@ -290,6 +294,7 @@ int8_t sec_prot_lib_pmkid_calc(const uint8_t pmk, const uint8_t auth_eui64, co`
`290`	`294`	`return -1;`
`291`	`295`	`}`
`292`	`296`
	`297`	`+ tr_info("PMKID %s EUI-64 %s %s", trace_array(pmkid, PMKID_LEN), trace_array(auth_eui64, 8), trace_array(supp_eui64, 8));`
`293`	`298`	`return 0;`
`294`	`299`	`}`
`295`	`300`
`@@ -311,6 +316,7 @@ int8_t sec_prot_lib_ptkid_calc(const uint8_t ptk, const uint8_t auth_eui64, co`
`311`	`316`	`return -1;`
`312`	`317`	`}`
`313`	`318`
	`319`	`+ tr_info("PTKID %s EUI-64 %s %s", trace_array(ptkid, PTKID_LEN), trace_array(auth_eui64, 8), trace_array(supp_eui64, 8));`
`314`	`320`	`return 0;`
`315`	`321`	`}`
`316`	`322`
`@@ -471,11 +477,15 @@ int8_t sec_prot_lib_pmkid_generate(sec_prot_t prot, uint8_t pmkid, bool is_aut`
`471`	`477`	`prot->addr_get(prot, local_eui64, remote_eui64);`
`472`	`478`	`}`
`473`	`479`
	`480`	`+ int8_t ret;`
	`481`	`+`
`474`	`482`	`if (is_auth) {`
`475`		`- return sec_prot_lib_pmkid_calc(pmk, local_eui64, remote_eui64, pmkid);`
	`483`	`+ ret = sec_prot_lib_pmkid_calc(pmk, local_eui64, remote_eui64, pmkid);`
`476`	`484`	`} else {`
`477`		`- return sec_prot_lib_pmkid_calc(pmk, remote_eui64, local_eui64, pmkid);`
	`485`	`+ ret = sec_prot_lib_pmkid_calc(pmk, remote_eui64, local_eui64, pmkid);`
`478`	`486`	`}`
	`487`	`+`
	`488`	`+ return ret;`
`479`	`489`	`}`
`480`	`490`
`481`	`491`	`int8_t sec_prot_lib_ptkid_generate(sec_prot_t prot, uint8_t ptkid, bool is_auth)`
`@@ -492,11 +502,15 @@ int8_t sec_prot_lib_ptkid_generate(sec_prot_t prot, uint8_t ptkid, bool is_aut`
`492`	`502`	`return -1;`
`493`	`503`	`}`
`494`	`504`
	`505`	`+ int8_t ret;`
	`506`	`+`
`495`	`507`	`if (is_auth) {`
`496`	`508`	`return sec_prot_lib_ptkid_calc(ptk, local_eui64, remote_eui64, ptkid);`
`497`	`509`	`} else {`
`498`	`510`	`return sec_prot_lib_ptkid_calc(ptk, remote_eui64, local_eui64, ptkid);`
`499`	`511`	`}`
	`512`	`+`
	`513`	`+ return ret;`
`500`	`514`	`}`
`501`	`515`
`502`	`516`	`int8_t sec_prot_lib_gtkhash_generate(uint8_t gtk, uint8_t gtk_hash)`