SAE was also relying on the ELL bug which was incorrectly performing
a subtraction on the Y coordinate based on the compressed point type.
Correct this and make the point type more clear (rather than
something like "is_odd + 2").
sae.c was failing to build on some platforms:
error: implicit declaration of function 'reallocarray'; did you mean 'realloc'?
[-Werror=implicit-function-declaration]
Building on GCC 8 resulted in this compiler error.
src/sae.c:107:25: error: implicit declaration of function 'reallocarray';
did you mean 'realloc'? [-Werror=implicit-function-declaration]
sm->rejected_groups = reallocarray(NULL, 2, sizeof(uint16_t));
- Mostly problems with whitespace:
- Use of spaces instead of tabs
- Stray spaces before closing ')
- Missing spaces
- Missing 'void' from function declarations & definitions that
take no arguments.
- Wrong indentation level
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.
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.
Setter which forces the use of group 19 rather than the group order
that ELL provides. Certain APs have been found to have buggy group
negotiation and only work if group 19 is tried first, and only. When
an AP like this this is found (based on vendor OUI match) 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.
Instead of requiring each auth_proto to perform validation of the frames
received via rx_authenticate & rx_associate, have netdev itself perform
the mpdu validation. This is unlikely to happen anyway since the kernel
performs its own frame validation. Print a warning in case the
validation fails.
There's no reason why a change in groups would result in the
anti-clogging token becoming invalid. This might result in us needing
an extra round-trip if the peer is using countermeasures and our
requested group was deemed unsuitable.
We may receive multiple anti-clogging request messages. We memdup the
token every time, without checking whether memory for one has already
been allocated. Free the old token prior to allocating a new one.
The group was not checked at all. The specification doesn't
mention doing so specifically, but we are only likely to receive an Anti
Clogging Token Request message once we have sent our initial Commit. So
the group should be something we could have sent or might potentially be
able to use.
In case an exceptional condition occurs, handle this more consistently
by returning the following errors:
-ENOMSG -- If a message results in the retransmission timer t0 being
restarted without actually sending anything.
-EBADMSG -- If a received message is to be silently discarded without
affecting the t0 timer.
-ETIMEDOUT -- If SYNC_MAX has been exceeded
-EPROTO -- If a fatal protocol error occurred
Now that sae_verify_* methods no longer allow dropped frames though,
there's no reason to keep these checks. sae_process_commit and
sae_process_confirm will now always receive messages in their respective
state.
sae_verify_* functions were correctly marking frames to be dropped, but
were returning 0, which caused the to-be-dropped frames to be further
processed inside sae_rx_authenticate. Fix that by returning a proper
error.
Make sure to return -EAGAIN whenever a received frame from the peer
results in a retransmission. This also prevents the frame from being
mistakenly processed further in sae_rx_authenticate.
Do not try to transition to a new state from sae_send_commit /
sae_send_confirm since these methods can be called due to
retransmissions or other unexpected messages. Instead, transition to
the new state explicitly from sae_process_commit / sae_process_confirm.
SAE protocol is meant to authenticate peers simultaneously. Hence it
includes a tie-breaker provision in case both peers enter into the
Committed state and the Commit messages arrive at the respective peers
near simultaneously.
However, in the case of STA or Infrastructure mode, only one peer (STA)
would normally enter the Committed state (via Init) and the tie-breaker
provision is not needed. If this condition is detected, abort the
connection.
Also remove the uneeded group change check in process_commit.
sae_compute_pwe doesn't really depend on the state of sae_sm. Only the
curve to be used for the PWE calculation is needed. Rework the function
signature to reflect that and remove unneeded member of struct sae_sm.
This fixes an infinite loop issue when authenticate frames time
out. If the AP is not responding IWD ends up retrying indefinitely
due to how SAE was handling this timeout. Inside sae_auth_timeout
it was actually sending another authenticate frame to reject
the SAE handshake. This, again, resulted in a timeout which called
the SAE timeout handler and repeated indefinitely.
The kernel resend behavior was not taken into account when writing
the SAE timeout behavior and in practice there is actually no need
for SAE to do much of anything in response to a timeout. The
kernel automatically resends Authenticate frames 3 times which mirrors
IWDs SAE behavior anyways. Because of this the authenticate timeout
handler can be completely removed, which will cause the connection
to fail in the case of an autentication timeout.
If there is an associate timeout, retry a few times in case
it was just a fluke. At this point SAE is fully negotiated
so it makes sense to attempt to save the connection.
Use a constant control flow in the derivation loop, avoiding leakage
in the iteration succesfuly converting the password.
Increase number of iterations (20 to 30) to avoid issues with
passwords needing more iterations.
The commit/confirm processing was incorrectly subtracting 2 from
the length when they should be subtracting 6. As with the other
similar change, the length is validated with mpdu_validate so
subtracting 6 will not cause an overflow.
This function was returning a boolean and the expected return was
a signed integer. Since this function actually returned false in
all cases the check for a success (0) return always worked.
The comment about the 'standard code path' was removed as this is
no longer valid.
If an authentication frame of length <= 5 is sent sae will overflow an
integer. The original cause of this was due to incorrectly using the
sizeof(struct mmpdu_header). The header can be either 24 or 28 bytes
depending on fc.order. sizeof does not account for this so 28 is always
the calculated length.
This, in addition to hostapd not including a group number when rejecting,
cause this erroneous length calculation to be worked around as seen in
the removed comment. The comment is still valid (and described again
in another location) but the actual check for len == 4 is not correct.
To fix this we now rely on mpdu_validate to check that the authentication
frame is valid, and then subtract the actual header length using
mmpdu_header_len rather than sizeof. Doing this lets us also remove the
length check since it was validated previously.
It is possible for a zero-length anti-clogging token payload to cause
IWD to abort. If the length passed into sae_process_anti_clogging was
1, l_memdup would be called with a size of -1. This will cause malloc
to abort.
Fix this by checking for a minimum packet length and dropping the
packet if the length is too small.
SAE was a bit trickier than OWE/FILS because the initial implementation
for SAE did not include parsing raw authenticate frames (netdev skipped
the header and passed just the authentication data). OWE/FILS did not
do this and parse the entire frame in the RX callbacks. Because of this
it was not as simple as just setting some RX callbacks. In addition,
the TX functions include some of the authentication header/data, but
not all (thanks NL80211), so this will require an overhaul to test-sae
since the unit test passes frames from one SM to another to test the
protocol end-to-end (essentially the header needs to be prepended to
any data coming from the TX functions for the end-to-end tests).
SAE was behaving inconsitently with respect to freeing the state.
It was freeing the SM internally on failure, but requiring netdev
free it on success.
This removes the call to sae_sm_free in sae.c upon failure, and
instead netdev frees the SM in the complete callback in all cases
regardless of success or failure.
It was assumed that the hunt-and-peck loop was guarenteed to find
a PWE. This was incorrect in terms of kernel support. If a system
does not have support for AF_ALG or runs out of file descriptors
the KDFs may fail. The loop continued to run if found == false,
which is also incorrect because we want to stop after 20 iterations
regarless of success.
This changes the loop to a for loop so it will always exit after
the set number of iterations.
Hostapd has now been updated to include the group number when rejecting
the connection with UNSUPP_FINITE_CYCLIC_GROUP. We still need the existing
len == 0 check because old hostapd versions will still behave this way.
James Prestwood