[PATCH v2 1/4] ie: add is_ie_default_sae_group_oui

[PATCH v2 1/4] ie: add is_ie_default_sae_group_oui

[James Prestwood]

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

Start an OUI list of vendors who have buggy SAE group negotiation
---
 src/ie.c | 25 +++++++++++++++++++++++++
 src/ie.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/src/ie.c b/src/ie.c
index a73d5bbc..3bf3b5f0 100644
--- a/src/ie.c
+++ b/src/ie.c
@@ -1363,6 +1363,31 @@ bool is_ie_wpa_ie(const uint8_t *data, uint8_t len)
 	return false;
 }
 
+/*
+ * List of vendor OUIs which require the default SAE group to be used
+ */
+static uint8_t use_default_sae_group_ouis[][3] = {
+	{ 0xf4, 0xf5, 0xe8 },
+};
+
+bool is_ie_default_sae_group_oui(const void *data, uint16_t len)
+{
+	unsigned int i = 0;
+
+	if (len < 3)
+		return false;
+
+	while (i < L_ARRAY_SIZE(use_default_sae_group_ouis)) {
+		if (!memcmp(use_default_sae_group_ouis[i], data, 3))
+			return true;
+
+		i++;
+	}
+
+	return false;
+}
+
+
 int ie_parse_wpa(struct ie_tlv_iter *iter, struct ie_rsn_info *out_info)
 {
 	const uint8_t *data = iter->data;
diff --git a/src/ie.h b/src/ie.h
index 25b56302..7c4f987d 100644
--- a/src/ie.h
+++ b/src/ie.h
@@ -524,6 +524,8 @@ int ie_parse_wpa_from_data(const uint8_t *data, size_t len,
 						struct ie_rsn_info *info);
 bool is_ie_wfa_ie(const uint8_t *data, uint8_t len, uint8_t oi_type);
 bool is_ie_wpa_ie(const uint8_t *data, uint8_t len);
+bool is_ie_default_sae_group_oui(const void *data, uint16_t len);
+
 bool ie_build_wpa(const struct ie_rsn_info *info, uint8_t *to);
 
 int ie_parse_bss_load(struct ie_tlv_iter *iter, uint16_t *out_sta_count,
-- 
2.31.1

[PATCH v2 2/4] scan: set force_default_sae_group if OUI matches

[James Prestwood]

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

---
 src/scan.c | 4 +++-
 src/scan.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index 3faa6eb4..16bd2a87 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -924,7 +924,9 @@ static bool scan_parse_vendor_specific(struct scan_bss *bss, const void *data,
 			return false;
 
 		bss->hs20_capable = true;
-	} else
+	} else if (is_ie_default_sae_group_oui(data, len))
+		bss->force_default_sae_group = true;
+	else
 		return false;
 
 	return true;
diff --git a/src/scan.h b/src/scan.h
index 81c84bae..8a57c2b3 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -83,6 +83,7 @@ struct scan_bss {
 	bool vht_capable : 1;
 	bool anqp_capable : 1;
 	bool hs20_capable : 1;
+	bool force_default_sae_group : 1;
 };
 
 struct scan_parameters {
-- 
2.31.1

[PATCH v2 3/4] sae: handle force_default_sae_group in scan_bss

[James Prestwood]

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

Now a scan_bss object is passed to sae_sm_new in order to detect if
the BSS's vendor OUI matches ones in which SAE group negotiation is
broken. When an AP like this is found SAE will use group 19
unconditionally, and fail if group 19 does not work. Other groups
could be tried upon failure but per the spec group 19 must be supported
so there isn't much use in trying other, optional groups.

Note: the check on 'bss' was added in order to make unit testing
easier to integrate as including scan.c in unit tests opens up a
can of worms.
---
 src/netdev.c |  2 +-
 src/sae.c    | 27 +++++++++++++++++++++++++++
 src/sae.h    |  2 ++
 3 files changed, 30 insertions(+), 1 deletion(-)

v2:
 * Only force group 19 when sae_type is LOOPING
 * Increment group_retry when forcing group otherwise
   IWD would continue to try group 19 over and over

diff --git a/src/netdev.c b/src/netdev.c
index d886efad..87b9c3f0 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3470,7 +3470,7 @@ static void netdev_connect_common(struct netdev *netdev,
 	switch (hs->akm_suite) {
 	case IE_RSN_AKM_SUITE_SAE_SHA256:
 	case IE_RSN_AKM_SUITE_FT_OVER_SAE_SHA256:
-		netdev->ap = sae_sm_new(hs, netdev_sae_tx_authenticate,
+		netdev->ap = sae_sm_new(hs, bss, netdev_sae_tx_authenticate,
 						netdev_sae_tx_associate,
 						netdev);
 
diff --git a/src/sae.c b/src/sae.c
index 5099473c..5b2c74fc 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -34,6 +34,7 @@
 #include "src/mpdu.h"
 #include "src/auth-proto.h"
 #include "src/sae.h"
+#include "src/scan.h"
 
 /* SHA-512 is the highest supported hashing function as of 802.11-2020 */
 #define SAE_MAX_HASH_LEN 64
@@ -83,6 +84,8 @@ struct sae_sm {
 	sae_tx_associate_func_t tx_assoc;
 	void *user_data;
 	enum crypto_sae sae_type;
+
+	bool force_default_group : 1;
 };
 
 static enum mmpdu_status_code sae_status_code(struct sae_sm *sm)
@@ -139,6 +142,24 @@ static int sae_choose_next_group(struct sae_sm *sm)
 	const unsigned int *ecc_groups = l_ecc_supported_ike_groups();
 	bool reset = sm->group_retry >= 0;
 
+	/*
+	 * If this is a buggy AP in which group negotiation is broken use the
+	 * default group 19 and fail if this is a retry.
+	 */
+	if (sm->sae_type == CRYPTO_SAE_LOOPING && sm->force_default_group) {
+		if (sm->group_retry != -1) {
+			l_warn("Forced default group but was rejected!");
+			return -ENOENT;
+		}
+
+		l_debug("Forcing default SAE group 19");
+
+		sm->group_retry++;
+		sm->group = 19;
+
+		goto get_curve;
+	}
+
 	do {
 		sm->group_retry++;
 
@@ -151,6 +172,8 @@ static int sae_choose_next_group(struct sae_sm *sm)
 		sae_reset_state(sm);
 
 	sm->group = ecc_groups[sm->group_retry];
+
+get_curve:
 	sm->curve = l_ecc_curve_from_ike_group(sm->group);
 
 	return 0;
@@ -1317,6 +1340,7 @@ static void sae_free(struct auth_proto *ap)
 }
 
 struct auth_proto *sae_sm_new(struct handshake_state *hs,
+				struct scan_bss *bss,
 				sae_tx_authenticate_func_t tx_auth,
 				sae_tx_associate_func_t tx_assoc,
 				void *user_data)
@@ -1351,5 +1375,8 @@ struct auth_proto *sae_sm_new(struct handshake_state *hs,
 		sm->sae_type = CRYPTO_SAE_LOOPING;
 	}
 
+	if (bss && bss->force_default_sae_group)
+		sm->force_default_group = true;
+
 	return &sm->ap;
 }
diff --git a/src/sae.h b/src/sae.h
index 668d084f..d8f9f2d7 100644
--- a/src/sae.h
+++ b/src/sae.h
@@ -23,6 +23,7 @@
 struct auth_proto;
 struct sae_sm;
 struct handshake_state;
+struct scan_bss;
 
 typedef void (*sae_tx_authenticate_func_t)(const uint8_t *data, size_t len,
 						void *user_data);
@@ -31,6 +32,7 @@ typedef void (*sae_tx_associate_func_t)(void *user_data);
 bool sae_sm_is_h2e(struct auth_proto *ap);
 
 struct auth_proto *sae_sm_new(struct handshake_state *hs,
+				struct scan_bss *bss,
 				sae_tx_authenticate_func_t tx_auth,
 				sae_tx_associate_func_t tx_assoc,
 				void *user_data);
-- 
2.31.1

[PATCH v2 4/4] unit: update test-sae with API change

[James Prestwood]

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

---
 Makefile.am     |  3 ++-
 unit/test-sae.c | 14 +++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e6d2fc91..8b0df2aa 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -521,7 +521,8 @@ unit_test_sae_SOURCES = unit/test-sae.c \
 				src/handshake.h src/handshake.c \
 				src/erp.h src/erp.c \
 				src/util.h src/util.c \
-				src/mpdu.h src/mpdu.c
+				src/mpdu.h src/mpdu.c \
+				src/scan.h
 unit_test_sae_LDADD = $(ell_ldadd)
 unit_test_sae_LDFLAGS = -Wl,-wrap,l_ecc_supported_ike_groups
 
diff --git a/unit/test-sae.c b/unit/test-sae.c
index d9ec6b31..00819eb2 100644
--- a/unit/test-sae.c
+++ b/unit/test-sae.c
@@ -171,7 +171,7 @@ static struct auth_proto *test_initialize(struct test_data *td)
 
 	memset(td->test_clogging_token, 0xde, 32);
 
-	ap = sae_sm_new(hs, test_tx_auth_func, test_tx_assoc_func, td);
+	ap = sae_sm_new(hs, NULL, test_tx_auth_func, test_tx_assoc_func, td);
 
 	td->commit_success = false;
 	auth_proto_start(ap);
@@ -423,8 +423,8 @@ static void test_bad_confirm(const void *arg)
 	handshake_state_set_passphrase(hs2, passphrase);
 	handshake_state_set_authenticator(hs2, true);
 
-	ap1 = sae_sm_new(hs1, end_to_end_tx_func, test_tx_assoc_func, td1);
-	ap2 = sae_sm_new(hs2, end_to_end_tx_func, test_tx_assoc_func, td2);
+	ap1 = sae_sm_new(hs1, NULL, end_to_end_tx_func, test_tx_assoc_func, td1);
+	ap2 = sae_sm_new(hs2, NULL, end_to_end_tx_func, test_tx_assoc_func, td2);
 
 	/* both peers send out commit */
 	ap1->start(ap1);
@@ -498,8 +498,8 @@ static void test_confirm_after_accept(const void *arg)
 	handshake_state_set_passphrase(hs2, passphrase);
 	handshake_state_set_authenticator(hs2, true);
 
-	ap1 = sae_sm_new(hs1, end_to_end_tx_func, test_tx_assoc_func, td1);
-	ap2 = sae_sm_new(hs2, end_to_end_tx_func, test_tx_assoc_func, td2);
+	ap1 = sae_sm_new(hs1, NULL, end_to_end_tx_func, test_tx_assoc_func, td1);
+	ap2 = sae_sm_new(hs2, NULL, end_to_end_tx_func, test_tx_assoc_func, td2);
 
 	/* both peers send out commit */
 	auth_proto_start(ap1);
@@ -583,8 +583,8 @@ static void test_end_to_end(const void *arg)
 	handshake_state_set_passphrase(hs2, passphrase);
 	handshake_state_set_authenticator(hs2, true);
 
-	ap1 = sae_sm_new(hs1, end_to_end_tx_func, test_tx_assoc_func, td1);
-	ap2 = sae_sm_new(hs2, end_to_end_tx_func, test_tx_assoc_func, td2);
+	ap1 = sae_sm_new(hs1, NULL, end_to_end_tx_func, test_tx_assoc_func, td1);
+	ap2 = sae_sm_new(hs2, NULL, end_to_end_tx_func, test_tx_assoc_func, td2);
 
 	/* both peers send out commit */
 	auth_proto_start(ap1);
-- 
2.31.1

Re: [PATCH v2 1/4] ie: add is_ie_default_sae_group_oui

[Denis Kenzior]

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

Hi James,

On 8/23/21 6:41 PM, James Prestwood wrote:
> Start an OUI list of vendors who have buggy SAE group negotiation
> ---
>   src/ie.c | 25 +++++++++++++++++++++++++
>   src/ie.h |  2 ++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/src/ie.c b/src/ie.c
> index a73d5bbc..3bf3b5f0 100644
> --- a/src/ie.c
> +++ b/src/ie.c
> @@ -1363,6 +1363,31 @@ bool is_ie_wpa_ie(const uint8_t *data, uint8_t len)
>   	return false;
>   }
>   
> +/*
> + * List of vendor OUIs which require the default SAE group to be used
> + */
> +static uint8_t use_default_sae_group_ouis[][3] = {
> +	{ 0xf4, 0xf5, 0xe8 },

I wonder if blacklisting the entire vendor oui is a good idea.  The Nest seems 
to send a subtype of '05' of the vendor IE.  Maybe we should limit it to that 
for now?

> +};
> +
> +bool is_ie_default_sae_group_oui(const void *data, uint16_t len)
> +{
> +	unsigned int i = 0;
> +
> +	if (len < 3)
> +		return false;
> +
> +	while (i < L_ARRAY_SIZE(use_default_sae_group_ouis)) {
> +		if (!memcmp(use_default_sae_group_ouis[i], data, 3))
> +			return true;

So supporting matching by OUI or OUI + type might be nice.

> +
> +		i++;
> +	}
> +
> +	return false;
> +}
> +
> +

double empty line

Regards,
-Denis

Re: [PATCH v2 3/4] sae: handle force_default_sae_group in scan_bss

[Denis Kenzior]

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

Hi James,

On 8/23/21 6:41 PM, James Prestwood wrote:
> Now a scan_bss object is passed to sae_sm_new in order to detect if
> the BSS's vendor OUI matches ones in which SAE group negotiation is
> broken. When an AP like this is found SAE will use group 19
> unconditionally, and fail if group 19 does not work. Other groups
> could be tried upon failure but per the spec group 19 must be supported
> so there isn't much use in trying other, optional groups.
> 
> Note: the check on 'bss' was added in order to make unit testing
> easier to integrate as including scan.c in unit tests opens up a
> can of worms.
> ---
>   src/netdev.c |  2 +-
>   src/sae.c    | 27 +++++++++++++++++++++++++++
>   src/sae.h    |  2 ++
>   3 files changed, 30 insertions(+), 1 deletion(-)
> 
> v2:
>   * Only force group 19 when sae_type is LOOPING
>   * Increment group_retry when forcing group otherwise
>     IWD would continue to try group 19 over and over
> 

<snip>

> diff --git a/src/sae.h b/src/sae.h
> index 668d084f..d8f9f2d7 100644
> --- a/src/sae.h
> +++ b/src/sae.h
> @@ -23,6 +23,7 @@
>   struct auth_proto;
>   struct sae_sm;
>   struct handshake_state;
> +struct scan_bss;
>   
>   typedef void (*sae_tx_authenticate_func_t)(const uint8_t *data, size_t len,
>   						void *user_data);
> @@ -31,6 +32,7 @@ typedef void (*sae_tx_associate_func_t)(void *user_data);
>   bool sae_sm_is_h2e(struct auth_proto *ap);
>   
>   struct auth_proto *sae_sm_new(struct handshake_state *hs,
> +				struct scan_bss *bss,
>   				sae_tx_authenticate_func_t tx_auth,
>   				sae_tx_associate_func_t tx_assoc,
>   				void *user_data);
> 

Looking at this again, I guess we might as well just add:

void sae_sm_set_force_group_19(struct auth_proto *ap);

Which would remove the need for scan_bss dependency and the unit test can remain 
untouched.

Regards,
-Denis