EAP-Success might come in with an identifier that is incremented by 1
from the last Response packet. Since identifier field is a byte, the
value might overflow (from 255 -> 0.) This overflow isn't handled
properly resulting in EAP-Success/Failure packets with a 0 identifier
due to overflow being erroneously ignored. Fix that.
src/eap.c: In function 'eap_rx_packet':
src/eap.c:419:50: error: 'vendor_type' may be used uninitialized in this function [-Werror=maybe-uninitialized]
419 | (type == EAP_TYPE_EXPANDED && vendor_id == (id) && vendor_type == (t))
| ^~
src/eap.c:430:11: note: 'vendor_type' was declared here
430 | uint32_t vendor_type;
It isn't clear why GCC complains about vendor_type, but not vendor_id.
But in all cases if type == EAP_TYPE_EXPANDED, then vendor_type and
vendor_id are set. Silence this spurious warning.
Expanded packets with a 0 vendor id need to be treated just like
non-expanded ones. This led to very nasty looking if statements
throughout this function. Fix that by introducing a nested function
to take care of the response type normalization. This also allows us to
drop uninitialized_var usage.
Expanded Nak packet contains (possibly multiple) 8 byte chunks that
contain the type (1 byte, always '254') vendor-id (3 bytes) and
vendor-type (4) bytes.
Unfortunately the current logic was reading the vendor-id at the wrong
offset (0 instead of 1) and so the extracted vendor-type was incorrect.
Fixes: 17c569ba4c ("eap: Add authenticator method logic and API")
If we received a Nak or an Expanded Nak packet, the intent was to print
our own method type. Instead we tried to print the Nak type contents.
Fix that by always passing in our method info to eap_type_to_str.
Fixes: 17c569ba4c ("eap: Add authenticator method logic and API")
The goal is to add specifically EAP-WSC registrar side and it looks like
extending our EAP and EAPoL code to support both supplicant and
authenticator-side methods is simpler than adding just EAP-WSC as a
special case.
Since EAP-WSC always ends in an EAP failure, I haven't actually tested
the success path.
Replace the usage of eap_send_response() in the method implementations
with a new eap_method_respond that skips the redundant "type" parameter.
The new eap_send_packet is used inside eap_method_respond and will be
reused for sending request packets in authenticator side EAP methods.
This was refactored to set the mtu via __eap_set_config rather than
passing the MTU into eap_init. This makes eap work in a similar fashion
as eapol (i.e. __eapol_set_config).
If __eap_set_config is not used, the MTU will be set to 1020, which is
the same as previously passing 0 to eap_init.
Many operations performed during an error in load_settings were the same
as the ones performed when freeing the eap object. Add eap_free_common
to unify these.
EAP identites are recommended to follow RFC 4282 (The Network Access
Identifier). This RFC recommends a maximum NAI length of 253 octets.
It also mentions that RADIUS is only able to support NAIs of 253
octets.
Because of this, IWD should not allow EAP identities larger than 253
bytes. This change adds a check in eap_load_settings to verify the
identity does not exceed this limit.
Replace existing uses of memset to clear secrets with explicit_bzero to
make sure it doesn't get optimized away. This has some side effects as
documented in gcc docs but is still recommended.
In eap_secret_info_free make sure we clear both strings in the case of
EAP_SECRET_REMOTE_USER_PASSWORD secrets.
Some of the EAP methods don't require a clear-text identity to
be sent with the Identity Response packet. The mandatory identity
filed has resulted in unnecessary transmission of the garbage
values. This patch makes the Identity field to be optional and
shift responsibility to ensure its existence to the individual
methods if the field is required. All necessary identity checks
have been previously propagated to individual methods.
eap_append_secret now takes a new cache_policy parameter which can be
used by the EAP method to signal that the value received from the agent
is to never be cached, i.e. each value can only be used once. The
parameter value should be EAP_CACHE_NEVER for this and we use this in
value EAP-GTC where the secret tokens are one time use. The
EAP_CACHE_TEMPORARY value is used in other methods, it preserves the
default behaviour where a secret can be cached for as long as the
network stays in range (this is the current implementation more than a
design choice I believe, I didn't go for a more specific enum name as
this may still change I suppose).
Since PEAP & TTLS expect to use eap_check_settings recursively, make
them use a private version of that API that does not perform cleanup and
can contain side-effects.
eap_check_settings itself will guarantee that no side effects happen on
error. It is meant to be used by code outside of the eap subsystem.
Missing secrets are freed by eap_send_agent_req() even in case of
failure, so it was erroneous to try to free them on error.
==1048== Invalid read of size 8
==1048== at 0x1603EC: l_queue_clear (queue.c:101)
==1048== by 0x1603B8: l_queue_destroy (queue.c:82)
==1048== by 0x135328: network_connect_8021x (network.c:943)
==1048== by 0x1354C4: network_connect (network.c:987)
==1048== by 0x178DD2: _dbus_object_tree_dispatch (dbus-service.c:1690)
==1048== by 0x16D32A: message_read_handler (dbus.c:285)
==1048== by 0x166EC3: io_callback (io.c:123)
==1048== by 0x165A1A: l_main_iterate (main.c:376)
==1048== by 0x165B58: l_main_run (main.c:423)
==1048== by 0x1102DA: main (main.c:458)
==1048== Address 0x5461850 is 0 bytes inside a block of size 24 free'd
==1048== at 0x4C2C13B: free (vg_replace_malloc.c:530)
==1048== by 0x15ED03: l_free (util.c:136)
==1048== by 0x1603C4: l_queue_destroy (queue.c:83)
==1048== by 0x134BD5: eap_secret_request_free (network.c:719)
==1048== by 0x134EF9: eap_send_agent_req (network.c:817)
==1048== by 0x1352F7: network_connect_8021x (network.c:936)
==1048== by 0x1354C4: network_connect (network.c:987)
==1048== by 0x178DD2: _dbus_object_tree_dispatch (dbus-service.c:1690)
==1048== by 0x16D32A: message_read_handler (dbus.c:285)
==1048== by 0x166EC3: io_callback (io.c:123)
==1048== by 0x165A1A: l_main_iterate (main.c:376)
==1048== by 0x165B58: l_main_run (main.c:423)
In eap_check_settings move the check for the EAP-Identity setting so
that the method's check_setting call back has a chance to request it
from the agent. Note the check can be also moved to the EAP methods
so that they are free to skip it if not NULL identity is ok.
Replace usages of l_settings_get_value with l_settings_get_string, which
will make sure the returned strings are unescaped but also allocates
memeory and forces us to use l_free on most of the strings. Some of
these strings we explicitly set with l_settings_set_string() in our code
so when we retrieved them with l_settings_get_value() we would receive a
different string if there were any escapable characters in the string.
I didn't replace any of the l_settings_get_value() uses where we're just
checking whether a setting is present, or those which are hexstrings or
EAP method names assuming that they can't have any special characters,
although this isn't future proof. I did use l_settings_get_string() for
file paths though.
Accept two setting IDs in eap_append_secret, first for the username and
second for the password in case of the EAP_SECRET_REMOTE_USER_PASSWORD
EAP secret type. In all other cases only the first setting is used.
Until now for EAP_SECRET_REMOTE_USER_PASSWORD secrets we'd generate the
two setting names by adding different suffixes to the ID parameter.
Using the two different setting names automatically fixes the issues
with using the EAP Identity returned by the agent in EAP-MSCHAPv2 and
EAP-PWD.
This is meant to reset the EAP state back to its original state without
affecting any state variables obtained through load_settings. This can
be useful for EAP Reauthentication triggered by the AP.
Some EAP servers might try to send us packets after the EAP connection
has been established. When EAP succeeds we destroy the EAP object. If
a new EAP request arrives we create a temporary EAP object to handle the
request (most likely to NAK it). However, if the packet is not destined
to a particular method (e.g. it is a notification) the current logic can
result in a crash.
src/netdev.c:netdev_set_gtk() 3
==4300== Invalid read of size 8
==4300== at 0x14204B: __eap_handle_request (eap.c:203)
==4300== by 0x142339: eap_rx_packet (eap.c:287)
==4300== by 0x12AEF9: eapol_rx_packet (eapol.c:1622)
==4300== by 0x12BBBC: __eapol_rx_packet (eapol.c:2018)
==4300== by 0x116D1E: netdev_pae_read (netdev.c:3121)
==4300== by 0x16672B: io_callback (io.c:123)
==4300== by 0x165239: l_main_iterate (main.c:376)
==4300== by 0x16537D: l_main_run (main.c:423)
==4300== by 0x10F95C: main (main.c:447)
==4300== Address 0x30 is not stack'd, malloc'd or (recently) free'd
==4300==
When the server sends an identity prompt or a notification, we were
trying to print from our local buffer, not from the actual packet. The
relevant valgrind trace is:
src/netdev.c:netdev_mlme_notify() MLME notification 64
==4300== Conditional jump or move depends on uninitialised value(s)
==4300== at 0x4C3006E: strnlen (vg_replace_strmem.c:425)
==4300== by 0x508C513: vfprintf (vfprintf.c:1643)
==4300== by 0x508EB75: buffered_vfprintf (vfprintf.c:2329)
==4300== by 0x508C1A1: vfprintf (vfprintf.c:1301)
==4300== by 0x167051: log_stderr (log.c:145)
==4300== by 0x16756E: l_log_with_location (log.c:293)
==4300== by 0x142173: __eap_handle_request (eap.c:235)
==4300== by 0x142339: eap_rx_packet (eap.c:287)
==4300== by 0x12AEF9: eapol_rx_packet (eapol.c:1622)
==4300== by 0x12BBBC: __eapol_rx_packet (eapol.c:2018)
==4300== by 0x116D1E: netdev_pae_read (netdev.c:3121)
==4300== by 0x16672B: io_callback (io.c:123)
==4300==
EAP identity prompt: ""
These EAP methods do not store the identity inside the settings file
since it is obtained from the SIM card, then provided to IWD via
get_identity method. If the get_identity method is implemented, do
not fail the settings check when EAP-Identity is missing.
With the goal of requesting the required passwords/passphrases, such as
the TLS private key passphrase, from the agent, add a static method
eap_check_settings to validate the settings and calculate what passwords
are needed for those settings, if any. This is separate from
eap_load_settings because that can only be called later, once we've
got an eap state machine object. We need to get all the needed EAP
credentials from the user before we even start connecting.
While we do this, we also validate the settings and output any error
messages through l_error (this could be changed so the messages go
somewhere else in the future), so I removed the error messages from
eap_load_settings and that method now assumes that eap_check_settings
has been called before.
eap_check_settings calls the appropriate method's .check_settings method
if the settings are complete enough to contain the method name. The
policy is that any data can be provided inside the l_settings object
(from the network provisioning/config file), but some of the more
sensitive fields, like private key passwords, can be optionally omitted
and then the UI will ask for them and iwd will be careful with
caching them.
Within struct eap_secret_info, "id" is mainly for the EAP method to
locate the info in the list. "value" is the actual value returned
by agent. "parameter" is an optional string to be passed to the agent.
For a private key passphrase it may be the path to the key file, for a
password it may be the username for which the password is requested.
1. Enforce implementation of handle_request function
2. In case of unimplemented handle_retransmit try to use
handle_request instead and rely on method specific
mechanism to restart the conversation if necessary
3. Make method->free implementation unrequired
