| Message ID | 20231205154647.1778389-4-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] scan: parse password identifier/exclusive bits | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-ci-gitlint | success | GitLint |
Hi James, On 12/5/23 09:46, James Prestwood wrote: > Adds a new network profile setting [Security].PasswordIdentifier. > When set (and the BSS enables SAE password identifiers) the network > and handshake object will read this and use it for the SAE > exchange. > > Loading the PSK will fail if there is no password identifier set > and the BSS sets the "exclusive" bit. If a password identifier is I'm not so sure about this. The trouble is that this logic is sufficient for the initial connection, but isn't sufficient when you consider re-association. > set and the BSS doesn't indicate support the setting will be ignored > (with a debug print). > --- > src/network.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > <snip> > @@ -641,6 +657,32 @@ static int network_load_psk(struct network *network, struct scan_bss *bss) > psk_len = 0; > } > > + /* > + * Sort out if the password identifier is required, should be used, " > + * or should be ignored. > + */ > + if (is_sae) { > + if (bss->sae_pw_id_exclusive && !password_id) { This likely needs to be taken into consideration much later, when building the actual handshake state. > + l_error("BSS requires SAE password identifiers, check " > + "[Security].PasswordIdentifier"); > + return -ENOKEY; > + } > + > + /* > + * If the profile contains a password identifier but the network > + * does not support it IWD will still attempt to connect. The Password identifier is only used by SAE H2E. One can easily imagine a weird network of mixed APs with some being SAE H2E, some not. This is why I didn't bother implementing this, it is a half-baked feature. > + * caveat here is if the connection is successful the sync will > + * remove the password identifier entry. Though this might be > + * unexpected to the user, retaining this (invalid) setting > + * isn't worth special casing. And this doesn't sound nice at all... I think the setting should be preserved. > + */ > + if (!bss->sae_pw_id_used && password_id) { > + l_debug("[Security].PasswordIdentifier set but BSS " > + "does not not use password identifiers"); > + l_free(l_steal_ptr(password_id)); > + } > + } > + > /* PSK can be generated from the passphrase but not the other way */ > if (!psk || is_sae) { > if (!passphrase) Regards, -Denis
Hi Denis, On 12/6/23 09:08, Denis Kenzior wrote: > Hi James, > > On 12/5/23 09:46, James Prestwood wrote: >> Adds a new network profile setting [Security].PasswordIdentifier. >> When set (and the BSS enables SAE password identifiers) the network >> and handshake object will read this and use it for the SAE >> exchange. >> >> Loading the PSK will fail if there is no password identifier set >> and the BSS sets the "exclusive" bit. If a password identifier is > > I'm not so sure about this. The trouble is that this logic is > sufficient for the initial connection, but isn't sufficient when you > consider re-association. Your right, roaming would be entirely broken between BSS's that mismatch using password identifiers. Maybe even hunt-and-peck and H2E? not entirely sure. We would need to re-derive the point for each roam, like in network_set_handshake_secrets_psk(). > >> set and the BSS doesn't indicate support the setting will be ignored >> (with a debug print). >> --- >> src/network.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 49 insertions(+), 1 deletion(-) >> > > <snip> > >> @@ -641,6 +657,32 @@ static int network_load_psk(struct network >> *network, struct scan_bss *bss) >> psk_len = 0; >> } >> + /* >> + * Sort out if the password identifier is required, should be >> used, " >> + * or should be ignored. >> + */ >> + if (is_sae) { >> + if (bss->sae_pw_id_exclusive && !password_id) { > > This likely needs to be taken into consideration much later, when > building the actual handshake state. Yeah, we'd need to move this into network_set_handshake_secrets_psk and rederive the points. And actually if we do this storing the points in the network profile doesn't make a whole lot of sense anymore since its being rederived every time. Alternatively we just keep it how I have it and tell they user they're network isn't configured properly :) > >> + l_error("BSS requires SAE password identifiers, check " >> + "[Security].PasswordIdentifier"); >> + return -ENOKEY; >> + } >> + >> + /* >> + * If the profile contains a password identifier but the >> network >> + * does not support it IWD will still attempt to connect. The > > Password identifier is only used by SAE H2E. One can easily imagine a > weird network of mixed APs with some being SAE H2E, some not. This is > why I didn't bother implementing this, it is a half-baked feature. Ugh, ok. I guess we have to keep the identifier around then. > >> + * caveat here is if the connection is successful the sync will >> + * remove the password identifier entry. Though this might be >> + * unexpected to the user, retaining this (invalid) setting >> + * isn't worth special casing. > > And this doesn't sound nice at all... I think the setting should be > preserved. > >> + */ >> + if (!bss->sae_pw_id_used && password_id) { >> + l_debug("[Security].PasswordIdentifier set but BSS " >> + "does not not use password identifiers"); >> + l_free(l_steal_ptr(password_id)); >> + } >> + } >> + >> /* PSK can be generated from the passphrase but not the other >> way */ >> if (!psk || is_sae) { >> if (!passphrase) > > Regards, > -Denis >
Hi James, >>> Loading the PSK will fail if there is no password identifier set >>> and the BSS sets the "exclusive" bit. If a password identifier is >> >> I'm not so sure about this. The trouble is that this logic is sufficient for >> the initial connection, but isn't sufficient when you consider re-association. > Your right, roaming would be entirely broken between BSS's that mismatch using > password identifiers. Maybe even hunt-and-peck and H2E? not entirely sure. We Well, ReAssociate would just use SAE passphrase directly, so it would work in theory... But it is a bit of a strange case. > would need to re-derive the point for each roam, like in > network_set_handshake_secrets_psk(). ?? You mean SAE-H2E with password identifier for BSSes that report exclusive/in-use bit and SAE-H2E for BSSes without? Or something else? >> >> This likely needs to be taken into consideration much later, when building the >> actual handshake state. > > Yeah, we'd need to move this into network_set_handshake_secrets_psk and rederive > the points. And actually if we do this storing the points in the network profile > doesn't make a whole lot of sense anymore since its being rederived every time. I would hate for this to be the outcome. Re-deriving the PT is pretty expensive. > > Alternatively we just keep it how I have it and tell they user they're network > isn't configured properly :) I think it could be argued that if PasswordIdentifier is set, then any BSSes that are not H2E/do not set the in-use bit are not connectable. Regards, -Denis
On 12/6/23 11:44, Denis Kenzior wrote: > Hi James, > >>>> Loading the PSK will fail if there is no password identifier set >>>> and the BSS sets the "exclusive" bit. If a password identifier is >>> >>> I'm not so sure about this. The trouble is that this logic is >>> sufficient for the initial connection, but isn't sufficient when you >>> consider re-association. >> Your right, roaming would be entirely broken between BSS's that >> mismatch using password identifiers. Maybe even hunt-and-peck and >> H2E? not entirely sure. We > > Well, ReAssociate would just use SAE passphrase directly, so it would > work in theory... But it is a bit of a strange case. > >> would need to re-derive the point for each roam, like in >> network_set_handshake_secrets_psk(). > > ?? You mean SAE-H2E with password identifier for BSSes that report > exclusive/in-use bit and SAE-H2E for BSSes without? Or something else? Yeah, I'm talking about multiple H2E BSS's that set or don't set the exclusive/in-use bits. Maybe it would be ok actually. I got concerned when I saw the points being set into the handshake in network_set_handshake_secrets_psk() (which happens on roaming) but it looks like this is only used for initial SAE association. Maybe roaming/FT would be ok? But this is all moot if we just bail early if the password identifier setting does not match the BSS's capabilities. >>> >>> This likely needs to be taken into consideration much later, when >>> building the actual handshake state. >> >> Yeah, we'd need to move this into network_set_handshake_secrets_psk >> and rederive the points. And actually if we do this storing the >> points in the network profile doesn't make a whole lot of sense >> anymore since its being rederived every time. > > I would hate for this to be the outcome. Re-deriving the PT is pretty > expensive. > >> >> Alternatively we just keep it how I have it and tell they user >> they're network isn't configured properly :) > > I think it could be argued that if PasswordIdentifier is set, then any > BSSes that are not H2E/do not set the in-use bit are not connectable. This works for me. Much easier and will enforce a properly configured network. > > Regards, > -Denis
diff --git a/src/network.c b/src/network.c index 79f964b2..d422b282 100644 --- a/src/network.c +++ b/src/network.c @@ -70,6 +70,7 @@ struct network { struct network_info *info; unsigned char *psk; char *passphrase; + char *password_identifier; struct l_ecc_point *sae_pt_19; /* SAE PT for Group 19 */ struct l_ecc_point *sae_pt_20; /* SAE PT for Group 20 */ unsigned int agent_request; @@ -124,6 +125,13 @@ static void network_reset_passphrase(struct network *network) network->passphrase = NULL; } + if (network->password_identifier) { + explicit_bzero(network->password_identifier, + strlen(network->password_identifier)); + l_free(network->password_identifier); + network->password_identifier = NULL; + } + if (network->sae_pt_19) { l_ecc_point_free(network->sae_pt_19); network->sae_pt_19 = NULL; @@ -317,7 +325,8 @@ static struct l_ecc_point *network_generate_sae_pt(struct network *network, l_debug("Generating PT for Group %u", group); pt = crypto_derive_sae_pt_ecc(group, network->ssid, - network->passphrase, NULL); + network->passphrase, + network->password_identifier); if (!pt) l_warn("SAE PT generation for Group %u failed", group); @@ -462,6 +471,10 @@ static int network_set_handshake_secrets_psk(struct network *network, handshake_state_set_passphrase(hs, network->passphrase); + if (network->password_identifier) + handshake_state_set_password_identifier(hs, + network->password_identifier); + if (ie_rsnxe_capable(hs->authenticator_rsnxe, IE_RSNX_SAE_H2E)) { l_debug("Authenticator is SAE H2E capable"); @@ -631,6 +644,9 @@ static int network_load_psk(struct network *network, struct scan_bss *bss) _auto_(l_free) char *passphrase = l_settings_get_string(network->settings, "Security", "Passphrase"); + _auto_(l_free) char *password_id = + l_settings_get_string(network->settings, "Security", + "PasswordIdentifier"); _auto_(l_free) char *path = storage_get_network_file_path(security, ssid); @@ -641,6 +657,32 @@ static int network_load_psk(struct network *network, struct scan_bss *bss) psk_len = 0; } + /* + * Sort out if the password identifier is required, should be used, " + * or should be ignored. + */ + if (is_sae) { + if (bss->sae_pw_id_exclusive && !password_id) { + l_error("BSS requires SAE password identifiers, check " + "[Security].PasswordIdentifier"); + return -ENOKEY; + } + + /* + * If the profile contains a password identifier but the network + * does not support it IWD will still attempt to connect. The + * caveat here is if the connection is successful the sync will + * remove the password identifier entry. Though this might be + * unexpected to the user, retaining this (invalid) setting + * isn't worth special casing. + */ + if (!bss->sae_pw_id_used && password_id) { + l_debug("[Security].PasswordIdentifier set but BSS " + "does not not use password identifiers"); + l_free(l_steal_ptr(password_id)); + } + } + /* PSK can be generated from the passphrase but not the other way */ if (!psk || is_sae) { if (!passphrase) @@ -655,6 +697,7 @@ static int network_load_psk(struct network *network, struct scan_bss *bss) network_reset_passphrase(network); network_reset_psk(network); network->passphrase = l_steal_ptr(passphrase); + network->password_identifier = l_steal_ptr(password_id); if (network_settings_load_pt_ecc(network, path, 19, &network->sae_pt_19) > 0) @@ -726,6 +769,11 @@ static void network_settings_save(struct network *network, l_settings_set_string(settings, "Security", "Passphrase", network->passphrase); + if (network->password_identifier) + l_settings_set_string(settings, "Security", + "PasswordIdentifier", + network->password_identifier); + if (network->sae_pt_19) network_settings_save_sae_pt_ecc(settings, network->sae_pt_19);