diff mbox series

[1/2] wpa_supplicant: Handle randomization changes for same ESS

Message ID 20221201154344.620775-1-amo@semihalf.com
State Changes Requested
Headers show
Series [1/2] wpa_supplicant: Handle randomization changes for same ESS | expand

Commit Message

Andrzej Ostruszka Dec. 1, 2022, 3:43 p.m. UTC
When MAC randomization settings change we should use new MAC address
even if we are associating to the same ESS.  For example consider this
scenario:
- hardware MAC is being used,
- we disconnect from the network,
- policy/style is changed via D-Bus to turn randomization on,
- we reconnect to the same network.

In the last step the randomized address should be used.

Changes to the randomization settings include both changes to the
policy/style to be used and changes to the pregenerated MAC address
value in case of mac_addr==3.

Signed-off-by: Andrzej Ostruszka <amo@semihalf.com>
---
 wpa_supplicant/wpa_supplicant.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Jouni Malinen Dec. 2, 2022, 10:49 a.m. UTC | #1
On Thu, Dec 01, 2022 at 04:43:43PM +0100, Andrzej Ostruszka wrote:
> When MAC randomization settings change we should use new MAC address
> even if we are associating to the same ESS.  For example consider this
> scenario:
> - hardware MAC is being used,
> - we disconnect from the network,
> - policy/style is changed via D-Bus to turn randomization on,
> - we reconnect to the same network.
> 
> In the last step the randomized address should be used.
> 
> Changes to the randomization settings include both changes to the
> policy/style to be used and changes to the pregenerated MAC address
> value in case of mac_addr==3.

That sounds reasonable, but one of the changes seems problematic:

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -2436,7 +2445,7 @@ void wpa_supplicant_associate(struct wpa_supplicant *wpa_s,
>  	wpa_s_setup_sae_pt(wpa_s->conf, ssid);
>  #endif /* CONFIG_SAE */
>  
> -	if (rand_style > 0 && !wpa_s->reassoc_same_ess) {
> +	if (rand_style > 0) {
>  		if (wpas_update_random_addr(wpa_s, rand_style, ssid) < 0)
>  			return;
>  		wpa_sm_pmksa_cache_flush(wpa_s->wpa, ssid);

Wouldn't this disable PMKSA caching completely for all rand_style > 0
cases? In particular, this flushing of the PMKSA cache entries seems
undesired for rand_style==3 when reassociating within the ESS using the
same MAC address.
Andrzej Ostruszka Dec. 5, 2022, 5:04 p.m. UTC | #2
On Fri, Dec 02, 2022 at 12:49:13PM +0200, Jouni Malinen wrote:
[...]
> > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> > @@ -2436,7 +2445,7 @@ void wpa_supplicant_associate(struct wpa_supplicant *wpa_s,
> >  	wpa_s_setup_sae_pt(wpa_s->conf, ssid);
> >  #endif /* CONFIG_SAE */
> >  
> > -	if (rand_style > 0 && !wpa_s->reassoc_same_ess) {
> > +	if (rand_style > 0) {
> >  		if (wpas_update_random_addr(wpa_s, rand_style, ssid) < 0)
> >  			return;
> >  		wpa_sm_pmksa_cache_flush(wpa_s->wpa, ssid);
> 
> Wouldn't this disable PMKSA caching completely for all rand_style > 0
> cases? In particular, this flushing of the PMKSA cache entries seems
> undesired for rand_style==3 when reassociating within the ESS using the
> same MAC address.

You are right.  I'll change wpas_update_random_addr() to return "> 0"
when the address has been changed, "0" for no change and "< 0" on error
(like it is now) and will flush only for "> 0".

BTW shouldn't we also clear cache when restoring hardware MAC?

With regards
Andrzej Ostruszka
Jouni Malinen Dec. 5, 2022, 5:16 p.m. UTC | #3
> On 5. Dec 2022, at 19.04, Andrzej Ostruszka <amo@semihalf.com> wrote:
> 
> On Fri, Dec 02, 2022 at 12:49:13PM +0200, Jouni Malinen wrote:
>> Wouldn't this disable PMKSA caching completely for all rand_style > 0
>> cases? In particular, this flushing of the PMKSA cache entries seems
>> undesired for rand_style==3 when reassociating within the ESS using the
>> same MAC address.
> 
> You are right.  I'll change wpas_update_random_addr() to return "> 0"
> when the address has been changed, "0" for no change and "< 0" on error
> (like it is now) and will flush only for "> 0".
> 
> BTW shouldn't we also clear cache when restoring hardware MAC?

The PMKSA cache entries are cleared mainly to avoid exposing the same PMKID when using MAC address randomization for privacy protection. In addition, entries are removed when something changes in the local configuration if that change might have an impact on the authentication or key derivation.

I recently added more checks on the local address that was used when a PMKSA cache entry was aded, so likely the only remaining reason to flush entries on address change would be to free some resources when using random MAC addresses with no plan on restoring a previously used address. For cases where a previously used MAC address is restored (whether a globally unique one or a per-ESS random one) for connection purposes, the PMKSA cache entries should not really be removed since use of the same PMKID does not reveal more than use of the same MAC address about the device and wpa_supplicant uses an entry for PMKSA caching only if the currently used address matches the one that was used to generate the PMKSA.

- Jouni
Andrzej Ostruszka Dec. 15, 2022, 3:39 p.m. UTC | #4
Firstly apologies for the delayed reply.

On Mon, Dec 05, 2022 at 07:16:18PM +0200, Jouni Malinen wrote:
> > On 5. Dec 2022, at 19.04, Andrzej Ostruszka <amo@semihalf.com> wrote:
[...]
> 
> For cases where a previously used MAC address is restored
> (whether a globally unique one or a per-ESS random one) for connection
> purposes, the PMKSA cache entries should not really be removed since
> use of the same PMKID does not reveal more than use of the same MAC
> address about the device and wpa_supplicant uses an entry for PMKSA
> caching only if the currently used address matches the one that was
> used to generate the PMKSA.

Got it.  I was just worried about some corner cases like e.g. user used
policy/style 3 but then decided to turn it off and we are now
reconnecting to the same ESS and want to restore hardware address.

Regards
Andrzej Ostruszka
diff mbox series

Patch

diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
index 9c711d154..349933c39 100644
--- a/wpa_supplicant/wpa_supplicant.c
+++ b/wpa_supplicant/wpa_supplicant.c
@@ -2238,14 +2238,23 @@  int wpas_update_random_addr(struct wpa_supplicant *wpa_s, int style,
 
 	os_get_reltime(&now);
 	if (wpa_s->last_mac_addr_style == style &&
-	    /* Pregenerated addresses do not expire */
-	    wpa_s->last_mac_addr_style != 3 &&
-	    wpa_s->last_mac_addr_change.sec != 0 &&
-	    !os_reltime_expired(&now, &wpa_s->last_mac_addr_change,
-				wpa_s->conf->rand_addr_lifetime)) {
-		wpa_msg(wpa_s, MSG_DEBUG,
-			"Previously selected random MAC address has not yet expired");
-		return 0;
+	    /* Random addresses are valid within given ESS so do not check
+	     * expiration/value when changing ESS. */
+	    wpa_s->reassoc_same_ess) {
+		if (style != 3) {
+			if (wpa_s->last_mac_addr_change.sec != 0 &&
+			    !os_reltime_expired(&now, &wpa_s->last_mac_addr_change,
+						wpa_s->conf->rand_addr_lifetime)) {
+				wpa_msg(wpa_s, MSG_DEBUG,
+					"Previously selected random MAC address has not yet expired");
+				return 0;
+			}
+		} else {
+			/* Pregenerated addresses do not expire but their value
+			 * might have changed, so let's check that. */
+			if (os_memcmp(wpa_s->own_addr, ssid->mac_value, ETH_ALEN) == 0)
+				return 0;
+		}
 	}
 
 	switch (style) {
@@ -2436,7 +2445,7 @@  void wpa_supplicant_associate(struct wpa_supplicant *wpa_s,
 	wpa_s_setup_sae_pt(wpa_s->conf, ssid);
 #endif /* CONFIG_SAE */
 
-	if (rand_style > 0 && !wpa_s->reassoc_same_ess) {
+	if (rand_style > 0) {
 		if (wpas_update_random_addr(wpa_s, rand_style, ssid) < 0)
 			return;
 		wpa_sm_pmksa_cache_flush(wpa_s->wpa, ssid);