On Sun, Dec 22, 2019 at 02:30:42PM +0100, Alexander Wetzel wrote: > This is not really "fixing" a connection in the current code. It just gets rid > of a avoidable scan in some cases. The next version for Extended Key ID > support patches depends on it or it breaks some owe connections when enabled > and it looks like some connections are currently only working due to another > decision or maybe a bug. (The IE mismatch detection issue outlined below.) That is not really a new scan, but a new instance of fetching the already existing scan results from the driver. Anyway, you are correct in this not being the expected code path to take here. It was just the proposed change that made me not understand what exactly is going on here.. I went through the debug log line by line to figure out what exactly happened. > Let's start with the reasoning why I think it should be changed: > wpa_supplicant_set_suites() can be called with bss set to NULL: > * @bss: Scan results for the selected BSS, or %NULL if not available wpa_supplicant_set_suites() should not be called at all in this case which is the main issue here.. That function is used when the driver instead of wpa_supplicant decides to roam to another BSS. That is not the case with OWE transition mode. > I'm pretty sure the intent for ap_ies_from_associnfo is to make sure we only > store ie data from a beacon in the state machine, to have them later available > in wpa_supplicant_validate_ie() to be verified against the IE from eapol#3. > wpa_supplicant_validate_ie() is by the way also assuming that the variables > are set and for some reason is not triggering a RSN mismatch when ap_wpa_ie or > ap_rsn_ie the state machine is NULL. (It's done for ap_rsnxe.) We may want to > fix that, too... I'll send you an alternate patch for this one including that, > too. But that may trigger disconnects with some broken APs and is a bit more > risky. The case of Beacon frame no including an RSNE sounds quite unlikely to be able to get to a state where RSN 4-way handshake is used since the RSNE from Beacon (or Probe Response) frame is the part that's used to negotiated use of RSN in the first place.. > With the code as it is today we just have some suboptimal cases we can see in > the following tests: > owe_transition_mode > owe_transition_mode_connect_cmd > owe_transition_mode_multi_bss > owe_transition_mode_open_multiple_scans > > Looking at owe_transition_mode.log0 from a test run we see the following order > of events: (I generously removed lines and just kept enough to see the picture. > The full logfile is available here: https://www.awhome.eu/index.php/s/jDQfpZmDGwsLPXA > and named cf28cfc12-orig-owe_transition_mode.log0) That location does not seem to be available anymore, but anyway, I think I understood most of it with one unclear, but not really directly related, item. > 321 wlan0: Request association with 02:00:00:00:03:00 > 337 wlan0: WPA: Selected cipher suites: group 16 pairwise 16 key_mgmt 4194304 proto 2 > 339 wlan0: WPA: clearing AP WPA IE > --- here we set the variable > 340 WPA: set AP RSN IE - hexdump(len=22): 30 14 01 00 00 0f ac 04 01 00 00 0f ac 04 01 00 00 0f ac 12 cc 00 > 341 wlan0: WPA: clearing AP RSNXE This is expected since wpa_supplicant has the updated BSS for the OWE BSS available at this point. > 508 wlan0: Associated to a new BSS: BSSID=02:00:00:00:03:00 > 511 wlan0: Network configuration found for the current AP That code path should not be entered at all.. The key debug entry that shows the incorrect behavior here is this one: Driver-initiated BSS selection changed the SSID to %s > 512 wlan0: WPA: Using WPA IE from AssocReq to set cipher suites > 513 wlan0: WPA: Selected cipher suites: group 16 pairwise 16 key_mgmt 4194304 proto 2 > --- here are the variables cleared without any good reason --- > 515 wlan0: WPA: clearing AP WPA IE > 516 wlan0: WPA: clearing AP RSN IE > 517 wlan0: WPA: clearing AP RSNXE Yes, none of this was supposed to happen since wpa_supplicant_set_suites() was not supposed to be called. > 631 wlan0: WPA: RX message 3 of 4-Way Handshake from 02:00:00:00:03:00 (ver=0) > --- here we need the deleted data again and trigger a scan the set the variables again --- > 636 wlan0: WPA: No WPA/RSN IE for this AP known. Trying to get from scan results This does not trigger a new scan; this is only fetching the current scan results from cfg80211. > 637 nl80211: Received scan results (3 BSSes) > 638 nl80211: Scan results indicate BSS status with 02:00:00:00:03:00 as associated > --- but we don't get them and as a result bypass the (also broken?) IE mismatch check > 647 wlan0: WPA: Could not find AP from the scan results That looks strange and is not something I see in my tests. That scan result has to be there in both wpa_supplicant and cfg80211 for the association to be started.. In practice, you should have seen this before association: nl80211: Trigger single channel scan to refresh cfg80211 BSS entry wlan0: nl80211: scan request nl80211: Scan SSID owe-random ... wlan0: nl80211: New scan results available nl80211: Scan results for missing cfg80211 BSS entry And now the entry for 02:00:00:00:03:00 with the correct (previously hidden) SSID is in place in cfg80211 (and it was already there with the correct SSID in wpa_supplicant BSS table). > >> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > >> @@ -1378,7 +1378,7 @@ int wpa_supplicant_set_suites(struct wpa_supplicant *wpa_s, > >> wpa_sm_set_param(wpa_s->wpa, WPA_PARAM_RSN_ENABLED, > >> !!(ssid->proto & (WPA_PROTO_RSN | WPA_PROTO_OSEN))); > >> > >> - if (bss || !wpa_s->ap_ies_from_associnfo) { > >> + if (bss && !wpa_s->ap_ies_from_associnfo) { > >> if (wpa_sm_set_ap_wpa_ie(wpa_s->wpa, bss_wpa, > >> bss_wpa ? 2 + bss_wpa[1] : 0) || So the reason I did not understand this (and well, still don't think this is correct) is that this function should never have been called in the first place. > I stopped digging into that after discovering that the bss is an optional > argument. But as explained above we must not update the state machine variables > with IEs from association requests. So I assume that was the intent of the code > and we just use the wrong logical operation for that. Which then deletes the > variables. > It could be that the correct fix is to make the bss mandatory or at least never > call wpa_supplicant_set_suites() when one of the variables are set... > But that's not my understanding of the code. Not calling wpa_supplicant_set_suites() is indeed the appropriate fix for this issue with OWE transition mode. [PATCH] OWE: Avoid incorrect profile update in transition mode The "unexpected" change of SSID between the current network profile (which uses the SSID from the open BSS in OWE transition mode) and the association with the OWE BSS (which uses a random, hidden SSID) resulted in wpa_supplicant incorrectly determining that this was a driver-initiated BSS selection ("Driver-initiated BSS selection changed the SSID to <the random SSID from OWE BSS>" in debug log). This ended up with updating security parameters based on the network profile inwpa_supplicant_set_suites() instead of using the already discovered information from scan results. In particular, this cleared the RSN supplicant state machine information of AP RSNE and resulted in having to fetch the scan results for the current BSS when processing EAPOL-Key msg 3/4. Fix this by recognizing the special case for OWE transition mode where the SSID for the associated AP does not actually match the SSID in the network profile. Signed-off-by: Jouni Malinen <j@xxxxx> --- wpa_supplicant/bss.h | 1 + wpa_supplicant/events.c | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/wpa_supplicant/bss.h b/wpa_supplicant/bss.h index 3ce8cd3f429a..0716761749f6 100644 --- a/wpa_supplicant/bss.h +++ b/wpa_supplicant/bss.h @@ -18,6 +18,7 @@ struct wpa_scan_res; #define WPA_BSS_AUTHENTICATED BIT(4) #define WPA_BSS_ASSOCIATED BIT(5) #define WPA_BSS_ANQP_FETCH_TRIED BIT(6) +#define WPA_BSS_OWE_TRANSITION BIT(7) struct wpa_bss_anqp_elem { struct dl_list list; diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index 37ae306bbb9e..1149aa90bacd 100644 --- a/wpa_supplicant/events.c +++ b/wpa_supplicant/events.c @@ -188,6 +188,16 @@ static int wpa_supplicant_select_config(struct wpa_supplicant *wpa_s) drv_ssid_len) == 0) return 0; /* current profile still in use */ +#ifdef CONFIG_OWE + if ((wpa_s->current_ssid->key_mgmt & WPA_KEY_MGMT_OWE) && + wpa_s->current_bss && + (wpa_s->current_bss->flags & WPA_BSS_OWE_TRANSITION) && + drv_ssid_len == wpa_s->current_bss->ssid_len && + os_memcmp(drv_ssid, wpa_s->current_bss->ssid, + drv_ssid_len) == 0) + return 0; /* current profile still in use */ +#endif /* CONFIG_OWE */ + wpa_msg(wpa_s, MSG_DEBUG, "Driver-initiated BSS selection changed the SSID to %s", wpa_ssid_txt(drv_ssid, drv_ssid_len)); @@ -1025,6 +1035,7 @@ static void owe_trans_ssid(struct wpa_supplicant *wpa_s, struct wpa_bss *bss, wpa_ssid_txt(pos, ssid_len)); os_memcpy(bss->ssid, pos, ssid_len); bss->ssid_len = ssid_len; + bss->flags |= WPA_BSS_OWE_TRANSITION; #endif /* CONFIG_OWE */ } -- 2.20.1 -- Jouni Malinen PGP id EFC895FA _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
