diff mbox series

[v2,07/14] AP: Rename SAE anti clogging variables and functions

Message ID 20201216110105.8380-8-ilan.peer@intel.com
State Changes Requested
Headers show
Series Support PASN with SAE, FILS and FT | expand

Commit Message

Peer, Ilan Dec. 16, 2020, 11 a.m. UTC 
PASN authentication mandates support for comeback flow, which
among others can be used for anti-clogging purposes.

As the SAE support for anti clogging can also be used for PASN,
start modifying the source code so the anti clogging support
can be used for both SAE and PASN.

As a start, rename some variables/functions etc. so that they
would not be SAE specific.

Signed-off-by: Ilan Peer <ilan.peer@intel.com>
---
 hostapd/config_file.c   |  4 +--
 src/ap/ap_config.c      |  2 +-
 src/ap/ap_config.h      |  2 +-
 src/ap/hostapd.h        |  8 ++---
 src/ap/ieee802_11.c     | 76 +++++++++++++++++++++++------------------
 tests/hwsim/test_sae.py | 12 +++----
 6 files changed, 56 insertions(+), 48 deletions(-)

Comments

Jouni Malinen Jan. 25, 2021, 10:56 p.m. UTC | #1 
On Wed, Dec 16, 2020 at 01:00:58PM +0200, Ilan Peer wrote:
> PASN authentication mandates support for comeback flow, which
> among others can be used for anti-clogging purposes.
> 
> As the SAE support for anti clogging can also be used for PASN,
> start modifying the source code so the anti clogging support
> can be used for both SAE and PASN.
> 
> As a start, rename some variables/functions etc. so that they
> would not be SAE specific.

> diff --git a/hostapd/config_file.c b/hostapd/config_file.c
> @@ -4203,8 +4203,8 @@ static int hostapd_config_fill(struct hostapd_config *conf,

> -	} else if (os_strcmp(buf, "sae_anti_clogging_threshold") == 0) {
> -		bss->sae_anti_clogging_threshold = atoi(pos);
> +	} else if (os_strcmp(buf, "anti_clogging_threshold") == 0) {
> +		bss->anti_clogging_threshold = atoi(pos);

This does not look reasonable since it would break backwards
compatibility with existing hostapd SAE configuration and would prevent
updated hostapd from starting with previously working configuration.
Furthermore, this does not update hostapd/hostapd.conf to match. I'll
drop this part about renaming the configuration variable itself.

> diff --git a/tests/hwsim/test_sae.py b/tests/hwsim/test_sae.py
> @@ -256,7 +256,7 @@ def test_sae_anti_clogging(dev, apdev):
> -    params['sae_anti_clogging_threshold'] = '1'
> +    params['anti_clogging_threshold'] = '1'

I'll drop these changes to test_sae.py since they should not be needed
with backwards compatible manner of handling configuration parameter
names.
Peer, Ilan Jan. 26, 2021, 7:01 a.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Tuesday, January 26, 2021 00:56
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH v2 07/14] AP: Rename SAE anti clogging variables and
> functions
> 
> On Wed, Dec 16, 2020 at 01:00:58PM +0200, Ilan Peer wrote:
> > PASN authentication mandates support for comeback flow, which among
> > others can be used for anti-clogging purposes.
> >
> > As the SAE support for anti clogging can also be used for PASN, start
> > modifying the source code so the anti clogging support can be used for
> > both SAE and PASN.
> >
> > As a start, rename some variables/functions etc. so that they would
> > not be SAE specific.
> 
> > diff --git a/hostapd/config_file.c b/hostapd/config_file.c @@ -4203,8
> > +4203,8 @@ static int hostapd_config_fill(struct hostapd_config *conf,
> 
> > -	} else if (os_strcmp(buf, "sae_anti_clogging_threshold") == 0) {
> > -		bss->sae_anti_clogging_threshold = atoi(pos);
> > +	} else if (os_strcmp(buf, "anti_clogging_threshold") == 0) {
> > +		bss->anti_clogging_threshold = atoi(pos);
> 
> This does not look reasonable since it would break backwards compatibility
> with existing hostapd SAE configuration and would prevent updated hostapd
> from starting with previously working configuration.
> Furthermore, this does not update hostapd/hostapd.conf to match. I'll drop
> this part about renaming the configuration variable itself.
> 

Agree. I did not properly take this into consideration.

> > diff --git a/tests/hwsim/test_sae.py b/tests/hwsim/test_sae.py @@
> > -256,7 +256,7 @@ def test_sae_anti_clogging(dev, apdev):
> > -    params['sae_anti_clogging_threshold'] = '1'
> > +    params['anti_clogging_threshold'] = '1'
> 
> I'll drop these changes to test_sae.py since they should not be needed with
> backwards compatible manner of handling configuration parameter names.
> 

Sure.

So eventually this patch would end up by only renaming internal functions and
variables? Are you introducing another configuration option to also set the
PASN anti clogging?

Regards,

Ilan.
Jouni Malinen Jan. 26, 2021, 11 a.m. UTC | #3 
On Tue, Jan 26, 2021 at 07:01:26AM +0000, Peer, Ilan wrote:
> So eventually this patch would end up by only renaming internal functions and
> variables? Are you introducing another configuration option to also set the
> PASN anti clogging?

I was first planning to just rename the internal functions and variables
as part of these patch series. However, now that I thought more about
the PASN comeback mechanism after your replies and after having re-read
P802.11ax/D2.6, I think I'll just skip all the patches related to the
comeback mechanism for now to get the rest of the patch series applied.
It is starting look clear that the comeback mechanism needs more thought
on both the AP and the station sides.

For the station side (patch 10/14), I see no point in using external
mechanism for the special case mentioned in the draft, i.e., when
Comeback After value of 0 is used. That is just like the SAE
anti-clogging mechanism, i.e., immediate retry with the cookie, and that
needs to happen automatically within wpa_supplicant. The case with
nonzero Comeback After value seems reasonable to handle external based
on your explanation.

For the AP side, this PASN mechanism is much more flexible than the SAE
one since there is an explicit indication of Comeback After and the
reasons for using this can be quite different. As such, it may not be
reasonable to share the same configuration parameter for this with SAE.
Furthermore, the hardcoded value of 10 TUs for Comeback After may not be
the best approach (0 TUs would sounds like a better default behavior,
but there may be other reasons for the AP to behave more dynamically in
this area).. And that was actually incorrectly documented in
wpa_pasn_add_parameter_ie() as being seconds, not TUs..
Peer, Ilan Jan. 26, 2021, 11:56 a.m. UTC | #4 
Hi,

> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Tuesday, January 26, 2021 13:01
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH v2 07/14] AP: Rename SAE anti clogging variables and
> functions
> 
> On Tue, Jan 26, 2021 at 07:01:26AM +0000, Peer, Ilan wrote:
> > So eventually this patch would end up by only renaming internal
> > functions and variables? Are you introducing another configuration
> > option to also set the PASN anti clogging?
> 
> I was first planning to just rename the internal functions and variables as part
> of these patch series. However, now that I thought more about the PASN
> comeback mechanism after your replies and after having re-read
> P802.11ax/D2.6, I think I'll just skip all the patches related to the comeback
> mechanism for now to get the rest of the patch series applied.
> It is starting look clear that the comeback mechanism needs more thought on
> both the AP and the station sides.
> 

Ok. AFAIU, you are referring to the implementation details and not the specification.

> For the station side (patch 10/14), I see no point in using external mechanism
> for the special case mentioned in the draft, i.e., when Comeback After value
> of 0 is used. That is just like the SAE anti-clogging mechanism, i.e., immediate
> retry with the cookie, and that needs to happen automatically within
> wpa_supplicant. The case with nonzero Comeback After value seems
> reasonable to handle external based on your explanation.
> 

I agree that the special case with the comeback ater 0 can be handled in the
wpa_supplicant. I can add such implementation once you are done with
the other patches.

> For the AP side, this PASN mechanism is much more flexible than the SAE
> one since there is an explicit indication of Comeback After and the reasons
> for using this can be quite different. As such, it may not be reasonable to
> share the same configuration parameter for this with SAE.

I thought that adopting the same approach as with SAE, at least in terms of
token generation and tracking would be good enough as an initial implementation,
which can be later be improved.

> Furthermore, the hardcoded value of 10 TUs for Comeback After may not be
> the best approach (0 TUs would sounds like a better default behavior, but
> there may be other reasons for the AP to behave more dynamically in this
> area).. And that was actually incorrectly documented in
> wpa_pasn_add_parameter_ie() as being seconds, not TUs..
> 

Agree. Again, that was only an initial implementation 😊 As with the case of
wpa_supplicant, I can also add a more comprehensive implementation also
for the AP side. 

Ilan.
diff mbox series

Patch

diff --git a/hostapd/config_file.c b/hostapd/config_file.c
index 436a052aaf..ba7c348388 100644
--- a/hostapd/config_file.c
+++ b/hostapd/config_file.c
@@ -4203,8 +4203,8 @@  static int hostapd_config_fill(struct hostapd_config *conf,
 	} else if (os_strcmp(buf, "assocresp_elements") == 0) {
 		if (parse_wpabuf_hex(line, buf, &bss->assocresp_elements, pos))
 			return 1;
-	} else if (os_strcmp(buf, "sae_anti_clogging_threshold") == 0) {
-		bss->sae_anti_clogging_threshold = atoi(pos);
+	} else if (os_strcmp(buf, "anti_clogging_threshold") == 0) {
+		bss->anti_clogging_threshold = atoi(pos);
 	} else if (os_strcmp(buf, "sae_sync") == 0) {
 		bss->sae_sync = atoi(pos);
 	} else if (os_strcmp(buf, "sae_groups") == 0) {
diff --git a/src/ap/ap_config.c b/src/ap/ap_config.c
index 84d13512bd..502a68f743 100644
--- a/src/ap/ap_config.c
+++ b/src/ap/ap_config.c
@@ -121,7 +121,7 @@  void hostapd_config_defaults_bss(struct hostapd_bss_config *bss)
 
 	bss->radius_das_time_window = 300;
 
-	bss->sae_anti_clogging_threshold = 5;
+	bss->anti_clogging_threshold = 5;
 	bss->sae_sync = 5;
 
 	bss->gas_frag_limit = 1400;
diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h
index 7e7b6a8cc0..9652a1ced8 100644
--- a/src/ap/ap_config.h
+++ b/src/ap/ap_config.h
@@ -655,7 +655,7 @@  struct hostapd_bss_config {
 	struct wpabuf *vendor_elements;
 	struct wpabuf *assocresp_elements;
 
-	unsigned int sae_anti_clogging_threshold;
+	unsigned int anti_clogging_threshold;
 	unsigned int sae_sync;
 	int sae_require_mfp;
 	int sae_confirm_immediate;
diff --git a/src/ap/hostapd.h b/src/ap/hostapd.h
index 80a34015e2..58feae17ac 100644
--- a/src/ap/hostapd.h
+++ b/src/ap/hostapd.h
@@ -327,10 +327,10 @@  struct hostapd_data {
 
 #ifdef CONFIG_SAE
 	/** Key used for generating SAE anti-clogging tokens */
-	u8 sae_token_key[8];
-	struct os_reltime last_sae_token_key_update;
-	u16 sae_token_idx;
-	u16 sae_pending_token_idx[256];
+	u8 comeback_key[8];
+	struct os_reltime last_comeback_key_update;
+	u16 comeback_idx;
+	u16 comeback_pending_idx[256];
 	int dot11RSNASAERetransPeriod; /* msec */
 	struct dl_list sae_commit_queue; /* struct hostapd_sae_commit_queue */
 #endif /* CONFIG_SAE */
diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
index 5c54da3cb9..0a24e297ff 100644
--- a/src/ap/ieee802_11.c
+++ b/src/ap/ieee802_11.c
@@ -685,12 +685,12 @@  static int auth_sae_send_confirm(struct hostapd_data *hapd,
 }
 
 
-static int use_sae_anti_clogging(struct hostapd_data *hapd)
+static int use_anti_clogging(struct hostapd_data *hapd)
 {
 	struct sta_info *sta;
 	unsigned int open = 0;
 
-	if (hapd->conf->sae_anti_clogging_threshold == 0)
+	if (hapd->conf->anti_clogging_threshold == 0)
 		return 1;
 
 	for (sta = hapd->sta_list; sta; sta = sta->next) {
@@ -700,7 +700,7 @@  static int use_sae_anti_clogging(struct hostapd_data *hapd)
 		    sta->sae->state != SAE_CONFIRMED)
 			continue;
 		open++;
-		if (open >= hapd->conf->sae_anti_clogging_threshold)
+		if (open >= hapd->conf->anti_clogging_threshold)
 			return 1;
 	}
 
@@ -708,27 +708,29 @@  static int use_sae_anti_clogging(struct hostapd_data *hapd)
 	 * there are enough pending commit messages in the processing queue to
 	 * potentially result in too many open sessions. */
 	if (open + dl_list_len(&hapd->sae_commit_queue) >=
-	    hapd->conf->sae_anti_clogging_threshold)
+	    hapd->conf->anti_clogging_threshold)
 		return 1;
 
 	return 0;
 }
 
 
-static int sae_token_hash(struct hostapd_data *hapd, const u8 *addr, u8 *idx)
+static int comeback_token_hash(struct hostapd_data *hapd, const u8 *addr,
+			       u8 *idx)
 {
 	u8 hash[SHA256_MAC_LEN];
 
-	if (hmac_sha256(hapd->sae_token_key, sizeof(hapd->sae_token_key),
-			addr, ETH_ALEN, hash) < 0)
+	if (hmac_sha256(hapd->comeback_key, sizeof(hapd->comeback_key),
+		    addr, ETH_ALEN, hash) < 0)
 		return -1;
+
 	*idx = hash[0];
 	return 0;
 }
 
 
-static int check_sae_token(struct hostapd_data *hapd, const u8 *addr,
-			   const u8 *token, size_t token_len)
+static int check_comeback_token(struct hostapd_data *hapd, const u8 *addr,
+				const u8 *token, size_t token_len)
 {
 	u8 mac[SHA256_MAC_LEN];
 	const u8 *addrs[2];
@@ -736,11 +738,14 @@  static int check_sae_token(struct hostapd_data *hapd, const u8 *addr,
 	u16 token_idx;
 	u8 idx;
 
-	if (token_len != SHA256_MAC_LEN || sae_token_hash(hapd, addr, &idx) < 0)
-		return -1;
-	token_idx = hapd->sae_pending_token_idx[idx];
+	if (token_len != SHA256_MAC_LEN ||
+	    comeback_token_hash(hapd, addr, &idx) < 0)
+	    return -1;
+
+	token_idx = hapd->comeback_pending_idx[idx];
 	if (token_idx == 0 || token_idx != WPA_GET_BE16(token)) {
-		wpa_printf(MSG_DEBUG, "SAE: Invalid anti-clogging token from "
+		wpa_printf(MSG_DEBUG,
+			   "Comeback: Invalid anti-clogging token from "
 			   MACSTR " - token_idx 0x%04x, expected 0x%04x",
 			   MAC2STR(addr), WPA_GET_BE16(token), token_idx);
 		return -1;
@@ -750,12 +755,12 @@  static int check_sae_token(struct hostapd_data *hapd, const u8 *addr,
 	len[0] = ETH_ALEN;
 	addrs[1] = token;
 	len[1] = 2;
-	if (hmac_sha256_vector(hapd->sae_token_key, sizeof(hapd->sae_token_key),
+	if (hmac_sha256_vector(hapd->comeback_key, sizeof(hapd->comeback_key),
 			       2, addrs, len, mac) < 0 ||
 	    os_memcmp_const(token + 2, &mac[2], SHA256_MAC_LEN - 2) != 0)
 		return -1;
 
-	hapd->sae_pending_token_idx[idx] = 0; /* invalidate used token */
+	hapd->comeback_pending_idx[idx] = 0; /* invalidate used token */
 
 	return 0;
 }
@@ -774,18 +779,19 @@  static struct wpabuf * auth_build_token_req(struct hostapd_data *hapd,
 	u16 token_idx;
 
 	os_get_reltime(&now);
-	if (!os_reltime_initialized(&hapd->last_sae_token_key_update) ||
-	    os_reltime_expired(&now, &hapd->last_sae_token_key_update, 60) ||
-	    hapd->sae_token_idx == 0xffff) {
-		if (random_get_bytes(hapd->sae_token_key,
-				     sizeof(hapd->sae_token_key)) < 0)
+	if (!os_reltime_initialized(&hapd->last_comeback_key_update) ||
+	    os_reltime_expired(&now, &hapd->last_comeback_key_update, 60) ||
+	    hapd->comeback_idx == 0xffff) {
+		if (random_get_bytes(hapd->comeback_key,
+				     sizeof(hapd->comeback_key)) < 0)
 			return NULL;
-		wpa_hexdump(MSG_DEBUG, "SAE: Updated token key",
-			    hapd->sae_token_key, sizeof(hapd->sae_token_key));
-		hapd->last_sae_token_key_update = now;
-		hapd->sae_token_idx = 0;
-		os_memset(hapd->sae_pending_token_idx, 0,
-			  sizeof(hapd->sae_pending_token_idx));
+		wpa_hexdump(MSG_DEBUG,
+			    "Comeback: Updated token key",
+			    hapd->comeback_key, sizeof(hapd->comeback_key));
+		hapd->last_comeback_key_update = now;
+		hapd->comeback_idx = 0;
+		os_memset(hapd->comeback_pending_idx, 0,
+			  sizeof(hapd->comeback_pending_idx));
 	}
 
 	buf = wpabuf_alloc(sizeof(le16) + 3 + SHA256_MAC_LEN);
@@ -801,15 +807,16 @@  static struct wpabuf * auth_build_token_req(struct hostapd_data *hapd,
 		wpabuf_put_u8(buf, WLAN_EID_EXT_ANTI_CLOGGING_TOKEN);
 	}
 
-	if (sae_token_hash(hapd, addr, &p_idx) < 0) {
+	if (comeback_token_hash(hapd, addr, &p_idx) < 0) {
 		wpabuf_free(buf);
 		return NULL;
 	}
-	token_idx = hapd->sae_pending_token_idx[p_idx];
+
+	token_idx = hapd->comeback_pending_idx[p_idx];
 	if (!token_idx) {
-		hapd->sae_token_idx++;
-		token_idx = hapd->sae_token_idx;
-		hapd->sae_pending_token_idx[p_idx] = token_idx;
+		hapd->comeback_idx++;
+		token_idx = hapd->comeback_idx;
+		hapd->comeback_pending_idx[p_idx] = token_idx;
 	}
 	WPA_PUT_BE16(idx, token_idx);
 	token = wpabuf_put(buf, SHA256_MAC_LEN);
@@ -817,7 +824,7 @@  static struct wpabuf * auth_build_token_req(struct hostapd_data *hapd,
 	len[0] = ETH_ALEN;
 	addrs[1] = idx;
 	len[1] = sizeof(idx);
-	if (hmac_sha256_vector(hapd->sae_token_key, sizeof(hapd->sae_token_key),
+	if (hmac_sha256_vector(hapd->comeback_key, sizeof(hapd->comeback_key),
 			       2, addrs, len, token) < 0) {
 		wpabuf_free(buf);
 		return NULL;
@@ -1448,7 +1455,8 @@  static void handle_auth_sae(struct hostapd_data *hapd, struct sta_info *sta,
 			goto remove_sta;
 		}
 
-		if (token && check_sae_token(hapd, sta->addr, token, token_len)
+		if (token && check_comeback_token(hapd, sta->addr, token,
+						  token_len)
 		    < 0) {
 			wpa_printf(MSG_DEBUG, "SAE: Drop commit message with "
 				   "incorrect token from " MACSTR,
@@ -1465,7 +1473,7 @@  static void handle_auth_sae(struct hostapd_data *hapd, struct sta_info *sta,
 			goto reply;
 		}
 
-		if (!token && use_sae_anti_clogging(hapd) && !allow_reuse) {
+		if (!token && use_anti_clogging(hapd) && !allow_reuse) {
 			int h2e = 0;
 
 			wpa_printf(MSG_DEBUG,
diff --git a/tests/hwsim/test_sae.py b/tests/hwsim/test_sae.py
index 9276e513ec..057ad5fe06 100644
--- a/tests/hwsim/test_sae.py
+++ b/tests/hwsim/test_sae.py
@@ -256,7 +256,7 @@  def test_sae_anti_clogging(dev, apdev):
     check_sae_capab(dev[1])
     params = hostapd.wpa2_params(ssid="test-sae", passphrase="12345678")
     params['wpa_key_mgmt'] = 'SAE'
-    params['sae_anti_clogging_threshold'] = '1'
+    params['anti_clogging_threshold'] = '1'
     hostapd.add_ap(apdev[0], params)
 
     dev[0].request("SET sae_groups ")
@@ -277,7 +277,7 @@  def test_sae_forced_anti_clogging(dev, apdev):
     check_sae_capab(dev[1])
     params = hostapd.wpa2_params(ssid="test-sae", passphrase="12345678")
     params['wpa_key_mgmt'] = 'SAE WPA-PSK'
-    params['sae_anti_clogging_threshold'] = '0'
+    params['anti_clogging_threshold'] = '0'
     hostapd.add_ap(apdev[0], params)
     dev[2].connect("test-sae", psk="12345678", scan_freq="2412")
     for i in range(0, 2):
@@ -291,7 +291,7 @@  def test_sae_mixed(dev, apdev):
     check_sae_capab(dev[1])
     params = hostapd.wpa2_params(ssid="test-sae", passphrase="12345678")
     params['wpa_key_mgmt'] = 'SAE WPA-PSK'
-    params['sae_anti_clogging_threshold'] = '0'
+    params['anti_clogging_threshold'] = '0'
     hapd = hostapd.add_ap(apdev[0], params)
 
     dev[2].connect("test-sae", psk="12345678", scan_freq="2412")
@@ -1771,7 +1771,7 @@  def test_sae_forced_anti_clogging_pw_id(dev, apdev):
     check_sae_capab(dev[0])
     params = hostapd.wpa2_params(ssid="test-sae")
     params['wpa_key_mgmt'] = 'SAE'
-    params['sae_anti_clogging_threshold'] = '0'
+    params['anti_clogging_threshold'] = '0'
     params['sae_password'] = 'secret|id=' + 29*'A'
     hostapd.add_ap(apdev[0], params)
     for i in range(0, 2):
@@ -2401,7 +2401,7 @@  def test_sae_forced_anti_clogging_h2e(dev, apdev):
     params = hostapd.wpa2_params(ssid="test-sae", passphrase="12345678")
     params['wpa_key_mgmt'] = 'SAE WPA-PSK'
     params['sae_pwe'] = "1"
-    params['sae_anti_clogging_threshold'] = '0'
+    params['anti_clogging_threshold'] = '0'
     hostapd.add_ap(apdev[0], params)
     dev[2].connect("test-sae", psk="12345678", scan_freq="2412")
     try:
@@ -2421,7 +2421,7 @@  def test_sae_forced_anti_clogging_h2e_loop(dev, apdev):
     params = hostapd.wpa2_params(ssid="test-sae", passphrase="12345678")
     params['wpa_key_mgmt'] = 'SAE WPA-PSK'
     params['sae_pwe'] = "2"
-    params['sae_anti_clogging_threshold'] = '0'
+    params['anti_clogging_threshold'] = '0'
     hostapd.add_ap(apdev[0], params)
     dev[2].connect("test-sae", psk="12345678", scan_freq="2412")
     try: