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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
