Re: [PATCH 1/3] nl80211: Migrate from set_tx to key_flag API

Am 01.03.20 um 16:59 schrieb Jouni Malinen:
> On Sat, Feb 29, 2020 at 01:47:02PM +0100, Alexander Wetzel wrote:
>> Isn't dynamic WEP using broadcast keys and only individual keys would be
>> affected? Whatever the right name is: Looks like it needs more attention in
>> the next patches.
> 
> As far as the IEEE 802.11 standard is concerned, WEP was defined to have
> four "default keys" (KeyID 0..3) and optional use of a separate key with
> "key mapping" (for unicast frames; uses KeyID 0). However, there were
> deployments of IEEE 802.1X "dynamic WEP" where separate unicast keys
> were used and they could use any of the four KeyIDs. The AP could, for
> example, alternate between KeyID 1 and 2 for the default (broadcast)
> keys and KeyID 0 and 3 for the individual (unicast) keys. More than one
> key could be configured and as such, there was a need to select which
> one of the configured keys to use for TX.
> 

Probably not relevant but since I mostly learned what WEP can and can't 
do by reading the code:
WEP only can use max four keys (KeyID 0..3) regardless how many of them 
are unicast and broadcast, correct? We can't e.g. install four broadcast 
and four unicast keys and thus have eight keys active? (I think that 
would actual work at the moment with at least mac80211.)

> This is obviously all pretty historic and ideally, we would not need to
> care about WEP anymore, but well, it's still there in the
> implementations today.. (Well, at least for wpa_supplicant/hostapd there
> is now CONFIG_WEP=y without which all the WEP stuff disappears.. Maybe
> to be removed completely in the v2.11.)
> 

I never thought I would have to learn the in and outs of WEP to get 
Extended Key ID implemented:-) But then we nearly have figured it out 
the main problem seems to be semantic now. Let's see what you think of 
the next patch and have a special eye on WEP handling there...

>> For unicast WEP keys we decided to handle them identical to pairwise keys
>> and should now probably use KEY_FLAG_PAIRWISE_RX and later
>> KEY_FLAG_PAIRWISE_RX_TX_MODIFY to activate them, I guess.
> 
> That old, but actually deployed, design of using any of the KeyIDs for
> unicast WEP is not fully compatible with the current pairwise key design
> in RSN. I guess it could be called very similar if comparing it to the
> KeyID 0+1 case with the only high level difference being in any of the
> four KeyIDs being available instead of fixed 0 and 1.
> 

Agree and that is how I plan to do it in the next patch now. But I 
simply do not see any code in hostapd/wpa_supplicant which actually 
tries to switch a WEP unicast key to a different keyID. When I did not 
miss anything it can only work by a full key install.
At the moment it looks like we don't have to care about switching KeyIDs 
for already installed WEP keys without fully reinstalling the Key(ID) 
and I doubt anyone is interested to add that for WEP nowadays.

Now I found one missing key_flag related to WEP (patch will be included 
in the next set) but besides that the current key_flags for WEP keys 
seems to be sufficient. I'm just struggling a bit when we have to call 
SET_KEY:

The current code seems to only call SET_KEY with 
NL80211_KEY_DEFAULT_TYPE_UNICAST for unicast WEP keys. I would have 
expected that to happen for WEP broadcast keys instead. (And thus tell 
the driver that this key has to be used also for unicast frames.)

Mac80211 and thus hwsim should not care about what we do with with 
SET_KEY after installing an unicast key. The last installed pairwise key 
will be used. I guess I'll simply make sure to do it identical for WEP 
as the current code.

>> Since we don't activate installed keys without reinstalling them at the
>> moment I guess we simply can use KEY_FLAG_PAIRWISE_RX and
>> KEY_FLAG_PAIRWISE_RX_TX and could keep the check.
>> Or we indeed allow KEY_FLAG_PAIRWISE to be paired with KEY_FLAG_DEFAULT.
>>
>> I'm leaning to go for KEY_FLAG_PAIRWISE_RX/KEY_FLAG_PAIRWISE_RX_TX at the
>> moment. Do you have any preferences?
> 
> I think we need to look at that "not activate installed keys without
> reinstalling" part.. Isn't that exactly what should be done with the
> PTK0+1 rekeying? This did not make much of a difference for broadcast
> since AP was the only entity transmitting those, but for unicast, there
> is a race condition on when _either_ device moves to using the new key.
> 

Yes, PTK0+1 - assuming we use that now for Extended Key ID - rekeying 
will switch key TX without installing the key again. But I think that 
will be the first time we do that that.

Note that this also solves the races: Extended Key ID has starting with 
the first rekey always two unicast keys per STA active. Both active for 
RX but only one for TX. So within a reasonable time frame (basically the 
rekey interval) all STAs are free to use the old or the new key with the 
knowledge that the other can decrypt it. (The requirement of supporting 
two unicast keys per STA at the same time prevents many drivers to 
support it)

> Ideally, both the AP and STA would first configure the new TK in place
> without enabling it for TX so that it is ready for RX whenever the other
> device starts transmitting. This would then be followed by another
> operation that changes the TX key from the previously used to the new
> one once there is sufficient reason to believe that the other device has
> had chance to have it key configured (e.g., STA could configure new key
> for RX before sending EAPOL-Key msg 4/4 and I guess the AP could do that
> before sending EAPOL-Key msg 3/4, so reception of these frames would be
> sufficient and robust indication).
> 

You are basically quoting the Extended Key ID specification here:-)
Exactly that is what the key_flag API was designed for.

> Or is that KEY_FLAG_PAIRWISE_RX and KEY_FLAG_PAIRWISE_RX_TX_MODIFY is
> meant to do? One important point here is that the key must not be
> configured again (due to KRACK protections) and instead, only the state
> of the configured key is to be chanted from RX-only to TX+RX (or in the
> language of setting a default TX key, change the default TX Key ID to
> the other one).
> 

KEY_FLAG_PAIRWISE_RX_TX_MODIFY is basically only telling the driver to 
use the key belonging to keyidx for this STA starting now. Theoretically 
we even couls back and forth between two installed keys and each would 
continue to use its PN values.
All other set_key parameters - including the key - will be simply 
ignored for that flag. Note that pairwise key installs unconditional 
activated a key for TX, too. Mac80211 needed patches to prevent a 
pairwise key install to also set the key for TX.

>> With Extended Key ID we use KEY_FLAG_MODIFY to tell the driver that an
>> installed (pairwise) key only needs an update of its TX status.
>> KEY_FLAG_DEFAULT is only intended to be combined with group/broadcast keys
>> and tries to clean up the inconsistent use set_tx has for pairwise keys.
>> (Where if I remember it right it's generally set in wpa_supplicant and never
>> in hostapd.)
> 
> So will KEY_FLAG_MODIFY make the device change its TX key from old to
> new without reconfiguring either actual key (TK) value or PN/RSC
> counters? Is this done with the new key (i.e., move from RX-only to
> TX+RX) or the old key (i.e., move from TX+RX to RX-only) or both? Or
> would a single operation take care of both those steps?
> 

Yes, all counters are unaffected.
KEY_FLAG_MODIFY can only be used for already installed keys and only be 
used to add TX to an installed key. (Removing TX from the old is 
implicit by setting the new key and in fact atomic in the kernel.)

But when we want to use KEY_FLAG_MODIFY without Extended Key ID we would 
have to do that internally with the same nl80211 calls as we use today.

Nevertheless this is only theoretical at the moment: We do not need it 
to migrate to key_flag API at all since we never switch a key without a 
(re)install so far.

> 
>>>> @@ -3226,8 +3235,6 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
>>>>    	}
>>>>    	ret = send_and_recv_msgs(drv, msg, NULL, NULL);
>>>> -	if (ret == -ENOENT)
>>>> -		ret = 0;
>>>
>>> I'd assume this is because the error cannot happen anymore with the
>>> error cases removed, but that was not obvious from the full context of
>>> this patch. Splitting the NL80211_CMD_SET_KEY changes to a separate
>>> patch with the commit message highlighting that detail would be helpful
>>> for understanding this.
> 
>> I could not find any user for that nor a reason we should ignore that. When
>> the driver is not able to set a key to default claiming success only helps
>> hiding what went really wrong.
>>
>> This is comparable old, the initial git commit using version 0.6.3 as seed
>> already has it. (The corresponding function was named i802_set_encryption()
>> at that time.) The oldest version using it I could find was hostapd-0.6.2
>> with what seems to be the time the netlink API got introduced.
> 
> This is really old and seems to come from the initial conversion of
> ioctl-based (hostap driver) interface:
> 
> https://w1.fi/cgit/hostap-history/commit/?id=8a6ae9063ca4c0d4bc87afbdaf7c703397cc19bf
> 

Ah, next time I know where to look for ancient history:-)

>> Is there any known reason to ignore the error? I commented it out and did a
>> test run without triggering a set_key error.
> 
> It looks like the ioctl-based design used a single command to
> add/set/delete a key and for that, handling ENOENT for deletion seems
> reasonable. I guess that got converted into nl80211 to apply to both
> NL80211_CMD_NEW/DEL_KEY and NL80211_CMD_SET_KEY without any particular
> use case in mind (it would still be of use with _DEL_KEY). As such, I
> think it is safe to remove this from _SET_KEY. I just want to make this
> history as part of that separate commit message since no one is going to
> remember this detail if ever having to look at that change again.. ;-)
> 
>> I guess we also should check if the exception for the first netlink call
>> should not be thrown out, too. When we can't install the "basic" key how
>> should it be possible to activate it? And what do we win by ignoring the
>> errors and than assuming the operation worked?
>> Looks like this risks sending out frames unencrypted...
> 
> My feeling is that it is really only applicable to DEL_KEY and as such,
> I'd welcome a patch removing it from _NEW_KEY anf _SET_KEY with the
> details above justifying why that is a good thing to do.
> 

Thanks for the confirmation.

I'll keep it for the DEL_KEY call but drop it unconditionally from the 
SET_KEY part which never can be called when we delete the key.

Alexander