diff mbox series

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

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

Commit Message

Peer, Ilan Feb. 24, 2020, 9:15 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     | 67 +++++++++++++++++++++--------------------
 tests/hwsim/test_sae.py | 12 ++++----
 6 files changed, 49 insertions(+), 46 deletions(-)

Comments

Jouni Malinen Feb. 25, 2020, 11:21 a.m. UTC | #1 
On Mon, Feb 24, 2020 at 11:15:55AM +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.

Why would these need to be renamed? Sure, it might be cleaner, but this
results in changes that do not need strictly speaking required and also
to changes that are problematic. Using 

> diff --git a/hostapd/config_file.c b/hostapd/config_file.c
> @@ -4198,8 +4198,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);

For example, this change of a configuration parameter name would break
all existing deployments when they update hostapd. That does not look
acceptable.
Peer, Ilan Feb. 25, 2020, 12:10 p.m. UTC | #2 
> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Tuesday, February 25, 2020 13:21
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH 07/14] AP: Rename SAE anti clogging variables and
> functions
> 
> On Mon, Feb 24, 2020 at 11:15:55AM +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.
> 
> Why would these need to be renamed? Sure, it might be cleaner, but this
> results in changes that do not need strictly speaking required and also to
> changes that are problematic. Using
> 
> > diff --git a/hostapd/config_file.c b/hostapd/config_file.c @@ -4198,8
> > +4198,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);
> 
> For example, this change of a configuration parameter name would break all
> existing deployments when they update hostapd. That does not look
> acceptable.
> 

My bad.

I would still prefer to have the renaming of internal functions etc. if possible and only leave the external API unchanged. If you think otherwise, I can drop this patch and align the following patches. Let me know what you prefer.

Thanks for taking a look at the patches,

Ilan.
Jouni Malinen Feb. 29, 2020, 10:31 p.m. UTC | #3 
On Tue, Feb 25, 2020 at 12:10:39PM +0000, Peer, Ilan wrote:
> I would still prefer to have the renaming of internal functions etc. if possible and only leave the external API unchanged. If you think otherwise, I can drop this patch and align the following patches. Let me know what you prefer.

That might be fine. However, are you sure that these values will be
shared for both SAE and PASN? Is there any case where the same STA might
be using both SAE and PASN in parallel? Could that result into any
unexpected behavior if there is only one set of anti-clogging parameters
and state?
Peer, Ilan March 1, 2020, 9:16 a.m. UTC | #4 
> -----Original Message-----
> From: Jouni Malinen <j@w1.fi>
> Sent: Sunday, March 01, 2020 00:32
> To: Peer, Ilan <ilan.peer@intel.com>
> Cc: hostap@lists.infradead.org
> Subject: Re: [PATCH 07/14] AP: Rename SAE anti clogging variables and
> functions
> 
> On Tue, Feb 25, 2020 at 12:10:39PM +0000, Peer, Ilan wrote:
> > I would still prefer to have the renaming of internal functions etc. if
> possible and only leave the external API unchanged. If you think otherwise, I
> can drop this patch and align the following patches. Let me know what you
> prefer.
> 
> That might be fine. However, are you sure that these values will be shared
> for both SAE and PASN? Is there any case where the same STA might be
> using both SAE and PASN in parallel? Could that result into any unexpected
> behavior if there is only one set of anti-clogging parameters and state?
> 

I cannot really guarantee that such a thing would be adequately implemented
by stations. As the anti-clogging token indexing  is based on the station address
I can extend comeback_token_hash() to concatenate the authentication algorithm
ID with the address, to allow concurrent support for SAE and PASN. What do you think?

Regards,

Ilan.
Jouni Malinen March 1, 2020, 3:23 p.m. UTC | #5 
On Sun, Mar 01, 2020 at 09:16:40AM +0000, Peer, Ilan wrote:
> I cannot really guarantee that such a thing would be adequately implemented
> by stations. As the anti-clogging token indexing  is based on the station address
> I can extend comeback_token_hash() to concatenate the authentication algorithm
> ID with the address, to allow concurrent support for SAE and PASN. What do you think?

The more I try to understand how comeback cookie mechanism in
P802.11az/D2.0 is supposed to work, the more I start to think that it
should really be designed differently.. This is similar to the SAE
anti-clogging token design and that design is known to not really
provide much protection since it does not require any significant
calculation need on the attacker side. With that in mind, I'm not sure I
have a good answer on how the current design should be implemented since
I hope the current design changes before the implementation was this
would be added.. Anyway, if we do need to move ahead with the current
design, it would likely be a good idea to make the tokens distinct from
the ones used in SAE.
Peer, Ilan March 1, 2020, 8:37 p.m. UTC | #6 
On Sun, 2020-03-01 at 17:23 +0200, Jouni Malinen wrote:
> On Sun, Mar 01, 2020 at 09:16:40AM +0000, Peer, Ilan wrote:
> > I cannot really guarantee that such a thing would be adequately
> > implemented
> > by stations. As the anti-clogging token indexing  is based on the station
> > address
> > I can extend comeback_token_hash() to concatenate the authentication
> > algorithm
> > ID with the address, to allow concurrent support for SAE and PASN. What do
> > you think?
> 
> The more I try to understand how comeback cookie mechanism in
> P802.11az/D2.0 is supposed to work, the more I start to think that it
> should really be designed differently.. This is similar to the SAE
> anti-clogging token design and that design is known to not really
> provide much protection since it does not require any significant
> calculation need on the attacker side. 

Yep. That's the main reason why I wanted to share the implementation.
 
> With that in mind, I'm not sure I
> have a good answer on how the current design should be implemented since
> I hope the current design changes before the implementation was this
> would be added.. Anyway, if we do need to move ahead with the current
> design, it would likely be a good idea to make the tokens distinct from
> the ones used in SAE.
> 

I'll take this path unless the specification changes to address your concerns.

Regards,

Ilan.
diff mbox series

Patch

diff --git a/hostapd/config_file.c b/hostapd/config_file.c
index 66d40b669d..0714a22edf 100644
--- a/hostapd/config_file.c
+++ b/hostapd/config_file.c
@@ -4198,8 +4198,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 b1bbd0bc62..fb06b30a93 100644
--- a/src/ap/ap_config.c
+++ b/src/ap/ap_config.c
@@ -112,7 +112,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 0ceccccdc6..cbdbf8af04 100644
--- a/src/ap/ap_config.h
+++ b/src/ap/ap_config.h
@@ -653,7 +653,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 11913aaddd..ddac7a5f89 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 f2f27bf8e7..68723ea1fb 100644
--- a/src/ap/ieee802_11.c
+++ b/src/ap/ieee802_11.c
@@ -642,12 +642,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) {
@@ -657,7 +657,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;
 	}
 
@@ -665,25 +665,25 @@  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 u8 sae_token_hash(struct hostapd_data *hapd, const u8 *addr)
+static u8 comeback_token_hash(struct hostapd_data *hapd, const u8 *addr)
 {
 	u8 hash[SHA256_MAC_LEN];
 
-	hmac_sha256(hapd->sae_token_key, sizeof(hapd->sae_token_key),
+	hmac_sha256(hapd->comeback_key, sizeof(hapd->comeback_key),
 		    addr, ETH_ALEN, hash);
 	return hash[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];
@@ -693,10 +693,11 @@  static int check_sae_token(struct hostapd_data *hapd, const u8 *addr,
 
 	if (token_len != SHA256_MAC_LEN)
 		return -1;
-	idx = sae_token_hash(hapd, addr);
-	token_idx = hapd->sae_pending_token_idx[idx];
+	idx = comeback_token_hash(hapd, addr);
+	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;
@@ -706,12 +707,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;
 }
@@ -730,18 +731,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);
@@ -757,12 +759,12 @@  static struct wpabuf * auth_build_token_req(struct hostapd_data *hapd,
 		wpabuf_put_u8(buf, WLAN_EID_EXT_ANTI_CLOGGING_TOKEN);
 	}
 
-	p_idx = sae_token_hash(hapd, addr);
-	token_idx = hapd->sae_pending_token_idx[p_idx];
+	p_idx = comeback_token_hash(hapd, addr);
+	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);
@@ -770,7 +772,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;
@@ -1382,7 +1384,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,
@@ -1401,7 +1404,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 a55bd07ef8..d1a666fec4 100644
--- a/tests/hwsim/test_sae.py
+++ b/tests/hwsim/test_sae.py
@@ -260,7 +260,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 ")
@@ -281,7 +281,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):
@@ -295,7 +295,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")
@@ -1698,7 +1698,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):
@@ -2286,7 +2286,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:
@@ -2306,7 +2306,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: