[PATCH 1/5] ap: Set the group cipher when sending START_AP

[PATCH 1/5] ap: Set the group cipher when sending START_AP

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

Seems this overlooked because an initial version of ap.c didn't have
group traffic support.  This doesn't fix some brcmfmac problems where it
silently overrides the group cipher in the beacon frames though.
---
 src/ap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/ap.c b/src/ap.c
index ce639907..c4715696 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2108,7 +2108,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
 	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
 	uint32_t hidden_ssid = NL80211_HIDDEN_SSID_NOT_IN_USE;
-	uint32_t nl_ciphers = ie_rsn_cipher_suite_to_cipher(ap->ciphers);
+	uint32_t group_nl_cipher =
+		ie_rsn_cipher_suite_to_cipher(ap->group_cipher);
 	uint32_t nl_akm = CRYPTO_AKM_PSK;
 	uint32_t wpa_version = NL80211_WPA_VERSION_2;
 	uint32_t auth_type = NL80211_AUTHTYPE_OPEN_SYSTEM;
@@ -2148,6 +2149,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 				&hidden_ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITES_PAIRWISE, 4,
 				&nl_ciphers);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITE_GROUP, 4,
+				&group_nl_cipher);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_WPA_VERSIONS, 4, &wpa_version);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_AKM_SUITES, 4, &nl_akm);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_AUTH_TYPE, 4, &auth_type);
-- 
2.20.1

[PATCH 2/5] ap: Fix cleanup on ap_parse_new_station_ies errors

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

---
 src/ap.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index c4715696..1e028122 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2211,14 +2211,15 @@ static bool ap_parse_new_station_ies(const void *data, uint16_t len,
 	while (ie_tlv_iter_next(&iter)) {
 		switch (ie_tlv_iter_get_tag(&iter)) {
 		case IE_TYPE_RSN:
-			if (ie_parse_rsne(&iter, NULL) < 0)
+			if (rsn || ie_parse_rsne(&iter, NULL) < 0)
 				goto parse_error;
 
 			rsn = l_memdup(ie_tlv_iter_get_data(&iter) - 2,
 					ie_tlv_iter_get_length(&iter) + 2);
 			break;
 		case IE_TYPE_EXTENDED_SUPPORTED_RATES:
-			if (ap_parse_supported_rates(&iter, &rates) < 0)
+			if (rates || ap_parse_supported_rates(&iter, &rates) <
+					0)
 				goto parse_error;
 
 			break;
@@ -2257,13 +2258,16 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 	while (l_genl_attr_next(&attr, &type, &len, &data)) {
 		switch (type) {
 		case NL80211_ATTR_IE:
+			if (assoc_rsne || rates)
+				goto cleanup;
+
 			if (!ap_parse_new_station_ies(data, len, &assoc_rsne,
 							&rates))
 				return;
 			break;
 		case NL80211_ATTR_MAC:
 			if (len != 6)
-				return;
+				goto cleanup;
 
 			memcpy(mac, data, 6);
 			break;
@@ -2271,18 +2275,15 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 	}
 
 	if (!assoc_rsne || !rates)
-		return;
+		goto cleanup;
 
 	/*
 	 * Softmac's should already have a station created. The above check
 	 * may also fail for softmac cards.
 	 */
 	sta = l_queue_find(ap->sta_states, ap_sta_match_addr, mac);
-	if (sta) {
-		l_free(assoc_rsne);
-		l_uintset_free(rates);
-		return;
-	}
+	if (sta)
+		goto cleanup;
 
 	sta = l_new(struct sta_state, 1);
 	memcpy(sta->addr, mac, 6);
@@ -2307,6 +2308,12 @@ static void ap_handle_new_station(struct ap_state *ap, struct l_genl_msg *msg)
 		l_error("Issuing SET_STATION failed");
 		ap_del_station(sta, MMPDU_REASON_CODE_UNSPECIFIED, true);
 	}
+
+	return;
+
+cleanup:
+	l_free(assoc_rsne);
+	l_uintset_free(rates);
 }
 
 static void ap_handle_del_station(struct ap_state *ap, struct l_genl_msg *msg)
-- 
2.20.1

[PATCH 3/5] eapol: Remove assumption of single cipher in authenticator IE

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5550 bytes --]

Allow the user of the eapol_sm & handshake_state APIs to have multiple
pairwise ciphers listed in the authenticator IE.  This comes at the cost
of requiring the user to call handshake_state_override_pairwise_cipher
to select the cipher when setting up the handshake state.
---
 src/adhoc.c     |  1 +
 src/ap.c        |  2 ++
 src/eapol.c     | 20 +++++++++++++++-----
 src/handshake.c | 25 +++++++++++++++----------
 src/handshake.h |  2 ++
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/adhoc.c b/src/adhoc.c
index 7f9ce765..d83ed94e 100644
--- a/src/adhoc.c
+++ b/src/adhoc.c
@@ -257,6 +257,7 @@ static struct eapol_sm *adhoc_new_sm(struct sta_state *sta, bool authenticator,
 	handshake_state_set_authenticator_ie(hs, bss_rsne);
 	handshake_state_set_supplicant_ie(hs, bss_rsne);
 	handshake_state_set_pmk(hs, adhoc->pmk, 32);
+	handshake_state_override_pairwise_cipher(hs, adhoc->ciphers);
 
 	if (authenticator) {
 		handshake_state_set_authenticator_address(hs, own_addr);
diff --git a/src/ap.c b/src/ap.c
index 1e028122..1359dcac 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -724,6 +724,8 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
 	ie_build_rsne(&rsn, bss_rsne);
 	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
 
+	handshake_state_override_pairwise_cipher(sta->hs, ap->ciphers);
+
 	if (gtk_rsc)
 		handshake_state_set_gtk(sta->hs, sta->ap->gtk,
 					sta->ap->gtk_index, gtk_rsc);
diff --git a/src/eapol.c b/src/eapol.c
index c42f1a05..49abb105 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -1275,7 +1275,7 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 {
 	uint8_t frame_buf[512];
 	unsigned int rsne_len = sm->handshake->authenticator_ie[1] + 2;
-	uint8_t key_data_buf[128 + rsne_len];
+	uint8_t key_data_buf[128 + rsne_len * 2];
 	int key_data_len = rsne_len;
 	struct eapol_key *ek = (struct eapol_key *) frame_buf;
 	enum crypto_cipher cipher = ie_rsn_cipher_suite_to_cipher(
@@ -1284,6 +1284,8 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 				sm->handshake->group_cipher);
 	const uint8_t *kck;
 	const uint8_t *kek;
+	uint16_t mask = sm->handshake->supplicant_ciphers &
+		sm->handshake->authenticator_ciphers;
 
 	sm->replay_counter++;
 
@@ -1306,12 +1308,20 @@ static void eapol_send_ptk_3_of_4(struct eapol_sm *sm)
 	ek->key_rsc[6] = 0;
 	ek->key_rsc[7] = 0;
 
-	/*
-	 * Just one RSNE in Key Data as we only set one cipher in ap->ciphers
-	 * currently.
-	 */
 	memcpy(key_data_buf, sm->handshake->authenticator_ie, rsne_len);
 
+	/* See if we need to add the second RSNE */
+	if (sm->handshake->pairwise_cipher != mask) {
+		struct ie_rsn_info rsn_info;
+
+		rsn_info.pairwise_ciphers = sm->handshake->pairwise_cipher;
+
+		if (!ie_build_rsne(&rsn_info, key_data_buf + rsne_len))
+			return;
+
+		key_data_len += key_data_buf[rsne_len + 1] + 2;
+	}
+
 	if (group_cipher) {
 		uint8_t *gtk_kde = key_data_buf + key_data_len;
 
diff --git a/src/handshake.c b/src/handshake.c
index e76cf8fb..884b8f96 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -133,14 +133,10 @@ void handshake_state_set_8021x_config(struct handshake_state *s,
 static bool handshake_state_setup_own_ciphers(struct handshake_state *s,
 						const struct ie_rsn_info *info)
 {
-	if (__builtin_popcount(info->pairwise_ciphers) != 1)
-		return false;
-
 	if (__builtin_popcount(info->akm_suites) != 1)
 		return false;
 
 	s->akm_suite = info->akm_suites;
-	s->pairwise_cipher = info->pairwise_ciphers;
 	s->group_cipher = info->group_cipher;
 	s->group_management_cipher = info->group_management_cipher;
 
@@ -164,9 +160,6 @@ bool handshake_state_set_authenticator_ie(struct handshake_state *s,
 	s->wpa_ie = is_ie_wpa_ie(ie + 2, ie[1]);
 	s->osen_ie = is_ie_wfa_ie(ie + 2, ie[1], IE_WFA_OI_OSEN);
 
-	if (!s->authenticator)
-		return true;
-
 	if (s->wpa_ie) {
 		if (ie_parse_wpa_from_data(ie, ie[1] + 2, &info) < 0)
 			return false;
@@ -178,6 +171,11 @@ bool handshake_state_set_authenticator_ie(struct handshake_state *s,
 			return false;
 	}
 
+	s->authenticator_ciphers = info.pairwise_ciphers;
+
+	if (!s->authenticator)
+		return true;
+
 	return handshake_state_setup_own_ciphers(s, &info);
 }
 
@@ -191,9 +189,6 @@ bool handshake_state_set_supplicant_ie(struct handshake_state *s,
 	s->wpa_ie = is_ie_wpa_ie(ie + 2, ie[1]);
 	s->osen_ie = is_ie_wfa_ie(ie + 2, ie[1], IE_WFA_OI_OSEN);
 
-	if (s->authenticator)
-		return true;
-
 	if (s->wpa_ie) {
 		if (ie_parse_wpa_from_data(ie, ie[1] + 2, &info) < 0)
 			return false;
@@ -205,6 +200,16 @@ bool handshake_state_set_supplicant_ie(struct handshake_state *s,
 			return false;
 	}
 
+	s->supplicant_ciphers = info.pairwise_ciphers;
+
+	if (s->authenticator)
+		return true;
+
+	if (__builtin_popcount(info.pairwise_ciphers) != 1)
+		return false;
+
+	s->pairwise_cipher = info.pairwise_ciphers;
+
 	return handshake_state_setup_own_ciphers(s, &info);
 }
 
diff --git a/src/handshake.h b/src/handshake.h
index b738efd9..12aee921 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -88,6 +88,8 @@ struct handshake_state {
 	uint8_t *mde;
 	uint8_t *fte;
 	enum ie_rsn_cipher_suite pairwise_cipher;
+	enum ie_rsn_cipher_suite authenticator_ciphers;
+	enum ie_rsn_cipher_suite supplicant_ciphers;
 	enum ie_rsn_cipher_suite group_cipher;
 	enum ie_rsn_cipher_suite group_management_cipher;
 	enum ie_rsn_akm_suite akm_suite;
-- 
2.20.1

[PATCH 4/5] ap: Allow setting multiple ciphers in ap->ciphers

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

ap->ciphers is still set to one selected cipher currently but this
reduces the amount of work needed to resolve the TODO comment about
listing all supported ciphers in the pairwise ciphers RSNE fields.
---
 src/ap.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 1359dcac..86866f9d 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -709,7 +709,9 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
 	struct ap_state *ap = sta->ap;
 	const uint8_t *own_addr = netdev_get_address(ap->netdev);
 	struct ie_rsn_info rsn;
-	uint8_t bss_rsne[24];
+	uint8_t bss_rsne[64];
+	uint16_t mask;
+	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
 
 	handshake_state_set_ssid(sta->hs, (void *) ap->config->ssid,
 					strlen(ap->config->ssid));
@@ -724,7 +726,10 @@ static void ap_start_handshake(struct sta_state *sta, bool use_eapol_start,
 	ie_build_rsne(&rsn, bss_rsne);
 	handshake_state_set_authenticator_ie(sta->hs, bss_rsne);
 
-	handshake_state_override_pairwise_cipher(sta->hs, ap->ciphers);
+	mask = ap->ciphers;
+	mask &= sta->hs->supplicant_ciphers ?: 0xffff;
+	handshake_state_override_pairwise_cipher(sta->hs,
+					wiphy_select_cipher(wiphy, mask));
 
 	if (gtk_rsc)
 		handshake_state_set_gtk(sta->hs, sta->ap->gtk,
@@ -2110,6 +2115,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
 	struct wiphy *wiphy = netdev_get_wiphy(ap->netdev);
 	uint32_t hidden_ssid = NL80211_HIDDEN_SSID_NOT_IN_USE;
+	unsigned int nl_ciphers_cnt = __builtin_popcount(ap->ciphers);
+	uint32_t nl_ciphers[nl_ciphers_cnt];
 	uint32_t group_nl_cipher =
 		ie_rsn_cipher_suite_to_cipher(ap->group_cipher);
 	uint32_t nl_akm = CRYPTO_AKM_PSK;
@@ -2118,11 +2125,17 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 	uint32_t ch_freq = scan_channel_to_freq(ap->config->channel,
 						SCAN_BAND_2_4_GHZ);
 	uint32_t ch_width = NL80211_CHAN_WIDTH_20;
+	unsigned int i;
 
 	static const uint8_t bcast_addr[6] = {
 		0xff, 0xff, 0xff, 0xff, 0xff, 0xff
 	};
 
+	for (i = 0, nl_ciphers_cnt = 0; i < 8; i++)
+		if (ap->ciphers & (1 << i))
+			nl_ciphers[nl_ciphers_cnt++] =
+				ie_rsn_cipher_suite_to_cipher(1 << i);
+
 	head_len = ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
 						bcast_addr, head, sizeof(head));
 	tail_len = ap_build_beacon_pr_tail(ap, false, tail);
@@ -2149,8 +2162,8 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 				ap->config->ssid);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_HIDDEN_SSID, 4,
 				&hidden_ssid);
-	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITES_PAIRWISE, 4,
-				&nl_ciphers);
+	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITES_PAIRWISE,
+				nl_ciphers_cnt * 4, nl_ciphers);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_CIPHER_SUITE_GROUP, 4,
 				&group_nl_cipher);
 	l_genl_msg_append_attr(cmd, NL80211_ATTR_WPA_VERSIONS, 4, &wpa_version);
-- 
2.20.1

[PATCH 5/5] unit: Use hs_override_pairwise_cipher in eapol authenticator tests

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

---
 unit/test-eapol.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/unit/test-eapol.c b/unit/test-eapol.c
index eba249ed..6032fc5b 100644
--- a/unit/test-eapol.c
+++ b/unit/test-eapol.c
@@ -3682,6 +3682,8 @@ static void eapol_ap_sta_handshake_test(const void *data)
 	handshake_state_set_authenticator_ie(s.ap_hs, ap_rsne);
 	handshake_state_set_ssid(s.ap_hs, (void *) ssid, strlen(ssid));
 	handshake_state_set_pmk(s.ap_hs, psk, 32);
+	handshake_state_override_pairwise_cipher(s.ap_hs,
+						IE_RSN_CIPHER_SUITE_CCMP);
 
 	handshake_state_set_authenticator(s.sta_hs, false);
 	handshake_state_set_event_func(s.sta_hs, test_ap_sta_hs_event, &s);
@@ -3743,6 +3745,8 @@ static void eapol_ap_sta_handshake_bad_psk_test(const void *data)
 	handshake_state_set_authenticator_ie(s.ap_hs, ap_rsne);
 	handshake_state_set_ssid(s.ap_hs, (void *) ssid, strlen(ssid));
 	handshake_state_set_pmk(s.ap_hs, psk1, 32);
+	handshake_state_override_pairwise_cipher(s.ap_hs,
+						IE_RSN_CIPHER_SUITE_CCMP);
 
 	handshake_state_set_authenticator(s.sta_hs, false);
 	handshake_state_set_event_func(s.sta_hs, test_ap_sta_hs_event, &s);
@@ -3804,6 +3808,8 @@ static void eapol_ap_sta_handshake_ip_alloc_ok_test(const void *data)
 	handshake_state_set_authenticator_ie(s.ap_hs, ap_rsne);
 	handshake_state_set_ssid(s.ap_hs, (void *) ssid, strlen(ssid));
 	handshake_state_set_pmk(s.ap_hs, psk, 32);
+	handshake_state_override_pairwise_cipher(s.ap_hs,
+						IE_RSN_CIPHER_SUITE_CCMP);
 	s.ap_hs->support_ip_allocation = true;
 	s.ap_hs->client_ip_addr = 0x01020304;
 	s.ap_hs->subnet_mask = 0xffff0000;
@@ -3871,6 +3877,8 @@ static void eapol_ap_sta_handshake_ip_alloc_no_req_test(const void *data)
 	handshake_state_set_authenticator_ie(s.ap_hs, ap_rsne);
 	handshake_state_set_ssid(s.ap_hs, (void *) ssid, strlen(ssid));
 	handshake_state_set_pmk(s.ap_hs, psk, 32);
+	handshake_state_override_pairwise_cipher(s.ap_hs,
+						IE_RSN_CIPHER_SUITE_CCMP);
 	s.ap_hs->support_ip_allocation = true;
 	s.ap_hs->client_ip_addr = 0x01020304;
 	s.ap_hs->subnet_mask = 0xffff0000;
-- 
2.20.1

Re: [PATCH 1/5] ap: Set the group cipher when sending START_AP

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

Hi Andrew,

On 1/29/21 5:27 PM, Andrew Zaborowski wrote:
> Seems this overlooked because an initial version of ap.c didn't have
> group traffic support.  This doesn't fix some brcmfmac problems where it
> silently overrides the group cipher in the beacon frames though.

That last sentence might require  more explanation?

> ---
>   src/ap.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 

This patch is broken though:

src/ap.c: In function ‘ap_build_cmd_start_ap’:
src/ap.c:2151:6: error: ‘nl_ciphers’ undeclared (first use in this function); 
did you mean ‘l_cipher’?
  2151 |     &nl_ciphers);
       |      ^~~~~~~~~~
       |      l_cipher
src/ap.c:2151:6: note: each undeclared identifier is reported only once for each

Regards,
-Denis

Re: [PATCH 2/5] ap: Fix cleanup on ap_parse_new_station_ies errors

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

Hi Andrew,

On 1/29/21 5:27 PM, Andrew Zaborowski wrote:
> ---
>   src/ap.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index c4715696..1e028122 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -2211,14 +2211,15 @@ static bool ap_parse_new_station_ies(const void *data, uint16_t len,
>   	while (ie_tlv_iter_next(&iter)) {
>   		switch (ie_tlv_iter_get_tag(&iter)) {
>   		case IE_TYPE_RSN:
> -			if (ie_parse_rsne(&iter, NULL) < 0)
> +			if (rsn || ie_parse_rsne(&iter, NULL) < 0)

This may be going beyond being simply paranoid since the kernel is giving these 
messages to us, but ok...

>   				goto parse_error;
>   
>   			rsn = l_memdup(ie_tlv_iter_get_data(&iter) - 2,
>   					ie_tlv_iter_get_length(&iter) + 2);
>   			break;
>   		case IE_TYPE_EXTENDED_SUPPORTED_RATES:
> -			if (ap_parse_supported_rates(&iter, &rates) < 0)
> +			if (rates || ap_parse_supported_rates(&iter, &rates) <
> +					0)
>   				goto parse_error;
>   
>   			break;

Applied, thanks.

Regards,
-Denis

Re: [PATCH 3/5] eapol: Remove assumption of single cipher in authenticator IE

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Hi Andrew,

On 1/29/21 5:27 PM, Andrew Zaborowski wrote:
> Allow the user of the eapol_sm & handshake_state APIs to have multiple
> pairwise ciphers listed in the authenticator IE.  This comes at the cost
> of requiring the user to call handshake_state_override_pairwise_cipher
> to select the cipher when setting up the handshake state.

That's unfortunate.  Why would we want to override pairwise_ciphers selected by 
the STA?  I mean we can maybe do this for testing, but in the general case? 
Furthermore, why would the override API be _required_?

What precludes us from advertising an RSNE with multiple pairwise ciphers and 
then honoring the single pairwise_cipher that the STA chose?

> ---
>   src/adhoc.c     |  1 +
>   src/ap.c        |  2 ++
>   src/eapol.c     | 20 +++++++++++++++-----
>   src/handshake.c | 25 +++++++++++++++----------
>   src/handshake.h |  2 ++
>   5 files changed, 35 insertions(+), 15 deletions(-)
> 

Regards,
-Denis

Re: [PATCH 3/5] eapol: Remove assumption of single cipher in authenticator IE

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

Hi Denis,

On Sat, 30 Jan 2021 at 03:28, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 1/29/21 5:27 PM, Andrew Zaborowski wrote:
> > Allow the user of the eapol_sm & handshake_state APIs to have multiple
> > pairwise ciphers listed in the authenticator IE.  This comes at the cost
> > of requiring the user to call handshake_state_override_pairwise_cipher
> > to select the cipher when setting up the handshake state.
>
> That's unfortunate.  Why would we want to override pairwise_ciphers selected by
> the STA?  I mean we can maybe do this for testing, but in the general case?
> Furthermore, why would the override API be _required_?
>
> What precludes us from advertising an RSNE with multiple pairwise ciphers and
> then honoring the single pairwise_cipher that the STA chose?

It was the assumption that the client STA can set more than one cipher
but I think that was wrong.  We didn't validate this because the
authenticator RSNE had only one cipher set so it was enough to bitwise
& them.  I'll add the check and simplify the code.

Best regards

Re: [PATCH 2/5] ap: Fix cleanup on ap_parse_new_station_ies errors

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On Sat, 30 Jan 2021 at 03:11, Denis Kenzior <denkenz@gmail.com> wrote:
> On 1/29/21 5:27 PM, Andrew Zaborowski wrote:
> > --- a/src/ap.c
> > +++ b/src/ap.c
> > @@ -2211,14 +2211,15 @@ static bool ap_parse_new_station_ies(const void *data, uint16_t len,
> >       while (ie_tlv_iter_next(&iter)) {
> >               switch (ie_tlv_iter_get_tag(&iter)) {
> >               case IE_TYPE_RSN:
> > -                     if (ie_parse_rsne(&iter, NULL) < 0)
> > +                     if (rsn || ie_parse_rsne(&iter, NULL) < 0)
>
> This may be going beyond being simply paranoid since the kernel is giving these
> messages to us, but ok...

So the IEs come from the client, I bet some drivers validate the IE
sequence before passing it on but I don't think they're required to do
this.  I think the check that NL80211_ATTR_MAC is still 6-bytes is
more of a stretch ;-)

Best regards