[PATCH v2 1/7] sae: fix a spec violation with duplicate commits

[PATCH v2 1/7] sae: fix a spec violation with duplicate commits

[James Prestwood]

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

If a commit is received while in an accepted state the spec states
the scalar should be checked against the previous commit and if
equal the message should be silently dropped.
---
 src/sae.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

v2:
 * Changed to check the scalar rather than always ignore.

diff --git a/src/sae.c b/src/sae.c
index 62fd6c88..c14b646f 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -1182,10 +1182,32 @@ static int sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 {
 	uint16_t sc;
 
-	/* spec does not specify what to do here, so print and discard */
-	if (trans != SAE_STATE_CONFIRMED) {
+	/*
+	 * 12.4.8.6.1 Parent process behavior
+	 *
+	 * "Upon receipt of an SAE Commit message... and it is in Accepted
+	 * state, the scalar in the received frame is checked against the
+	 * peer-scalar used in authentication of the existing protocol instance
+	 * (in Accepted state). If it is identical, the frame shall be dropped"
+	 */
+	if (trans == SAE_STATE_COMMITTED) {
+		bool drop;
+		unsigned int nbytes = l_ecc_curve_get_scalar_bytes(sm->curve);
+		struct l_ecc_scalar *p_scalar;
+
+		if (len < nbytes + 2)
+			return -EMSGSIZE;
+
+		p_scalar = l_ecc_scalar_new(sm->curve, frame + 2, nbytes);
+
+		drop = l_ecc_scalars_are_equal(sm->p_scalar, p_scalar);
+		l_ecc_scalar_free(p_scalar);
+
+		if (drop)
+			return -EBADMSG;
+
 		l_error("received transaction %u in accepted state", trans);
-		return -EBADMSG;
+		return -EPROTO;
 	}
 
 	if (sm->sync > SAE_SYNC_MAX)
-- 
2.31.1

[PATCH v2 2/7] sae: don't send commit/confirm in confirmed state

[James Prestwood]

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

This works around a hostapd bug (described more in the TODO comment)
that deaths IWD if the initial commit is not acked. This behavior
has been identified in consumer access points and likely won't ever
be patched for older devices. Because of this IWD must work around
the problem which can be eliminated by not sending out this commit
message.

This bug was reported to the hostapd ML:

https://lists.infradead.org/pipermail/hostap/2021-September/039842.html

This change should not cause any compatibility problems to non-hostapd
access points and is identical to how wpa_supplicant treats this
scenario.
---
 src/sae.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

v2:
 * Changed to simply drop the message. Sending only a confirm is pointless
   since it would not verify without a commit. Also remove sc++ since this
   should only be incremented when a confirm is sent.

diff --git a/src/sae.c b/src/sae.c
index c14b646f..de2e0e0a 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -1163,14 +1163,26 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
 	 * transmit its Commit and Confirm (with the new Sc value) messages.
 	 */
 	sm->sync++;
-	sm->sc++;
-
-	sae_send_commit(sm, true);
 
-	if (!sae_send_confirm(sm))
-		return -EPROTO;
+	/*
+	 * TODO: There is a bug in hostapd which deaths stations if a commit
+	 * is received in an Accepted SAE state. This can be triggered if the
+	 * STA's commit is not acked, which triggers a re-transmission, and
+	 * ultimated causes the AP to death IWD.
+	 *
+	 * Since this bug is present in production APs out in the wild we must
+	 * work around it by going against the spec (802.11-2020 12.4.8.6.5) and
+	 * simply drop the message.
+	 *
+	 * sm->sc++;
+	 *
+	 * sae_send_commit(sm, true);
+	 *
+	 * if (!sae_send_confirm(sm))
+	 *         return -EPROTO;
+	 */
 
-	return -EAGAIN;
+	return -EBADMSG;
 }
 
 /*
-- 
2.31.1

Re: [PATCH v2 2/7] sae: don't send commit/confirm in confirmed state

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:18 PM, James Prestwood wrote:
> This works around a hostapd bug (described more in the TODO comment)
> that deaths IWD if the initial commit is not acked. This behavior

Also kernel re-transmit behavior is a bit aggressive which is a big contributing 
factor.  It is definitely not what the spec describes.

> has been identified in consumer access points and likely won't ever
> be patched for older devices. Because of this IWD must work around
> the problem which can be eliminated by not sending out this commit
> message.
> 
> This bug was reported to the hostapd ML:
> 
> https://lists.infradead.org/pipermail/hostap/2021-September/039842.html
> 
> This change should not cause any compatibility problems to non-hostapd
> access points and is identical to how wpa_supplicant treats this
> scenario.
> ---
>   src/sae.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
> 
> v2:
>   * Changed to simply drop the message. Sending only a confirm is pointless
>     since it would not verify without a commit. Also remove sc++ since this
>     should only be incremented when a confirm is sent.
> 
> diff --git a/src/sae.c b/src/sae.c
> index c14b646f..de2e0e0a 100644
> --- a/src/sae.c
> +++ b/src/sae.c
> @@ -1163,14 +1163,26 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
>   	 * transmit its Commit and Confirm (with the new Sc value) messages.
>   	 */
>   	sm->sync++;

So why do we sync in this case?

I would actually do something like:

/* Due to kernel retransmit behavior on missing acks + hostapd using incorrect 
behavior, the following sequence can happen:
   < seq >

For this reason, in station mode, we ignore any Commit messages in the Confirmed 
state as they're likely caused by retransmissions.
*/

if (!sm->handshake->authenticator)
	return -EBADMSG;

> -	sm->sc++;
> -
> -	sae_send_commit(sm, true);
>   
> -	if (!sae_send_confirm(sm))
> -		return -EPROTO;
> +	/*
> +	 * TODO: There is a bug in hostapd which deaths stations if a commit
> +	 * is received in an Accepted SAE state. This can be triggered if the
> +	 * STA's commit is not acked, which triggers a re-transmission, and
> +	 * ultimated causes the AP to death IWD.
> +	 *
> +	 * Since this bug is present in production APs out in the wild we must
> +	 * work around it by going against the spec (802.11-2020 12.4.8.6.5) and
> +	 * simply drop the message.
> +	 *
> +	 * sm->sc++;
> +	 *
> +	 * sae_send_commit(sm, true);
> +	 *
> +	 * if (!sae_send_confirm(sm))
> +	 *         return -EPROTO;
> +	 */
>   
> -	return -EAGAIN;
> +	return -EBADMSG;
>   }
>   
>   /*
> 

Regards,
-Denis

[PATCH v2 3/7] auth-proto: document acceptable return values for auth-protos

[James Prestwood]

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

Since all auth-protos are hidden behind an abstraction they need
to be consisten with the return values as some should be handled
specially.
---
 src/auth-proto.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/auth-proto.h b/src/auth-proto.h
index d3c4686f..f39c361c 100644
--- a/src/auth-proto.h
+++ b/src/auth-proto.h
@@ -25,6 +25,18 @@
 struct auth_proto {
 	bool (*start)(struct auth_proto *ap);
 	void (*free)(struct auth_proto *ap);
+	/*
+	 * Callback to receive an Authenticate frame. auth-protos should
+	 * return error codes consistent with one another as some are treated
+	 * specially:
+	 *
+	 * 0 indicates success
+	 * -ENOMSG or -EBADMSG indicates the message should be ignored silently
+	 * -EAGAIN indicates to try again (handled by auth-proto internally)
+	 * -EPROTO indicates a fatal error
+	 * Any other < 0 return will be treated as a fatal error
+	 * > 0 indicates a fatal error with status code.
+	 */
 	int (*rx_authenticate)(struct auth_proto *driver,
 					const uint8_t *frame, size_t len);
 	int (*rx_associate)(struct auth_proto *driver,
-- 
2.31.1

Re: [PATCH v2 3/7] auth-proto: document acceptable return values for auth-protos

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:19 PM, James Prestwood wrote:
> Since all auth-protos are hidden behind an abstraction they need
> to be consisten with the return values as some should be handled
> specially.
> ---
>   src/auth-proto.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/src/auth-proto.h b/src/auth-proto.h
> index d3c4686f..f39c361c 100644
> --- a/src/auth-proto.h
> +++ b/src/auth-proto.h
> @@ -25,6 +25,18 @@
>   struct auth_proto {
>   	bool (*start)(struct auth_proto *ap);
>   	void (*free)(struct auth_proto *ap);
> +	/*
> +	 * Callback to receive an Authenticate frame. auth-protos should
> +	 * return error codes consistent with one another as some are treated
> +	 * specially:
> +	 *
> +	 * 0 indicates success

Also implies a state transition has happened, right?

> +	 * -ENOMSG or -EBADMSG indicates the message should be ignored silently
> +	 * -EAGAIN indicates to try again (handled by auth-proto internally)

Implies a retry is happening and state hasn't changed?

> +	 * -EPROTO indicates a fatal error
> +	 * Any other < 0 return will be treated as a fatal error
> +	 * > 0 indicates a fatal error with status code.

Might want to mention that this is only applicable for non-station cases.

> +	 */
>   	int (*rx_authenticate)(struct auth_proto *driver,
>   					const uint8_t *frame, size_t len);
>   	int (*rx_associate)(struct auth_proto *driver,
> 

Regards,
-Denis

[PATCH v2 4/7] fils: change fatal return code to -EPROTO

[James Prestwood]

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

This keeps FILS consistent with what netdev expects for a fatal
auth-proto return.
---
 src/fils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fils.c b/src/fils.c
index 7fffefb3..f661f1bf 100644
--- a/src/fils.c
+++ b/src/fils.c
@@ -289,7 +289,7 @@ static int fils_derive_key_data(struct fils_sm *fils)
 		if (ie_parse_rsne_from_data(fils->hs->supplicant_ie,
 						fils->hs->supplicant_ie[1] + 2,
 						&rsn_info) < 0)
-			return -EBADMSG;
+			return -EPROTO;
 
 		rsn_info.num_pmkids = 1;
 		rsn_info.pmkids = fils->hs->pmk_r1_name;
-- 
2.31.1

Re: [PATCH v2 4/7] fils: change fatal return code to -EPROTO

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:19 PM, James Prestwood wrote:
> This keeps FILS consistent with what netdev expects for a fatal
> auth-proto return.
> ---
>   src/fils.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH v2 5/7] ft: change fatal return code to -EPROTO

[James Prestwood]

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

This keeps FT consistent with what netdev expects for a fatal
auth-proto return.
---
 src/ft.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ft.c b/src/ft.c
index 13733019..c167f4bc 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -520,7 +520,7 @@ static int ft_process_ies(struct handshake_state *hs, const uint8_t *ies,
 	return 0;
 
 ft_error:
-	return -EBADMSG;
+	return -EPROTO;
 }
 
 int ft_over_ds_parse_action_response(const uint8_t *frame, size_t frame_len,
-- 
2.31.1

Re: [PATCH v2 5/7] ft: change fatal return code to -EPROTO

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:19 PM, James Prestwood wrote:
> This keeps FT consistent with what netdev expects for a fatal
> auth-proto return.
> ---
>   src/ft.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ft.c b/src/ft.c
> index 13733019..c167f4bc 100644
> --- a/src/ft.c
> +++ b/src/ft.c
> @@ -520,7 +520,7 @@ static int ft_process_ies(struct handshake_state *hs, const uint8_t *ies,
>   	return 0;
>   
>   ft_error:
> -	return -EBADMSG;
> +	return -EPROTO;

The error code from ft_process_ies isn't actually returned to the upper layers. 
  In fact, ft returns a status_code instead.  So, maybe we should hold off on 
this for now.

>   }
>   
>   int ft_over_ds_parse_action_response(const uint8_t *frame, size_t frame_len,
> 

Regards,
-Denis

[PATCH v2 6/7] netdev: handle non-fatal auth-proto returns

[James Prestwood]

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

This adds -EBADMSG/-ENOMSG to returns which won't result in
a deauth/failed connection.
---
 src/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/netdev.c b/src/netdev.c
index 7909c37e..8bbd5abb 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2715,7 +2715,8 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
 		status_code = L_CPU_TO_LE16(auth->status);
 
 		ret = auth_proto_rx_authenticate(netdev->ap, frame, frame_len);
-		if (ret == 0 || ret == -EAGAIN)
+		if (ret == 0 || ret == -EAGAIN || ret == -EBADMSG ||
+						ret == -ENOMSG)
 			return;
 		else if (ret > 0)
 			status_code = (uint16_t)ret;
-- 
2.31.1

Re: [PATCH v2 6/7] netdev: handle non-fatal auth-proto returns

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:19 PM, James Prestwood wrote:
> This adds -EBADMSG/-ENOMSG to returns which won't result in
> a deauth/failed connection.
> ---
>   src/netdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/netdev.c b/src/netdev.c
> index 7909c37e..8bbd5abb 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -2715,7 +2715,8 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
>   		status_code = L_CPU_TO_LE16(auth->status);
>   
>   		ret = auth_proto_rx_authenticate(netdev->ap, frame, frame_len);
> -		if (ret == 0 || ret == -EAGAIN)
> +		if (ret == 0 || ret == -EAGAIN || ret == -EBADMSG ||
> +						ret == -ENOMSG)

So we may have to be a bit careful here.  If we receive -EBADMSG/-ENOMSG and the 
status_code != 0, kernel won't keep retransmitting the Authenticate frame.  So 
we would never actually leave the connecting state in this case...

I guess we could ignore EBADMSG/ENOMSG only if status_code == 0, and otherwise 
just let the connection fail.

>   			return;
>   		else if (ret > 0)
>   			status_code = (uint16_t)ret;
> 

Regards,
-Denis

[PATCH v2 7/7] auto-t: add sae test for non-acked commit

[James Prestwood]

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

This test simulates the scenario where IWDs commit is not acked which
exposes a hostapd bug that ultimately fails the connection. This behavior
can be seen by reverting the commit which works around this issue:

"sae: don't send commit in confirmed state"

With the above patch applied this test should pass.

Note: The existing timeout test was reused as it was not of much use
anyways. All it did was block auth/assoc frames and expect a failure
which didn't exercise any SAE logic anyways.
---
 autotests/testSAE/timeout_test.py | 41 ++++++++++++++++---------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/autotests/testSAE/timeout_test.py b/autotests/testSAE/timeout_test.py
index abef109b..1792f1c0 100644
--- a/autotests/testSAE/timeout_test.py
+++ b/autotests/testSAE/timeout_test.py
@@ -9,13 +9,12 @@ from iwd import IWD
 from iwd import PSKAgent
 from iwd import NetworkType
 from hwsim import Hwsim
+from hostapd import HostapdCLI
+from config import ctx
 
 class Test(unittest.TestCase):
 
     def validate_connection(self, wd):
-        hwsim = Hwsim()
-        bss_radio = hwsim.get_radio('rad0')
-
         psk_agent = PSKAgent(["secret123", "secret123"])
         wd.register_psk_agent(psk_agent)
 
@@ -30,29 +29,31 @@ class Test(unittest.TestCase):
         condition = 'not obj.connected'
         wd.wait_for_object_condition(network.network_object, condition)
 
+        network.network_object.connect()
+
+        condition = 'obj.state == DeviceState.connected'
+        wd.wait_for_object_condition(device, condition)
+
+    def test_not_acked_commit(self):
+        #
+        # TODO: This merely forces group 19 by acting as a 'buggy' AP. This is
+        # needed because the hwsim rule only matches once and must be matched
+        # on the first commit, not during group negotiation.
+        #
+        hostapd = HostapdCLI(config='ssidSAE.conf')
+        hostapd.set_value('vendor_elements', 'dd0cf4f5e8050500000000000000')
+
+        hwsim = Hwsim()
+        bss_radio = hwsim.get_radio('rad0')
+
         rule0 = hwsim.rules.create()
         rule0.source = bss_radio.addresses[0]
-        rule0.bidirectional = True
         rule0.drop = True
         rule0.prefix = 'b0'
+        rule0.match_times = 1
+        rule0.drop_ack = True
         rule0.enabled = True
 
-        wd.wait(1)
-        print(rule0)
-
-        with self.assertRaises(iwd.FailedEx):
-            network.network_object.connect()
-
-        condition = 'not obj.connected'
-        wd.wait_for_object_condition(network.network_object, condition)
-
-        rule0.prefix = '00'
-        with self.assertRaises(iwd.FailedEx):
-            network.network_object.connect()
-
-        wd.unregister_psk_agent(psk_agent)
-
-    def test_connection_success(self):
         wd = IWD(True)
         self.validate_connection(wd)
 
-- 
2.31.1

Re: [PATCH v2 1/7] sae: fix a spec violation with duplicate commits

[Denis Kenzior]

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

Hi James,

On 9/8/21 1:18 PM, James Prestwood wrote:
> If a commit is received while in an accepted state the spec states
> the scalar should be checked against the previous commit and if
> equal the message should be silently dropped.
> ---
>   src/sae.c | 28 +++++++++++++++++++++++++---
>   1 file changed, 25 insertions(+), 3 deletions(-)
> 
> v2:
>   * Changed to check the scalar rather than always ignore.
> 

Applied, thanks.

Regards,
-Denis