[PATCH v3 1/4] sae: don't send commit/confirm in confirmed state

[PATCH v3 1/4] sae: don't send commit/confirm in confirmed state

[James Prestwood]

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

This works around a hostapd bug (described more in the TODO comment)
which is exposed because of the kernels overly agressive re-transmit
behavior on missed ACKs. Combined this results in a death 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 | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

v3:
 * Kept existing behavior for non-STA handshakes
 * Better described the sequence of events that requires this workaround

diff --git a/src/sae.c b/src/sae.c
index c14b646f..ccba74cc 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -1158,6 +1158,31 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
 	if (l_get_le16(frame) != sm->group)
 		return -EBADMSG;
 
+	/*
+	 * Because of kernel retransmit behavior on missed ACKs plus hostapd's
+	 * incorrect handling of confirm packets while in accepted state the
+	 * following can happen:
+	 *
+	 * 1. Client sends commit, not acked (committed state)
+	 * 2. AP receives commit, sends commit reply (committed state)
+	 * 3. Client retransmits original commit
+	 * 4. Client receives AP's commit, sends confirm (confirmed state)
+	 * 5. AP receives clients retransmitted commit, sends only commit
+	 * 6. AP receives clients confirm and accepts (accepted state)
+	 * 7. Client receives AP's commit and sends both commit + confirm
+	 *    (the code below).
+	 * 8. AP receives clients commit while in accepted state, and deauths
+	 *
+	 * Due to this, any commit received while in a confirmed state will be
+	 * ignored by IWD since it is probably caused by this retransmission
+	 * and sending the commit/confirm below would likely cause hostapd to
+	 * deauth us.
+	 *
+	 * As for non-sta (currently not used) we want to keep with the spec.
+	 */
+	if (!sm->handshake->authenticator)
+		return -EBADMSG;
+
 	/*
 	 * the protocol instance shall increment Sync, increment Sc, and
 	 * transmit its Commit and Confirm (with the new Sc value) messages.
@@ -1170,7 +1195,7 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
 	if (!sae_send_confirm(sm))
 		return -EPROTO;
 
-	return -EAGAIN;
+	return -EBADMSG;
 }
 
 /*
-- 
2.31.1

[PATCH v3 2/4] auth-proto: document acceptable return values for auth-protos

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1423 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

v3:
 * Added some more details to the comment

diff --git a/src/auth-proto.h b/src/auth-proto.h
index d3c4686f..33a74291 100644
--- a/src/auth-proto.h
+++ b/src/auth-proto.h
@@ -25,6 +25,21 @@
 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, and that a state transition occurred.
+	 * -ENOMSG or -EBADMSG indicates the message should be ignored silently
+	 * -EAGAIN indicates a retry, and no state transition occurred. Any
+	 *         retry is handled by the 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. This only applies to
+	 *         non-sta cases as non-zero status codes are rejected by the
+	 *         kernel when in station mode.
+	 */
 	int (*rx_authenticate)(struct auth_proto *driver,
 					const uint8_t *frame, size_t len);
 	int (*rx_associate)(struct auth_proto *driver,
-- 
2.31.1

[PATCH v3 3/4] netdev: handle non-fatal auth-proto returns

[James Prestwood]

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

This adds -EBADMSG/-ENOMSG to be handled specially by allowing the
frame to be dropped, similar to 0 or -EAGAIN. The difference though
is that a non-zero authenticate status will result in the kernel
not re-transmitting which ultimately could leave IWD in a connecting
state with no way of leaving. Because of this we handle a non-zero
status for these two returns as a failed connection.
---
 src/netdev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

v3:
 * Only drop the message if the status was successful.

diff --git a/src/netdev.c b/src/netdev.c
index 7909c37e..fc4613e6 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2717,7 +2717,17 @@ static void netdev_authenticate_event(struct l_genl_msg *msg,
 		ret = auth_proto_rx_authenticate(netdev->ap, frame, frame_len);
 		if (ret == 0 || ret == -EAGAIN)
 			return;
-		else if (ret > 0)
+		else if (ret == -EBADMSG || ret == -ENOMSG) {
+			/*
+			 * Only drop if the status is success. Otherwise
+			 * dropping the message here would prevent the kernel
+			 * from re-transmitting, leaving IWD in this connecting
+			 * state. If this happens the only option is to fail the
+			 * connection here.
+			 */
+			if (status_code == 0)
+				return;
+		} else if (ret > 0)
 			status_code = (uint16_t)ret;
 	}
 
-- 
2.31.1

[PATCH v3 4/4] 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