| Message ID | 1497378556-9713-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Rejected |
| Headers | show |
On 06/13/2017 11:29 AM, greearb@candelatech.com wrote: > From: Wojciech Dubowik <Wojciech.Dubowik@neratec.com> > > Supplicant is using generic L2 send function for EAPOL > messages which doesn't give back status whether frame has been > acked or not. It can lead to wrong wpa states when EAPOL 4/4 > is lost i.e. client is in connected state but keys aren't > established on AP side. > Fix that by using nl80211_send_eapol_data as for AP side > and check in conneced state that 4/4 EAPOL has been acked. > > As a combined improvement, do not actually set the keys until > we receive notification that the 4/4 message was sent. This fixes > races in ath10k CT firmware, and may eventually let other firmware > remove hacks that were needed to work around this key-setting > race. Any comments on this? We have been testing it for a while, and it seems to work well. Thanks, Ben > > Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@neratec.com> > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > src/drivers/driver.h | 12 +++ > src/drivers/driver_nl80211.c | 11 +++ > src/rsn_supp/wpa.c | 205 +++++++++++++++++++++++++++++++------------ > src/rsn_supp/wpa.h | 12 +++ > src/rsn_supp/wpa_i.h | 5 ++ > wpa_supplicant/driver_i.h | 10 +++ > wpa_supplicant/events.c | 22 +++-- > wpa_supplicant/wpas_glue.c | 9 ++ > 8 files changed, 223 insertions(+), 63 deletions(-) > > diff --git a/src/drivers/driver.h b/src/drivers/driver.h > index 85d5117..d1327f0 100644 > --- a/src/drivers/driver.h > +++ b/src/drivers/driver.h > @@ -2701,6 +2701,18 @@ struct wpa_driver_ops { > const u8 *own_addr, u32 flags); > > /** > + * send_eapol - Send an EAPOL packet (STA only) > + * @priv: private driver interface data > + * @addr: Destination MAC address > + * @data: EAPOL packet starting with IEEE 802.1X header > + * @data_len: Length of the EAPOL packet in octets > + * > + * Returns: 0 on success, -1 on failure > + */ > + int (*send_eapol)(void *priv, const u8 *addr, const u8 *data, > + size_t data_len); > + > + /** > * sta_deauth - Deauthenticate a station (AP only) > * @priv: Private driver interface data > * @own_addr: Source address and BSSID for the Deauthentication frame > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > index 0d5a04d..d7ee936 100644 > --- a/src/drivers/driver_nl80211.c > +++ b/src/drivers/driver_nl80211.c > @@ -4901,6 +4901,16 @@ static int wpa_driver_nl80211_hapd_send_eapol( > return res; > } > > +static int wpa_driver_nl80211_send_eapol( > + void *priv, const u8 *addr, const u8 *data, > + size_t data_len) > +{ > + struct i802_bss *bss = priv; > + > + return nl80211_send_eapol_data(bss, addr, data, data_len); > +} > + > + > > static int wpa_driver_nl80211_sta_set_flags(void *priv, const u8 *addr, > unsigned int total_flags, > @@ -10543,6 +10553,7 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = { > .sta_add = wpa_driver_nl80211_sta_add, > .sta_remove = driver_nl80211_sta_remove, > .hapd_send_eapol = wpa_driver_nl80211_hapd_send_eapol, > + .send_eapol = wpa_driver_nl80211_send_eapol, > .sta_set_flags = wpa_driver_nl80211_sta_set_flags, > .hapd_init = i802_init, > .hapd_deinit = i802_deinit, > diff --git a/src/rsn_supp/wpa.c b/src/rsn_supp/wpa.c > index 8a1d164..be6ab74 100644 > --- a/src/rsn_supp/wpa.c > +++ b/src/rsn_supp/wpa.c > @@ -696,6 +696,8 @@ static void wpa_supplicant_process_1_of_4(struct wpa_sm *sm, > } > #endif /* CONFIG_P2P */ > > + sm->waiting_for_4_of_4_sent = 0; /* not yet */ > + > if (wpa_supplicant_send_2_of_4(sm, sm->bssid, key, ver, sm->snonce, > kde, kde_len, ptk) < 0) > goto failed; > @@ -1344,15 +1346,14 @@ int wpa_supplicant_send_4_of_4(struct wpa_sm *sm, const unsigned char *dst, > } > > > -static void wpa_supplicant_process_3_of_4(struct wpa_sm *sm, > - const struct wpa_eapol_key *key, > - u16 ver, const u8 *key_data, > - size_t key_data_len) > +static void wpa_supplicant_process_3_of_4_send(struct wpa_sm *sm, > + const struct wpa_eapol_key *key, > + u16 ver, const u8 *key_data, > + size_t key_data_len) > { > u16 key_info, keylen; > struct wpa_eapol_ie_parse ie; > > - wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); > wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, "WPA: RX message 3 of 4-Way " > "Handshake from " MACSTR " (ver=%d)", MAC2STR(sm->bssid), ver); > > @@ -1412,64 +1413,16 @@ static void wpa_supplicant_process_3_of_4(struct wpa_sm *sm, > } > #endif /* CONFIG_P2P */ > > + sm->waiting_for_4_of_4_sent = 1; > if (wpa_supplicant_send_4_of_4(sm, sm->bssid, key, ver, key_info, > &sm->ptk) < 0) { > + sm->waiting_for_4_of_4_sent = 0; > goto failed; > } > - > - /* SNonce was successfully used in msg 3/4, so mark it to be renewed > - * for the next 4-Way Handshake. If msg 3 is received again, the old > - * SNonce will still be used to avoid changing PTK. */ > - sm->renew_snonce = 1; > - > - if (key_info & WPA_KEY_INFO_INSTALL) { > - if (wpa_supplicant_install_ptk(sm, key)) > - goto failed; > - } > - > - if (key_info & WPA_KEY_INFO_SECURE) { > - wpa_sm_mlme_setprotection( > - sm, sm->bssid, MLME_SETPROTECTION_PROTECT_TYPE_RX, > - MLME_SETPROTECTION_KEY_TYPE_PAIRWISE); > - eapol_sm_notify_portValid(sm->eapol, TRUE); > - } > - wpa_sm_set_state(sm, WPA_GROUP_HANDSHAKE); > - > - if (sm->group_cipher == WPA_CIPHER_GTK_NOT_USED) { > - wpa_supplicant_key_neg_complete(sm, sm->bssid, > - key_info & WPA_KEY_INFO_SECURE); > - } else if (ie.gtk && > - wpa_supplicant_pairwise_gtk(sm, key, > - ie.gtk, ie.gtk_len, key_info) < 0) { > - wpa_msg(sm->ctx->msg_ctx, MSG_INFO, > - "RSN: Failed to configure GTK"); > - goto failed; > - } > - > - if (ieee80211w_set_keys(sm, &ie) < 0) { > - wpa_msg(sm->ctx->msg_ctx, MSG_INFO, > - "RSN: Failed to configure IGTK"); > - goto failed; > - } > - > - if (ie.gtk) > - wpa_sm_set_rekey_offload(sm); > - > - if (sm->proto == WPA_PROTO_RSN && wpa_key_mgmt_suite_b(sm->key_mgmt)) { > - struct rsn_pmksa_cache_entry *sa; > - > - sa = pmksa_cache_add(sm->pmksa, sm->pmk, sm->pmk_len, NULL, > - sm->ptk.kck, sm->ptk.kck_len, > - sm->bssid, sm->own_addr, > - sm->network_ctx, sm->key_mgmt, NULL); > - if (!sm->cur_pmksa) > - sm->cur_pmksa = sa; > - } > - > - sm->msg_3_of_4_ok = 1; > return; > > failed: > + wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); > wpa_sm_deauthenticate(sm, WLAN_REASON_UNSPECIFIED); > } > > @@ -1979,6 +1932,131 @@ static int wpa_supp_aead_decrypt(struct wpa_sm *sm, u8 *buf, size_t buf_len, > } > #endif /* CONFIG_FILS */ > > +static void wpa_supplicant_process_4_of_4_sent(struct wpa_sm *sm) > +{ > + size_t key_data_len; > + struct wpa_eapol_key *key; > + u16 key_info, ver; > + struct wpa_eapol_ie_parse ie; > + u8 *mic, *key_data; > + size_t mic_len; > + u8 *buf = sm->last_3_of_4_buf; > + > + mic_len = wpa_mic_len(sm->key_mgmt); > + > + key = (struct wpa_eapol_key *) (buf + sizeof(struct ieee802_1x_hdr)); > + mic = (u8 *) (key + 1); > + key_data = mic + mic_len + 2; > + > + wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); > + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, "WPA: Transmitted 4 of 4-Way " > + "Handshake from " MACSTR, MAC2STR(sm->bssid)); > + > + key_info = WPA_GET_BE16(key->key_info); > + ver = key_info & WPA_KEY_INFO_TYPE_MASK; > + > + sm->waiting_for_4_of_4_sent = 0; > + > + key_data_len = WPA_GET_BE16(mic + mic_len); > + > + /* Decrypt the key so we can parse the IEs, etc */ > +#ifdef CONFIG_FILS > + if (!mic_len && (key_info & WPA_KEY_INFO_ENCR_KEY_DATA)) { > + if (wpa_supp_aead_decrypt(sm, buf, sm->last_3_of_4_len, &key_data_len)) > + goto failed; > + } > +#endif /* CONFIG_FILS */ > + > + if ((sm->proto == WPA_PROTO_RSN || sm->proto == WPA_PROTO_OSEN) && > + (key_info & WPA_KEY_INFO_ENCR_KEY_DATA) && mic_len) { > + if (wpa_supplicant_decrypt_key_data(sm, key, mic_len, > + ver, key_data, > + &key_data_len)) > + goto failed; > + } > + > + wpa_hexdump(MSG_DEBUG, "WPA: IE KeyData", key_data, key_data_len); > + if (wpa_supplicant_parse_ies(key_data, key_data_len, &ie) < 0) > + goto failed; > + > + /* SNonce was successfully used in msg 3/4, so mark it to be renewed > + * for the next 4-Way Handshake. If msg 3 is received again, the old > + * SNonce will still be used to avoid changing PTK. */ > + sm->renew_snonce = 1; > + > + if (key_info & WPA_KEY_INFO_INSTALL) { > + if (wpa_supplicant_install_ptk(sm, key)) > + goto failed; > + } > + > + if (key_info & WPA_KEY_INFO_SECURE) { > + wpa_sm_mlme_setprotection( > + sm, sm->bssid, MLME_SETPROTECTION_PROTECT_TYPE_RX, > + MLME_SETPROTECTION_KEY_TYPE_PAIRWISE); > + eapol_sm_notify_portValid(sm->eapol, TRUE); > + } > + wpa_sm_set_state(sm, WPA_GROUP_HANDSHAKE); > + > + if (sm->group_cipher == WPA_CIPHER_GTK_NOT_USED) { > + wpa_supplicant_key_neg_complete(sm, sm->bssid, > + key_info & WPA_KEY_INFO_SECURE); > + } else if (ie.gtk && > + wpa_supplicant_pairwise_gtk(sm, key, > + ie.gtk, ie.gtk_len, key_info) < 0) { > + wpa_msg(sm->ctx->msg_ctx, MSG_INFO, > + "RSN: Failed to configure GTK"); > + goto failed; > + } > + > + if (ieee80211w_set_keys(sm, &ie) < 0) { > + wpa_msg(sm->ctx->msg_ctx, MSG_INFO, > + "RSN: Failed to configure IGTK"); > + goto failed; > + } > + > + if (ie.gtk) > + wpa_sm_set_rekey_offload(sm); > + > + if (sm->proto == WPA_PROTO_RSN && wpa_key_mgmt_suite_b(sm->key_mgmt)) { > + struct rsn_pmksa_cache_entry *sa; > + > + sa = pmksa_cache_add(sm->pmksa, sm->pmk, sm->pmk_len, NULL, > + sm->ptk.kck, sm->ptk.kck_len, > + sm->bssid, sm->own_addr, > + sm->network_ctx, sm->key_mgmt, NULL); > + if (!sm->cur_pmksa) > + sm->cur_pmksa = sa; > + } > + > + sm->msg_3_of_4_ok = 1; > + return; > + > +failed: > + wpa_sm_deauthenticate(sm, WLAN_REASON_UNSPECIFIED); > +} > + > +void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available) > +{ > + sm->eapol_tx_status_available = is_available; > +} > + > +/* De-auth if return is < 0 */ > +int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, > + const u8 *buf, size_t len, int ack) > +{ > + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, > + "EAPOL_TX_STATUS: ACK(%d) waiting 4/4-tx-status: %d", ack, sm->waiting_for_4_of_4_sent); > + if (ack && sm->waiting_for_4_of_4_sent) { > + wpa_supplicant_process_4_of_4_sent(sm); > + } > + else if (!ack && sm->waiting_for_4_of_4_sent) { > + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, > + "EAPOL 4/4 Not acked, disconnecting"); > + return -1; > + } > + return 0; > +} > + > #ifdef CONFIG_TESTING_OPTIONS > /* Mostly same as below, but this should not change any state. Returns the > * message type so we can make decisions before feeding this into the state > @@ -2391,8 +2469,19 @@ int wpa_sm_rx_eapol(struct wpa_sm *sm, const u8 *src_addr, > } else if (key_info & (WPA_KEY_INFO_MIC | > WPA_KEY_INFO_ENCR_KEY_DATA)) { > /* 3/4 4-Way Handshake */ > - wpa_supplicant_process_3_of_4(sm, key, ver, key_data, > - key_data_len); > + /* Save buffer for doing the second half of the 4/4 processing > + * once we get 4/4 ack status > + */ > + int my_len = sizeof(sm->last_3_of_4_buf); > + if (len < my_len) > + my_len = len; > + memcpy(sm->last_3_of_4_buf, buf, my_len); > + sm->last_3_of_4_len = my_len; > + > + wpa_supplicant_process_3_of_4_send(sm, key, ver, key_data, > + key_data_len); > + if (!sm->eapol_tx_status_available) > + wpa_supplicant_process_4_of_4_sent(sm); > } else { > /* 1/4 4-Way Handshake */ > wpa_supplicant_process_1_of_4(sm, src_addr, key, > diff --git a/src/rsn_supp/wpa.h b/src/rsn_supp/wpa.h > index 7e9873c..3eb9d72 100644 > --- a/src/rsn_supp/wpa.h > +++ b/src/rsn_supp/wpa.h > @@ -213,6 +213,9 @@ void wpa_sm_set_rx_replay_ctr(struct wpa_sm *sm, const u8 *rx_replay_counter); > void wpa_sm_set_ptk_kck_kek(struct wpa_sm *sm, > const u8 *ptk_kck, size_t ptk_kck_len, > const u8 *ptk_kek, size_t ptk_kek_len); > +int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, > + const u8 *buf, size_t len, int ack); > +void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available); > > #else /* CONFIG_NO_WPA */ > > @@ -395,6 +398,15 @@ static inline void wpa_sm_set_ptk_kck_kek(struct wpa_sm *sm, const u8 *ptk_kck, > { > } > > +static int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, > + const u8 *buf, size_t len, int ack) > +{ > +} > + > +static void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available) > +{ > +} > + > #endif /* CONFIG_NO_WPA */ > > #ifdef CONFIG_PEERKEY > diff --git a/src/rsn_supp/wpa_i.h b/src/rsn_supp/wpa_i.h > index 75af31e..0d305a3 100644 > --- a/src/rsn_supp/wpa_i.h > +++ b/src/rsn_supp/wpa_i.h > @@ -170,6 +170,11 @@ struct wpa_sm { > #ifdef CONFIG_OWE > struct crypto_ecdh *owe_ecdh; > #endif /* CONFIG_OWE */ > + > + u8 waiting_for_4_of_4_sent; /* boolean */ > + u8 eapol_tx_status_available; > + u16 last_3_of_4_len; > + u8 last_3_of_4_buf[1500]; > }; > > > diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h > index 0ef39b7..c5cd342 100644 > --- a/wpa_supplicant/driver_i.h > +++ b/wpa_supplicant/driver_i.h > @@ -356,6 +356,16 @@ static inline int wpa_drv_hapd_send_eapol(struct wpa_supplicant *wpa_s, > return -1; > } > > +static inline int wpa_drv_send_eapol(struct wpa_supplicant *wpa_s, > + const u8 *addr, const u8 *data, > + size_t data_len) > +{ > + if (wpa_s->driver->hapd_send_eapol) > + return wpa_s->driver->send_eapol(wpa_s->drv_priv, addr, > + data, data_len); > + return -1; > +} > + > static inline int wpa_drv_sta_set_flags(struct wpa_supplicant *wpa_s, > const u8 *addr, int total_flags, > int flags_or, int flags_and) > diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c > index 1d3c1a5..32cd1aa 100644 > --- a/wpa_supplicant/events.c > +++ b/wpa_supplicant/events.c > @@ -4046,13 +4046,25 @@ void wpa_supplicant_event(void *ctx, enum wpa_event_type event, > } > #endif /* CONFIG_AP */ > break; > -#ifdef CONFIG_AP > case EVENT_EAPOL_TX_STATUS: > - ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst, > - data->eapol_tx_status.data, > - data->eapol_tx_status.data_len, > - data->eapol_tx_status.ack); > + if (wpa_s->ap_iface) { > + ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst, > + data->eapol_tx_status.data, > + data->eapol_tx_status.data_len, > + data->eapol_tx_status.ack); > + } > + else { > + if (wpa_sm_eapol_tx_status(wpa_s->wpa, data->eapol_tx_status.dst, > + data->eapol_tx_status.data, > + data->eapol_tx_status.data_len, > + data->eapol_tx_status.ack) < 0) { > + wpa_s->own_disconnect_req = 1; > + wpa_supplicant_deauthenticate( > + wpa_s, WLAN_REASON_4WAY_HANDSHAKE_TIMEOUT); > + } > + } > break; > +#ifdef CONFIG_AP > case EVENT_DRIVER_CLIENT_POLL_OK: > ap_client_poll_ok(wpa_s, data->client_poll.addr); > break; > diff --git a/wpa_supplicant/wpas_glue.c b/wpa_supplicant/wpas_glue.c > index ad20dc9..c656eec 100644 > --- a/wpa_supplicant/wpas_glue.c > +++ b/wpa_supplicant/wpas_glue.c > @@ -97,6 +97,7 @@ static u8 * wpa_alloc_eapol(const struct wpa_supplicant *wpa_s, u8 type, > static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, > u16 proto, const u8 *buf, size_t len) > { > + int ret; > #ifdef CONFIG_TESTING_OPTIONS > if (wpa_s->ext_eapol_frame_io && proto == ETH_P_EAPOL) { > size_t hex_len = 2 * len + 1; > @@ -111,6 +112,14 @@ static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, > return 0; > } > #endif /* CONFIG_TESTING_OPTIONS */ > + ret = wpa_drv_send_eapol(wpa_s, dest, buf, len); > + > + wpa_sm_eapol_tx_status_available(wpa_s->wpa, ret >= 0); > + > + if (ret >= 0) > + return ret; > + > + wpa_dbg(wpa_s, MSG_DEBUG, "wpa_drv_send_eapol rv (%d)", ret); > > if (wpa_s->l2) { > return l2_packet_send(wpa_s->l2, dest, proto, buf, len); >
> -----Original Message----- > From: Hostap [mailto:hostap-bounces@lists.infradead.org] On Behalf Of Ben > Greear > Sent: Friday, July 07, 2017 12:42 AM > To: hostap@lists.infradead.org > Cc: Wojciech Dubowik > Subject: Re: [PATCH] wpa_supplicant: Wait for eapol 4/4 tx-status before > setting key. > > On 06/13/2017 11:29 AM, greearb@candelatech.com wrote: > > From: Wojciech Dubowik <Wojciech.Dubowik@neratec.com> > > > > Supplicant is using generic L2 send function for EAPOL messages which > > doesn't give back status whether frame has been acked or not. It can > > lead to wrong wpa states when EAPOL 4/4 is lost i.e. client is in > > connected state but keys aren't established on AP side. > > Fix that by using nl80211_send_eapol_data as for AP side and check in > > conneced state that 4/4 EAPOL has been acked. > > > > As a combined improvement, do not actually set the keys until we > > receive notification that the 4/4 message was sent. This fixes races > > in ath10k CT firmware, and may eventually let other firmware remove > > hacks that were needed to work around this key-setting race. > > Any comments on this? We have been testing it for a while, and it seems to > work well. I think that delaying the 'set_key' too much might also be problematic since it may raise interop issues where we might lose the first frames sent from the remote device which will be already encrypted. > > Thanks, > Ben > Thanks, Amir
On 07/09/2017 12:28 AM, Ayun, Amir wrote: >> -----Original Message----- >> From: Hostap [mailto:hostap-bounces@lists.infradead.org] On Behalf Of Ben >> Greear >> Sent: Friday, July 07, 2017 12:42 AM >> To: hostap@lists.infradead.org >> Cc: Wojciech Dubowik >> Subject: Re: [PATCH] wpa_supplicant: Wait for eapol 4/4 tx-status before >> setting key. >> >> On 06/13/2017 11:29 AM, greearb@candelatech.com wrote: >>> From: Wojciech Dubowik <Wojciech.Dubowik@neratec.com> >>> >>> Supplicant is using generic L2 send function for EAPOL messages which >>> doesn't give back status whether frame has been acked or not. It can >>> lead to wrong wpa states when EAPOL 4/4 is lost i.e. client is in >>> connected state but keys aren't established on AP side. >>> Fix that by using nl80211_send_eapol_data as for AP side and check in >>> conneced state that 4/4 EAPOL has been acked. >>> >>> As a combined improvement, do not actually set the keys until we >>> receive notification that the 4/4 message was sent. This fixes races >>> in ath10k CT firmware, and may eventually let other firmware remove >>> hacks that were needed to work around this key-setting race. >> >> Any comments on this? We have been testing it for a while, and it seems to >> work well. > > I think that delaying the 'set_key' too much might also be problematic since it may raise interop issues > where we might lose the first frames sent from the remote device which will be already encrypted. It should not delay it by more than a milisecond or so, and most of the time stations are going to do DHCP before the AP will send them anything useful? Without having supplicant wait, drivers/firmware are forced to implement various hacks and/or just accept races and retry when things go wrong. At least things like ath10k firmware are still going to wait for the ACK to actually set the key, but they will do all of that logic in firmware instead of supplicant, which complicates and adds bugs to the firmware. Thanks, Ben
On 06/07/2017 23:42, Ben Greear wrote: > On 06/13/2017 11:29 AM, greearb@candelatech.com wrote: >> From: Wojciech Dubowik <Wojciech.Dubowik@neratec.com> >> >> Supplicant is using generic L2 send function for EAPOL >> messages which doesn't give back status whether frame has been >> acked or not. It can lead to wrong wpa states when EAPOL 4/4 >> is lost i.e. client is in connected state but keys aren't >> established on AP side. >> Fix that by using nl80211_send_eapol_data as for AP side >> and check in conneced state that 4/4 EAPOL has been acked. >> >> As a combined improvement, do not actually set the keys until >> we receive notification that the 4/4 message was sent. This fixes >> races in ath10k CT firmware, and may eventually let other firmware >> remove hacks that were needed to work around this key-setting >> race. > > Any comments on this? We have been testing it for a while, and it > seems to work well. I have the same comment as Ilan Peer: disconnecting when failing to send 4/4 is a bit brutal, especially if the 4HS is used for a PTK renewal. I understand that this helps with the case where the authenticator received the 4/4 but the supplicant failed to receive the ACK for it after many retries. But that case should be a bit rare, no ? I'm also curious if it improves the situation of IBSS-RSN, where disconnection isn't possible if the supplicant and authenticator end up in the wrong state.
> I have the same comment as Ilan Peer: disconnecting when failing to > send 4/4 is a bit brutal, especially if the 4HS is used for a PTK > renewal. I understand that this helps with the case where the > authenticator received the 4/4 but the supplicant failed to receive > the ACK for it after many retries. But that case should be a bit rare, > no ? Disconnecting on lost EAPOL 4/4 is just the first step. I understand it's a bit brutal. The end target should be to integrate ACK into state machine and use it instead of fixed timeouts. This requires a lot of effort and I am not able to come with the end solution just right now. It gets a bit more complex as some legacy devices might not have this info and supplicant doesn't only supports mac80211 drivers. It's true this case is rare as it happens only in very noisy environment. But when it happens it make couple of seconds break in communication. So one would see it more while roaming frequently. On the other hand, when there is an ACK info for EAPOL, why not to use it instead of firing and forgetting? Wojtek > > I'm also curious if it improves the situation of IBSS-RSN, where > disconnection isn't possible if the supplicant and authenticator end > up in the wrong state. > > _______________________________________________ > Hostap mailing list > Hostap@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/hostap
On Tue, Jun 13, 2017 at 11:29:16AM -0700, greearb@candelatech.com wrote: > Supplicant is using generic L2 send function for EAPOL > messages which doesn't give back status whether frame has been > acked or not. It can lead to wrong wpa states when EAPOL 4/4 > is lost i.e. client is in connected state but keys aren't > established on AP side. > Fix that by using nl80211_send_eapol_data as for AP side > and check in conneced state that 4/4 EAPOL has been acked. That part about tracking TX status sounds fine.. > As a combined improvement, do not actually set the keys until > we receive notification that the 4/4 message was sent. This fixes > races in ath10k CT firmware, and may eventually let other firmware > remove hacks that were needed to work around this key-setting > race. However, this is going to the wrong direction to work around an issue. The pairwise key (TK) needs to be set sooner, not later; but only for RX first. Delaying setting of the TK for RX will introduce more issues with Action frame RX when using PMF. There are number of cases where the AP sends a protected Action frame immediately after receiving the EAPOL-Key msg 4/4. Any extra delay on the station side will increase likelihood of dropping such frames. The correct way to fix this is to finally provide means for setting RX-only key that is then converted to TX+RX at a suitable point in time. In practice, doing EAPOL over nl80211 with extra flags for controlling whether to encrypt (and even more complicated, whether to use the previous key during PTK rekeying) may end up being the easiest way of fixing this.
On 09/10/2017 02:27 AM, Jouni Malinen wrote: > On Tue, Jun 13, 2017 at 11:29:16AM -0700, greearb@candelatech.com wrote: >> Supplicant is using generic L2 send function for EAPOL >> messages which doesn't give back status whether frame has been >> acked or not. It can lead to wrong wpa states when EAPOL 4/4 >> is lost i.e. client is in connected state but keys aren't >> established on AP side. >> Fix that by using nl80211_send_eapol_data as for AP side >> and check in conneced state that 4/4 EAPOL has been acked. > > That part about tracking TX status sounds fine.. > >> As a combined improvement, do not actually set the keys until >> we receive notification that the 4/4 message was sent. This fixes >> races in ath10k CT firmware, and may eventually let other firmware >> remove hacks that were needed to work around this key-setting >> race. > > However, this is going to the wrong direction to work around an issue. > The pairwise key (TK) needs to be set sooner, not later; but only for RX > first. Delaying setting of the TK for RX will introduce more issues with > Action frame RX when using PMF. There are number of cases where the AP > sends a protected Action frame immediately after receiving the EAPOL-Key > msg 4/4. Any extra delay on the station side will increase likelihood of > dropping such frames. The correct way to fix this is to finally provide > means for setting RX-only key that is then converted to TX+RX at a > suitable point in time. In practice, doing EAPOL over nl80211 with extra > flags for controlling whether to encrypt (and even more complicated, > whether to use the previous key during PTK rekeying) may end up being > the easiest way of fixing this. I don't think there is any API that can do this for ath10k firmware currently. But, if I were able to fix the driver & firmware somehow to allow setting an rx-key only, then I guess that should work for any type of frame, not just EAPOL? In other words, is it *just* the need for setting an rx-only key, or are there more strange issues that need to be resolved at the same time? Even if I do this, it will probably not make it upstream since QCA is unlikely to take my patches...but maybe that is OK and other vendors at least could do the right thing in the future? I guess a new netlink flag to the set-key command that was 'RX-ONLY' or similar could be used on the set-key where it currently is in un-patched supplicant, and then the normal set-key could be called after the 4/4 is received like in the patch I posted? Thanks, Ben
diff --git a/src/drivers/driver.h b/src/drivers/driver.h index 85d5117..d1327f0 100644 --- a/src/drivers/driver.h +++ b/src/drivers/driver.h @@ -2701,6 +2701,18 @@ struct wpa_driver_ops { const u8 *own_addr, u32 flags); /** + * send_eapol - Send an EAPOL packet (STA only) + * @priv: private driver interface data + * @addr: Destination MAC address + * @data: EAPOL packet starting with IEEE 802.1X header + * @data_len: Length of the EAPOL packet in octets + * + * Returns: 0 on success, -1 on failure + */ + int (*send_eapol)(void *priv, const u8 *addr, const u8 *data, + size_t data_len); + + /** * sta_deauth - Deauthenticate a station (AP only) * @priv: Private driver interface data * @own_addr: Source address and BSSID for the Deauthentication frame diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 0d5a04d..d7ee936 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -4901,6 +4901,16 @@ static int wpa_driver_nl80211_hapd_send_eapol( return res; } +static int wpa_driver_nl80211_send_eapol( + void *priv, const u8 *addr, const u8 *data, + size_t data_len) +{ + struct i802_bss *bss = priv; + + return nl80211_send_eapol_data(bss, addr, data, data_len); +} + + static int wpa_driver_nl80211_sta_set_flags(void *priv, const u8 *addr, unsigned int total_flags, @@ -10543,6 +10553,7 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = { .sta_add = wpa_driver_nl80211_sta_add, .sta_remove = driver_nl80211_sta_remove, .hapd_send_eapol = wpa_driver_nl80211_hapd_send_eapol, + .send_eapol = wpa_driver_nl80211_send_eapol, .sta_set_flags = wpa_driver_nl80211_sta_set_flags, .hapd_init = i802_init, .hapd_deinit = i802_deinit, diff --git a/src/rsn_supp/wpa.c b/src/rsn_supp/wpa.c index 8a1d164..be6ab74 100644 --- a/src/rsn_supp/wpa.c +++ b/src/rsn_supp/wpa.c @@ -696,6 +696,8 @@ static void wpa_supplicant_process_1_of_4(struct wpa_sm *sm, } #endif /* CONFIG_P2P */ + sm->waiting_for_4_of_4_sent = 0; /* not yet */ + if (wpa_supplicant_send_2_of_4(sm, sm->bssid, key, ver, sm->snonce, kde, kde_len, ptk) < 0) goto failed; @@ -1344,15 +1346,14 @@ int wpa_supplicant_send_4_of_4(struct wpa_sm *sm, const unsigned char *dst, } -static void wpa_supplicant_process_3_of_4(struct wpa_sm *sm, - const struct wpa_eapol_key *key, - u16 ver, const u8 *key_data, - size_t key_data_len) +static void wpa_supplicant_process_3_of_4_send(struct wpa_sm *sm, + const struct wpa_eapol_key *key, + u16 ver, const u8 *key_data, + size_t key_data_len) { u16 key_info, keylen; struct wpa_eapol_ie_parse ie; - wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, "WPA: RX message 3 of 4-Way " "Handshake from " MACSTR " (ver=%d)", MAC2STR(sm->bssid), ver); @@ -1412,64 +1413,16 @@ static void wpa_supplicant_process_3_of_4(struct wpa_sm *sm, } #endif /* CONFIG_P2P */ + sm->waiting_for_4_of_4_sent = 1; if (wpa_supplicant_send_4_of_4(sm, sm->bssid, key, ver, key_info, &sm->ptk) < 0) { + sm->waiting_for_4_of_4_sent = 0; goto failed; } - - /* SNonce was successfully used in msg 3/4, so mark it to be renewed - * for the next 4-Way Handshake. If msg 3 is received again, the old - * SNonce will still be used to avoid changing PTK. */ - sm->renew_snonce = 1; - - if (key_info & WPA_KEY_INFO_INSTALL) { - if (wpa_supplicant_install_ptk(sm, key)) - goto failed; - } - - if (key_info & WPA_KEY_INFO_SECURE) { - wpa_sm_mlme_setprotection( - sm, sm->bssid, MLME_SETPROTECTION_PROTECT_TYPE_RX, - MLME_SETPROTECTION_KEY_TYPE_PAIRWISE); - eapol_sm_notify_portValid(sm->eapol, TRUE); - } - wpa_sm_set_state(sm, WPA_GROUP_HANDSHAKE); - - if (sm->group_cipher == WPA_CIPHER_GTK_NOT_USED) { - wpa_supplicant_key_neg_complete(sm, sm->bssid, - key_info & WPA_KEY_INFO_SECURE); - } else if (ie.gtk && - wpa_supplicant_pairwise_gtk(sm, key, - ie.gtk, ie.gtk_len, key_info) < 0) { - wpa_msg(sm->ctx->msg_ctx, MSG_INFO, - "RSN: Failed to configure GTK"); - goto failed; - } - - if (ieee80211w_set_keys(sm, &ie) < 0) { - wpa_msg(sm->ctx->msg_ctx, MSG_INFO, - "RSN: Failed to configure IGTK"); - goto failed; - } - - if (ie.gtk) - wpa_sm_set_rekey_offload(sm); - - if (sm->proto == WPA_PROTO_RSN && wpa_key_mgmt_suite_b(sm->key_mgmt)) { - struct rsn_pmksa_cache_entry *sa; - - sa = pmksa_cache_add(sm->pmksa, sm->pmk, sm->pmk_len, NULL, - sm->ptk.kck, sm->ptk.kck_len, - sm->bssid, sm->own_addr, - sm->network_ctx, sm->key_mgmt, NULL); - if (!sm->cur_pmksa) - sm->cur_pmksa = sa; - } - - sm->msg_3_of_4_ok = 1; return; failed: + wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); wpa_sm_deauthenticate(sm, WLAN_REASON_UNSPECIFIED); } @@ -1979,6 +1932,131 @@ static int wpa_supp_aead_decrypt(struct wpa_sm *sm, u8 *buf, size_t buf_len, } #endif /* CONFIG_FILS */ +static void wpa_supplicant_process_4_of_4_sent(struct wpa_sm *sm) +{ + size_t key_data_len; + struct wpa_eapol_key *key; + u16 key_info, ver; + struct wpa_eapol_ie_parse ie; + u8 *mic, *key_data; + size_t mic_len; + u8 *buf = sm->last_3_of_4_buf; + + mic_len = wpa_mic_len(sm->key_mgmt); + + key = (struct wpa_eapol_key *) (buf + sizeof(struct ieee802_1x_hdr)); + mic = (u8 *) (key + 1); + key_data = mic + mic_len + 2; + + wpa_sm_set_state(sm, WPA_4WAY_HANDSHAKE); + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, "WPA: Transmitted 4 of 4-Way " + "Handshake from " MACSTR, MAC2STR(sm->bssid)); + + key_info = WPA_GET_BE16(key->key_info); + ver = key_info & WPA_KEY_INFO_TYPE_MASK; + + sm->waiting_for_4_of_4_sent = 0; + + key_data_len = WPA_GET_BE16(mic + mic_len); + + /* Decrypt the key so we can parse the IEs, etc */ +#ifdef CONFIG_FILS + if (!mic_len && (key_info & WPA_KEY_INFO_ENCR_KEY_DATA)) { + if (wpa_supp_aead_decrypt(sm, buf, sm->last_3_of_4_len, &key_data_len)) + goto failed; + } +#endif /* CONFIG_FILS */ + + if ((sm->proto == WPA_PROTO_RSN || sm->proto == WPA_PROTO_OSEN) && + (key_info & WPA_KEY_INFO_ENCR_KEY_DATA) && mic_len) { + if (wpa_supplicant_decrypt_key_data(sm, key, mic_len, + ver, key_data, + &key_data_len)) + goto failed; + } + + wpa_hexdump(MSG_DEBUG, "WPA: IE KeyData", key_data, key_data_len); + if (wpa_supplicant_parse_ies(key_data, key_data_len, &ie) < 0) + goto failed; + + /* SNonce was successfully used in msg 3/4, so mark it to be renewed + * for the next 4-Way Handshake. If msg 3 is received again, the old + * SNonce will still be used to avoid changing PTK. */ + sm->renew_snonce = 1; + + if (key_info & WPA_KEY_INFO_INSTALL) { + if (wpa_supplicant_install_ptk(sm, key)) + goto failed; + } + + if (key_info & WPA_KEY_INFO_SECURE) { + wpa_sm_mlme_setprotection( + sm, sm->bssid, MLME_SETPROTECTION_PROTECT_TYPE_RX, + MLME_SETPROTECTION_KEY_TYPE_PAIRWISE); + eapol_sm_notify_portValid(sm->eapol, TRUE); + } + wpa_sm_set_state(sm, WPA_GROUP_HANDSHAKE); + + if (sm->group_cipher == WPA_CIPHER_GTK_NOT_USED) { + wpa_supplicant_key_neg_complete(sm, sm->bssid, + key_info & WPA_KEY_INFO_SECURE); + } else if (ie.gtk && + wpa_supplicant_pairwise_gtk(sm, key, + ie.gtk, ie.gtk_len, key_info) < 0) { + wpa_msg(sm->ctx->msg_ctx, MSG_INFO, + "RSN: Failed to configure GTK"); + goto failed; + } + + if (ieee80211w_set_keys(sm, &ie) < 0) { + wpa_msg(sm->ctx->msg_ctx, MSG_INFO, + "RSN: Failed to configure IGTK"); + goto failed; + } + + if (ie.gtk) + wpa_sm_set_rekey_offload(sm); + + if (sm->proto == WPA_PROTO_RSN && wpa_key_mgmt_suite_b(sm->key_mgmt)) { + struct rsn_pmksa_cache_entry *sa; + + sa = pmksa_cache_add(sm->pmksa, sm->pmk, sm->pmk_len, NULL, + sm->ptk.kck, sm->ptk.kck_len, + sm->bssid, sm->own_addr, + sm->network_ctx, sm->key_mgmt, NULL); + if (!sm->cur_pmksa) + sm->cur_pmksa = sa; + } + + sm->msg_3_of_4_ok = 1; + return; + +failed: + wpa_sm_deauthenticate(sm, WLAN_REASON_UNSPECIFIED); +} + +void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available) +{ + sm->eapol_tx_status_available = is_available; +} + +/* De-auth if return is < 0 */ +int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, + const u8 *buf, size_t len, int ack) +{ + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, + "EAPOL_TX_STATUS: ACK(%d) waiting 4/4-tx-status: %d", ack, sm->waiting_for_4_of_4_sent); + if (ack && sm->waiting_for_4_of_4_sent) { + wpa_supplicant_process_4_of_4_sent(sm); + } + else if (!ack && sm->waiting_for_4_of_4_sent) { + wpa_dbg(sm->ctx->msg_ctx, MSG_DEBUG, + "EAPOL 4/4 Not acked, disconnecting"); + return -1; + } + return 0; +} + #ifdef CONFIG_TESTING_OPTIONS /* Mostly same as below, but this should not change any state. Returns the * message type so we can make decisions before feeding this into the state @@ -2391,8 +2469,19 @@ int wpa_sm_rx_eapol(struct wpa_sm *sm, const u8 *src_addr, } else if (key_info & (WPA_KEY_INFO_MIC | WPA_KEY_INFO_ENCR_KEY_DATA)) { /* 3/4 4-Way Handshake */ - wpa_supplicant_process_3_of_4(sm, key, ver, key_data, - key_data_len); + /* Save buffer for doing the second half of the 4/4 processing + * once we get 4/4 ack status + */ + int my_len = sizeof(sm->last_3_of_4_buf); + if (len < my_len) + my_len = len; + memcpy(sm->last_3_of_4_buf, buf, my_len); + sm->last_3_of_4_len = my_len; + + wpa_supplicant_process_3_of_4_send(sm, key, ver, key_data, + key_data_len); + if (!sm->eapol_tx_status_available) + wpa_supplicant_process_4_of_4_sent(sm); } else { /* 1/4 4-Way Handshake */ wpa_supplicant_process_1_of_4(sm, src_addr, key, diff --git a/src/rsn_supp/wpa.h b/src/rsn_supp/wpa.h index 7e9873c..3eb9d72 100644 --- a/src/rsn_supp/wpa.h +++ b/src/rsn_supp/wpa.h @@ -213,6 +213,9 @@ void wpa_sm_set_rx_replay_ctr(struct wpa_sm *sm, const u8 *rx_replay_counter); void wpa_sm_set_ptk_kck_kek(struct wpa_sm *sm, const u8 *ptk_kck, size_t ptk_kck_len, const u8 *ptk_kek, size_t ptk_kek_len); +int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, + const u8 *buf, size_t len, int ack); +void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available); #else /* CONFIG_NO_WPA */ @@ -395,6 +398,15 @@ static inline void wpa_sm_set_ptk_kck_kek(struct wpa_sm *sm, const u8 *ptk_kck, { } +static int wpa_sm_eapol_tx_status(struct wpa_sm *sm, const u8 *dst, + const u8 *buf, size_t len, int ack) +{ +} + +static void wpa_sm_eapol_tx_status_available(struct wpa_sm *sm, int is_available) +{ +} + #endif /* CONFIG_NO_WPA */ #ifdef CONFIG_PEERKEY diff --git a/src/rsn_supp/wpa_i.h b/src/rsn_supp/wpa_i.h index 75af31e..0d305a3 100644 --- a/src/rsn_supp/wpa_i.h +++ b/src/rsn_supp/wpa_i.h @@ -170,6 +170,11 @@ struct wpa_sm { #ifdef CONFIG_OWE struct crypto_ecdh *owe_ecdh; #endif /* CONFIG_OWE */ + + u8 waiting_for_4_of_4_sent; /* boolean */ + u8 eapol_tx_status_available; + u16 last_3_of_4_len; + u8 last_3_of_4_buf[1500]; }; diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h index 0ef39b7..c5cd342 100644 --- a/wpa_supplicant/driver_i.h +++ b/wpa_supplicant/driver_i.h @@ -356,6 +356,16 @@ static inline int wpa_drv_hapd_send_eapol(struct wpa_supplicant *wpa_s, return -1; } +static inline int wpa_drv_send_eapol(struct wpa_supplicant *wpa_s, + const u8 *addr, const u8 *data, + size_t data_len) +{ + if (wpa_s->driver->hapd_send_eapol) + return wpa_s->driver->send_eapol(wpa_s->drv_priv, addr, + data, data_len); + return -1; +} + static inline int wpa_drv_sta_set_flags(struct wpa_supplicant *wpa_s, const u8 *addr, int total_flags, int flags_or, int flags_and) diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 1d3c1a5..32cd1aa 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -4046,13 +4046,25 @@ void wpa_supplicant_event(void *ctx, enum wpa_event_type event, } #endif /* CONFIG_AP */ break; -#ifdef CONFIG_AP case EVENT_EAPOL_TX_STATUS: - ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst, - data->eapol_tx_status.data, - data->eapol_tx_status.data_len, - data->eapol_tx_status.ack); + if (wpa_s->ap_iface) { + ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst, + data->eapol_tx_status.data, + data->eapol_tx_status.data_len, + data->eapol_tx_status.ack); + } + else { + if (wpa_sm_eapol_tx_status(wpa_s->wpa, data->eapol_tx_status.dst, + data->eapol_tx_status.data, + data->eapol_tx_status.data_len, + data->eapol_tx_status.ack) < 0) { + wpa_s->own_disconnect_req = 1; + wpa_supplicant_deauthenticate( + wpa_s, WLAN_REASON_4WAY_HANDSHAKE_TIMEOUT); + } + } break; +#ifdef CONFIG_AP case EVENT_DRIVER_CLIENT_POLL_OK: ap_client_poll_ok(wpa_s, data->client_poll.addr); break; diff --git a/wpa_supplicant/wpas_glue.c b/wpa_supplicant/wpas_glue.c index ad20dc9..c656eec 100644 --- a/wpa_supplicant/wpas_glue.c +++ b/wpa_supplicant/wpas_glue.c @@ -97,6 +97,7 @@ static u8 * wpa_alloc_eapol(const struct wpa_supplicant *wpa_s, u8 type, static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, u16 proto, const u8 *buf, size_t len) { + int ret; #ifdef CONFIG_TESTING_OPTIONS if (wpa_s->ext_eapol_frame_io && proto == ETH_P_EAPOL) { size_t hex_len = 2 * len + 1; @@ -111,6 +112,14 @@ static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, return 0; } #endif /* CONFIG_TESTING_OPTIONS */ + ret = wpa_drv_send_eapol(wpa_s, dest, buf, len); + + wpa_sm_eapol_tx_status_available(wpa_s->wpa, ret >= 0); + + if (ret >= 0) + return ret; + + wpa_dbg(wpa_s, MSG_DEBUG, "wpa_drv_send_eapol rv (%d)", ret); if (wpa_s->l2) { return l2_packet_send(wpa_s->l2, dest, proto, buf, len);