* [PATCH v4 2/5] auth-proto: document acceptable return values for auth-protos
2021-09-08 21:31 [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state James Prestwood
@ 2021-09-08 21:31 ` James Prestwood
2021-09-08 21:31 ` [PATCH v4 3/5] mpdu: add MMPDU_STATUS_CODE_SAE_PK James Prestwood
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2021-09-08 21:31 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 1373 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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 3/5] mpdu: add MMPDU_STATUS_CODE_SAE_PK
2021-09-08 21:31 [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state James Prestwood
2021-09-08 21:31 ` [PATCH v4 2/5] auth-proto: document acceptable return values for auth-protos James Prestwood
@ 2021-09-08 21:31 ` James Prestwood
2021-09-08 21:32 ` [PATCH v4 4/5] netdev: handle non-fatal auth-proto returns James Prestwood
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2021-09-08 21:31 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 460 bytes --]
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 4/5] netdev: handle non-fatal auth-proto returns
2021-09-08 21:31 [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state James Prestwood
2021-09-08 21:31 ` [PATCH v4 2/5] auth-proto: document acceptable return values for auth-protos James Prestwood
2021-09-08 21:31 ` [PATCH v4 3/5] mpdu: add MMPDU_STATUS_CODE_SAE_PK James Prestwood
@ 2021-09-08 21:32 ` James Prestwood
2021-09-08 21:32 ` [PATCH v4 5/5] auto-t: add sae test for non-acked commit James Prestwood
2021-09-08 21:48 ` [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2021-09-08 21:32 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 2709 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.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 5/5] auto-t: add sae test for non-acked commit
2021-09-08 21:31 [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state James Prestwood
` (2 preceding siblings ...)
2021-09-08 21:32 ` [PATCH v4 4/5] netdev: handle non-fatal auth-proto returns James Prestwood
@ 2021-09-08 21:32 ` James Prestwood
2021-09-08 21:48 ` [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: James Prestwood @ 2021-09-08 21:32 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state
2021-09-08 21:31 [PATCH v4 1/5] sae: don't send commit/confirm in confirmed state James Prestwood
` (3 preceding siblings ...)
2021-09-08 21:32 ` [PATCH v4 5/5] auto-t: add sae test for non-acked commit James Prestwood
@ 2021-09-08 21:48 ` Denis Kenzior
4 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2021-09-08 21:48 UTC (permalink / raw)
To: iwd
[-- Attachment #1: Type: text/plain, Size: 973 bytes --]
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
^ permalink raw reply [flat|nested] 6+ messages in thread