7:31 a.m.
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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
v4:
* Keep the -EAGAIN return for non-sta case
diff --git a/src/sae.c b/src/sae.c
index c14b646f..7e6377f5 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.
--
2.31.1
7:31 a.m.
New subject: [PATCH v4 2/5] auth-proto: document acceptable return values for auth-protos
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(+)
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
7:31 a.m.
New subject: [PATCH v4 3/5] mpdu: add MMPDU_STATUS_CODE_SAE_PK
---
src/mpdu.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/mpdu.h b/src/mpdu.h
index 98c264f9..36fe2470 100644
--- a/src/mpdu.h
+++ b/src/mpdu.h
@@ -232,6 +232,7 @@ enum mmpdu_status_code {
MMPDU_STATUS_CODE_RESTRICT_AUTH_GDB = 106,
MMPDU_STATUS_CODE_AUTHORIZATION_DEENABLED = 107,
MMPDU_STATUS_CODE_SAE_HASH_TO_ELEMENT = 126,
+ MMPDU_STATUS_CODE_SAE_PK = 127,
};
/* 802.11, Section 8.2.4.1.1, Figure 8-2 */
--
2.31.1
7:32 a.m.
New subject: [PATCH v4 4/5] netdev: handle non-fatal auth-proto returns
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.
In addition the kernel treats a few special error codes (and the auth
transaction as more context) special for SAE. For this a helper was
added which checks for these conditions.
---
src/netdev.c | 39 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
v4:
* Added helper funtion to handle the special return codes the
kernel handles. This logic should mirror the kernels check for
these conditions.
diff --git a/src/netdev.c b/src/netdev.c
index 7909c37e..12de74b2 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2644,6 +2644,30 @@ static void netdev_cmd_ft_reassociate_cb(struct l_genl_msg *msg,
}
}
+static bool netdev_sae_frame_ok(struct netdev *netdev,
+ const struct mmpdu_authentication *auth)
+{
+ uint16_t status = auth->status;
+ uint16_t trans = auth->transaction_sequence;
+
+ if (netdev->handshake->akm_suite != IE_RSN_AKM_SUITE_SAE_SHA256)
+ return false;
+
+ /*
+ * We must copy the kernels logic here to only drop frames slilently
+ * that will result in an automatic retransmission. The kernel only does
+ * this for SAE auth frames if the following conditions are met (as seen
+ * in net/mac80211/mlme.c:ieee80211_rx_mgmt_auth()):
+ */
+ if (status == MMPDU_STATUS_CODE_ANTI_CLOGGING_TOKEN_REQ ||
+ (trans == 1 &&
+ (status == MMPDU_STATUS_CODE_SAE_HASH_TO_ELEMENT ||
+ status == MMPDU_STATUS_CODE_SAE_PK)))
+ return true;
+
+ return false;
+}
+
static void netdev_authenticate_event(struct l_genl_msg *msg,
struct netdev *netdev)
{
@@ -2717,7 +2741,20 @@ 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;
+
+ if (netdev_sae_frame_ok(netdev, auth))
+ return;
+ } else if (ret > 0)
status_code = (uint16_t)ret;
}
--
2.31.1
7:32 a.m.
New subject: [PATCH v4 5/5] auto-t: add sae test for non-acked commit
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
7:48 a.m.
Hi James,
On 9/8/21 4:31 PM, James Prestwood wrote:
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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
Patches 1-3 and 5 applied, thanks.
Regards,
-Denis
375
days inactive
375
days old
5 comments
2 participants
participants (2)
-
Denis Kenzior -
James Prestwood