Re: [PATCH 04/10] network: add support for SAE password identifiers

[James Prestwood <prestwoj@gmail.com>]

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
>