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. 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.) > 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. > 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. 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). 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). > 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? > > > @@ -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 > 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. -- Jouni Malinen PGP id EFC895FA _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
