| Message ID | 20200227215248.113686-1-alexander@wetzel-home.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] nl80211: Migrate from set_tx to key_flag API | expand |
On Thu, Feb 27, 2020 at 10:52:46PM +0100, Alexander Wetzel wrote: > Stop using set_tx and cleanup/restructure the key install logic > depending on it. > > The updated logic is also no longer incorrectly installing some pairwise > keys as default keys and has additional sanity checks refusing > unexpected keys. Regarding the changes on setting the default pairwise key.. Isn't that preventing some use cases? For example, in IEEE 802.1X with dynamic WEP keys, the Authenticator could configure two different unicast keys with different Key IDs and then swap which key is used for transmission. And wouldn't this apply for the new PTK rekeying case with two Key ID values as well? In general, it would probably make sense to split patch into quite a few smaller patches that each address a clear individual change on their own at least for the clear independent fixes. This patch was a bit difficult to review and I could not come to conclusion that all of it is fine even though most places were clear. > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > + } else { > wpa_printf(MSG_DEBUG, " broadcast key"); > - > - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); > - if (!types || > - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) > - goto fail; > - nla_nest_end(key_msg, types); > } So I take this removal here is because that attribute is not really used in the NL80211_CMD_NEW_KEY case. > @@ -3197,19 +3206,19 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, > if (addr && is_broadcast_ether_addr(addr)) { > struct nlattr *types; > > + wpa_printf(MSG_DEBUG, " group key"); > types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); > if (!types || > nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) > goto fail; > nla_nest_end(key_msg, types); And this is where we have that attribute for NL80211_CMD_SET_KEY when selecting which one of the broadcast keys to use for transmission. > } else if (addr) { > - struct nlattr *types; > - > - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); > - if (!types || > - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST)) > - goto fail; > - nla_nest_end(key_msg, types); > + wpa_printf(MSG_DEBUG, > + "nl80211: Default group key can't use a unicast address"); > + ret = -EINVAL; > + goto fail; But this is the case that would be used for changing which of the multiple unicast keys is used for transmission. Why would this not be an acceptable operation? > @@ -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.
Am 28.02.20 um 10:37 schrieb Jouni Malinen: > On Thu, Feb 27, 2020 at 10:52:46PM +0100, Alexander Wetzel wrote: >> Stop using set_tx and cleanup/restructure the key install logic >> depending on it. >> >> The updated logic is also no longer incorrectly installing some pairwise >> keys as default keys and has additional sanity checks refusing >> unexpected keys. > > Regarding the changes on setting the default pairwise key.. Isn't that > preventing some use cases? For example, in IEEE 802.1X with dynamic WEP > keys, the Authenticator could configure two different unicast keys with > different Key IDs and then swap which key is used for transmission. And 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. 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. 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? > wouldn't this apply for the new PTK rekeying case with two Key ID values > as well? 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.) > > In general, it would probably make sense to split patch into quite a few > smaller patches that each address a clear individual change on their own > at least for the clear independent fixes. This patch was a bit difficult > to review and I could not come to conclusion that all of it is fine even > though most places were clear. > Ok, I'll split it up. While working on the mail I found other oddities so it will probably also do things differently. > >> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c >> + } else { >> wpa_printf(MSG_DEBUG, " broadcast key"); >> - >> - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); >> - if (!types || >> - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) >> - goto fail; >> - nla_nest_end(key_msg, types); >> } > > So I take this removal here is because that attribute is not really used > in the NL80211_CMD_NEW_KEY case. Correct. I remember checking the kernel nl80211 code for that and while it pared the settings they are not passed down to the drivers and do nothing. These attributes only have an effect with NL80211_CMD_SET_KEY. > >> @@ -3197,19 +3206,19 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, >> if (addr && is_broadcast_ether_addr(addr)) { >> struct nlattr *types; >> >> + wpa_printf(MSG_DEBUG, " group key"); >> types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); >> if (!types || >> nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) >> goto fail; >> nla_nest_end(key_msg, types); > > And this is where we have that attribute for NL80211_CMD_SET_KEY when > selecting which one of the broadcast keys to use for transmission. > >> } else if (addr) { >> - struct nlattr *types; >> - >> - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); >> - if (!types || >> - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST)) >> - goto fail; >> - nla_nest_end(key_msg, types); >> + wpa_printf(MSG_DEBUG, >> + "nl80211: Default group key can't use a unicast address"); >> + ret = -EINVAL; >> + goto fail; > > But this is the case that would be used for changing which of the > multiple unicast keys is used for transmission. Why would this not be an > acceptable operation? > I think you are right and I dropped too much here. It's a bit strange this did not cause any tests to fail... I'll add that again for WEP unicast and WPA-NONE Keys. >> @@ -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. Is there any known reason to ignore the error? I commented it out and did a test run without triggering a set_key error. 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... Alexander
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.
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
On Sun, Mar 01, 2020 at 09:10:14PM +0100, Alexander Wetzel wrote: > 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.) As far as the IEEE 802.11 standard is concerned, there can be four default keys (KeyID 0..3) and one mapping (pairwise) key per peer-STA (KeyID 0). There is no constraint on using KeyID for both a mapping key (of which there can be multiple on the AP side and one on the STA side) and a default key. So yes, there could be a default key with KeyID 0 and a key mapping key with the same KeyID 0 and the receiver would need to be able to determine which key to use based on whether the Address 1 value in the frame header is a broadcast/multicast address. I do not know whether anyone has actually ever deployed a device that uses this combination, though. > 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... I don't think it is completely accurate since use of KEY_FLAG_GROUP* with a WEP default key looks confusing to me. Those default keys can be used for both unicast and multicast/broadcast frames. That said, I don't think I want to spend any more effort with WEP, so I think I can live with this until such time that I get to remove all WEP related code from hostap.git.. ;-) PS. The IEEE 802.11 standard defines a cipher suite selector 00-0F-AC:0 "Use group cipher suite". When this cipher suite is used as the pairwise cipher suite, no pairwise cipher is configured. Instead, the group cipher is used for both multicast/broadcast and unicast frames. So there is actually a defined corner case where that comment about regarding WEP default keys does actually apply to RSN as well. I'm only mentioning this here for completeness sake for anyone who really wants to understand all the possible combinations that have been defined. Use of that 00-0F-AC:0 cipher suite selector is strongly discouraged nowadays. It was defined only as a temporary solution to allow software-only update on some devices from WEP to TKIP. In practice, I don't think this ever got deployed, so it is fine to ignore this.
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index 18e4b8eef..964fbd26f 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -3053,7 +3053,6 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, struct nl_msg *msg; struct nl_msg *key_msg; int ret; - int tdls = 0; const char *ifname = params->ifname; enum wpa_alg alg = params->alg; const u8 *addr = params->addr; @@ -3064,6 +3063,7 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, const u8 *key = params->key; size_t key_len = params->key_len; int vlan_id = params->vlan_id; + enum key_flag key_flag = params->key_flag; /* Ignore for P2P Device */ if (drv->nlmode == NL80211_IFTYPE_P2P_DEVICE) @@ -3071,15 +3071,17 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, ifindex = if_nametoindex(ifname); wpa_printf(MSG_DEBUG, "%s: ifindex=%d (%s) alg=%d addr=%p key_idx=%d " - "set_tx=%d seq_len=%lu key_len=%lu", + "set_tx=%d seq_len=%lu key_len=%lu key_flag=0x%x", __func__, ifindex, ifname, alg, addr, key_idx, set_tx, - (unsigned long) seq_len, (unsigned long) key_len); + (unsigned long) seq_len, (unsigned long) key_len, key_flag); #ifdef CONFIG_TDLS if (key_idx == -1) { key_idx = 0; - tdls = 1; } #endif /* CONFIG_TDLS */ + if (key_flag & KEY_FLAG_PAIRWISE && + key_flag & (KEY_FLAG_GROUP | KEY_FLAG_DEFAULT)) + return -EINVAL; #ifdef CONFIG_DRIVER_NL80211_QCA if (alg == WPA_ALG_PMK && @@ -3094,10 +3096,13 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, if (alg == WPA_ALG_PMK && (drv->capa.flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE_8021X)) return nl80211_set_pmk(drv, key, key_len, addr); + if (key_flag & KEY_FLAG_PMK) + return -EINVAL; + ret = -ENOBUFS; key_msg = nlmsg_alloc(); if (!key_msg) - return -ENOBUFS; + return ret; if (alg == WPA_ALG_NONE) { msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_DEL_KEY); @@ -3107,8 +3112,10 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, u32 suite; suite = wpa_alg_to_cipher_suite(alg, key_len); - if (!suite) + if (!suite) { + ret = -EINVAL; goto fail2; + } msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_NEW_KEY); if (!msg) goto fail2; @@ -3129,22 +3136,24 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, addr)) goto fail; - if (alg != WPA_ALG_WEP && key_idx && !set_tx) { + if (key_flag == KEY_FLAG_GROUP_RX) { wpa_printf(MSG_DEBUG, " RSN IBSS RX GTK"); if (nla_put_u32(key_msg, NL80211_KEY_TYPE, NL80211_KEYTYPE_GROUP)) goto fail; + } else if (key_flag & (KEY_FLAG_GROUP | KEY_FLAG_DEFAULT) || + !(key_flag & KEY_FLAG_PAIRWISE)) { + ret = -EINVAL; + goto fail; + } else { + wpa_printf(MSG_DEBUG, " pairwise key"); } - } else if (addr && is_broadcast_ether_addr(addr)) { - struct nlattr *types; - + } else if (key_flag & KEY_FLAG_PAIRWISE || + !(key_flag & KEY_FLAG_GROUP)) { + ret = -EINVAL; + goto fail; + } else { wpa_printf(MSG_DEBUG, " broadcast key"); - - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); - if (!types || - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) - goto fail; - nla_nest_end(key_msg, types); } if (nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) || nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) @@ -3167,18 +3176,18 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, ret, strerror(-ret)); /* - * If we failed or don't need to set the default TX key (below), + * If we failed or don't need to set the key as default (below), * we're done here. */ - if (ret || !set_tx || alg == WPA_ALG_NONE || tdls) - return ret; - if (is_ap_interface(drv->nlmode) && addr && - !is_broadcast_ether_addr(addr)) + if (ret || !(key_flag & KEY_FLAG_DEFAULT)) return ret; + if (!(key_flag & KEY_FLAG_GROUP)) + return -EINVAL; + ret = -ENOBUFS; key_msg = nlmsg_alloc(); if (!key_msg) - return -ENOBUFS; + return ret; msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_SET_KEY); if (!msg) @@ -3197,19 +3206,19 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss, if (addr && is_broadcast_ether_addr(addr)) { struct nlattr *types; + wpa_printf(MSG_DEBUG, " group key"); types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); if (!types || nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) goto fail; nla_nest_end(key_msg, types); } else if (addr) { - struct nlattr *types; - - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); - if (!types || - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST)) - goto fail; - nla_nest_end(key_msg, types); + wpa_printf(MSG_DEBUG, + "nl80211: Default group key can't use a unicast address"); + ret = -EINVAL; + goto fail; + } else { + wpa_printf(MSG_DEBUG, " WEP/WPA-NONE key"); } if (nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) @@ -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; if (ret) wpa_printf(MSG_DEBUG, "nl80211: set_key default failed; err=%d %s", @@ -3240,7 +3247,7 @@ fail: fail2: nl80211_nlmsg_clear(key_msg); nlmsg_free(key_msg); - return -ENOBUFS; + return ret; }
Stop using set_tx and cleanup/restructure the key install logic depending on it. The updated logic is also no longer incorrectly installing some pairwise keys as default keys and has additional sanity checks refusing unexpected keys. Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> --- src/drivers/driver_nl80211.c | 71 ++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 32 deletions(-)