Re: [PATCH] wpa_supplicant: Don't incorrectly clear ie scan data

>> 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..
> 

I was more thinking about an attack: Utilize the fact that 
wpa_supplicant can be tricked into clearing the variables and clear the 
way to a downgrade attack. (I don't see a real attack vector, just a way 
to bypass some of the RSN checks for OWE.)

>> 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.
> 

Yea, sorry. The share expired after one month. I shared it again: 
https://www.awhome.eu/index.php/s/fataWRskGzoqmFN

>> 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

Strange that it looks different in your test runs...

I used the git version cf28cfc12 to create the log and still have the 
full run on the disc. But I just tried it with the near-current version 
bdb2eaf87 and my normal kernel 5.5.7-gentoo kernel and it still was 
there for me. No patches applied at all to hostapd or mac80211/nl80211, 
only patched iwlwifi which is not even loaded during the test runs.

That said it's of course gone when I try ecb5219d8 where the RSN 
variables are no longer cleared.


> 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
> 

yes, I see that in the old log.

> 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.

I did not dig deeper into that and you know the code much better than me 
so I assume the issue is now fixed.
But what function has then ap_ies_from_associnfo? I assumed that the 
intend was to make sure only beacon RSN can be stored, so we have the 
beacon data handy when needed.

As it is I could not find any obvious protection to not update the 
variables with data taken from non-beacon frames, risking that when we 
try to compare e.g. an eapol RSN against a beacon we are using 
non-beacon data.

Alexander