| Message ID | 20190915200837.196283-3-alexander@wetzel-home.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Support seamless PTK rekeys with Extended Key ID | expand |
On Sun, Sep 15, 2019 at 10:08:22PM +0200, Alexander Wetzel wrote: > @@ -3045,26 +3046,31 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, > + key_msg = nlmsg_alloc(); ... > + if (nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) || > + nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) > goto fail; ... > + key_msg = nlmsg_alloc(); This seems to leak memory (that nla_put_nested() used key_msg, but did not free it). And also leave in key information in heap. > + if (nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) > + goto fail; > + > ret = send_and_recv_msgs(drv, msg, NULL, NULL); Same here. > +fail2: > + nl80211_nlmsg_clear(key_msg); > + nlmsg_free(key_msg); These need to be done in the success cases as well. No need to send this patch again because of this, though, since I've already addressed that in my work version.
> On Sun, Sep 15, 2019 at 10:08:22PM +0200, Alexander Wetzel wrote: >> @@ -3045,26 +3046,31 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, > >> + key_msg = nlmsg_alloc(); > ... > >> + if (nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) || >> + nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) >> goto fail; > ... > >> + key_msg = nlmsg_alloc(); > > This seems to leak memory (that nla_put_nested() used key_msg, but did > not free it). And also leave in key information in heap. Thanks for pointing that out... It kind of makes sense: I was simply assuming that nla_put_nested() would fold the information into the existing netlink message and never tough to cross check that. But when it's copying the information we have a memory leak... So I've also now added nlmsg_free(key_msg); after each call to nla_put_nested() in my tree. >> + if (nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) >> + goto fail; >> + >> ret = send_and_recv_msgs(drv, msg, NULL, NULL); > > Same here. > >> +fail2: >> + nl80211_nlmsg_clear(key_msg); >> + nlmsg_free(key_msg); > > These need to be done in the success cases as well. > > No need to send this patch again because of this, though, since I've > already addressed that in my work version. > Ok. But as always when sending a patch out I already found another issue: handle_extended_key_id() for hostapd is kind of stupid in the current patchset. By moving it some lines we can also fold the TKIP check into it. I've mostly done that - more tests pending - but then I also noticed that handle_extended_key() should be ok to check less and want to look into that, too. (Should be trivial, so I probably can do that today.) I'm also thinking about to dropping all the "& 0x03" in src/ap/wpa_auth.c. The one I added for keyidx_active should be "& 0x01" but since we do that on trusted internal variables which already have the correct values there should be no need to do that at all... Do you maybe prefer just a diff with the changes compared to V6? I could either act like V6 has been merged or even just send you one patch with the accumulated fixes. Alexander
On Thu, Sep 19, 2019 at 06:16:09PM +0200, Alexander Wetzel wrote: > But as always when sending a patch out I already found another issue: > > handle_extended_key_id() for hostapd is kind of stupid in the current > patchset. By moving it some lines we can also fold the TKIP check into it. > I've mostly done that - more tests pending - but then I also noticed that > handle_extended_key() should be ok to check less and want to look into that, > too. (Should be trivial, so I probably can do that today.) > > I'm also thinking about to dropping all the "& 0x03" in src/ap/wpa_auth.c. > The one I added for keyidx_active should be "& 0x01" but since we do that on > trusted internal variables which already have the correct values there > should be no need to do that at all... > > Do you maybe prefer just a diff with the changes compared to V6? > I could either act like V6 has been merged or even just send you one patch > with the accumulated fixes. It would probably be easiest if you can send separate patches on top of full V6 patchset for now since I'm going through those and will most likely apply at least a subset of them. I can easily merge in such changes to pending patches if they are fixing or cleaning up items within not yet fully applied changes.
Am 20.09.19 um 11:26 schrieb Jouni Malinen: > On Thu, Sep 19, 2019 at 06:16:09PM +0200, Alexander Wetzel wrote: >> But as always when sending a patch out I already found another issue: >> >> handle_extended_key_id() for hostapd is kind of stupid in the current >> patchset. By moving it some lines we can also fold the TKIP check into it. >> I've mostly done that - more tests pending - but then I also noticed that >> handle_extended_key() should be ok to check less and want to look into that, >> too. (Should be trivial, so I probably can do that today.) >> >> I'm also thinking about to dropping all the "& 0x03" in src/ap/wpa_auth.c. >> The one I added for keyidx_active should be "& 0x01" but since we do that on >> trusted internal variables which already have the correct values there >> should be no need to do that at all... >> >> Do you maybe prefer just a diff with the changes compared to V6? >> I could either act like V6 has been merged or even just send you one patch >> with the accumulated fixes. > > It would probably be easiest if you can send separate patches on top of > full V6 patchset for now since I'm going through those and will most > likely apply at least a subset of them. I can easily merge in such > changes to pending patches if they are fixing or cleaning up items > within not yet fully applied changes. Are you interested in a V8? The recent commit 865721c69 ("Merge wpa_supplicant and hostapd EAPOL-Key KDE parsers") breaks the submitted patch series v6 and v6a in some inconvenient ways. I've rebased the series and also moved the code and the patches around to deal with that. And I've incorporated the changes you are planning and I'm aware of. (I've not merged any patches together so far, though.) Additional I've moved the patch to drop "set_tx" in front of all Extended Key ID patches to have a clean split between key_type API and Extended Key ID patches and integrated the fixes from V6a to the corresponding patches. So I've now basically a V8 of the series which applies cleanly to the current HEAD with next to no "real" code changes I could send out if that's useful for you. (I just have to run the tests to see if one of the new tests needs some tweaking, too.) Alexander
On Tue, Oct 29, 2019 at 01:32:13PM +0100, Alexander Wetzel wrote: > Are you interested in a V8? > > The recent commit 865721c69 ("Merge wpa_supplicant and hostapd EAPOL-Key KDE > parsers") breaks the submitted patch series v6 and v6a in some inconvenient > ways. > > I've rebased the series and also moved the code and the patches around to > deal with that. And I've incorporated the changes you are planning and I'm > aware of. (I've not merged any patches together so far, though.) > > Additional I've moved the patch to drop "set_tx" in front of all Extended > Key ID patches to have a clean split between key_type API and Extended Key > ID patches and integrated the fixes from V6a to the corresponding patches. > > So I've now basically a V8 of the series which applies cleanly to the > current HEAD with next to no "real" code changes I could send out if that's > useful for you. (I just have to run the tests to see if one of the new tests > needs some tweaking, too.) Since you've already rebased, yes, it would probably be easiest if you sent a full new set since I have not yet found time to finish going through the previous fixes.
>> So I've now basically a V8 of the series which applies cleanly to the >> current HEAD with next to no "real" code changes I could send out if that's >> useful for you. (I just have to run the tests to see if one of the new tests >> needs some tweaking, too.) > > Since you've already rebased, yes, it would probably be easiest if you > sent a full new set since I have not yet found time to finish going > through the previous fixes. > I've again some improvements for the patch series ready but there is already more tweaking I can do. Since the patches 1-7 of he series remain unchanged I think I'll just wait on feedback for those and not post a new series for now. If you prefer more frequent updates I can of course just wrap up and release the next version instead... The improvements of the Extended Key ID patches are (so far) nothing fundamental. Here are the highlights: - handle_extended_key_id() for wpa_supplicant no longer needs the rsn_ie as an argument and uses the ap_rsn_ie instead. - The above tweak needs a separate fix which will probably also be part of the next patch set but it's basically independent. (It also should avoid an extra scan on connect in many cases.) - OSEN/HS20 Extended Key ID support preparations will be dropped. After studying OSEN it seems to be unable to handle Extended Key ID without some violation of the Extended Key ID or OSEN standards. (They really seem to have added the RSN capabilities to the wrong IE, allowing it to advertise only the AP features...) - I also have a standalone wpa_supplicant OSEN patch which needs some more testing but seems to be solid. It's basically filling in the few remaining gaps to handle OSEN elements with the existing RSN functions and checks. (With a HS20 compatible way to actually inform the AP that the client STA is able to handle Extended Key ID this would basically complete the wpa_supplicant support for it. We only would have to enable it..) Alexander
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c index c7dad201b..9a4d8153a 100644 --- a/src/drivers/driver_nl80211.c +++ b/src/drivers/driver_nl80211.c @@ -3011,7 +3011,8 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, { struct wpa_driver_nl80211_data *drv = bss->drv; int ifindex; - struct nl_msg *msg = NULL; + struct nl_msg *msg; + struct nl_msg *key_msg; int ret; int tdls = 0; @@ -3045,26 +3046,31 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, (drv->capa.flags & WPA_DRIVER_FLAGS_4WAY_HANDSHAKE_8021X)) return nl80211_set_pmk(drv, key, key_len, addr); + key_msg = nlmsg_alloc(); + if (!key_msg) + return -ENOBUFS; + if (alg == WPA_ALG_NONE) { msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_DEL_KEY); if (!msg) - return -ENOBUFS; + goto fail2; } else { u32 suite; suite = wpa_alg_to_cipher_suite(alg, key_len); if (!suite) - goto fail; + goto fail2; msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_NEW_KEY); - if (!msg || - nla_put(msg, NL80211_ATTR_KEY_DATA, key_len, key) || - nla_put_u32(msg, NL80211_ATTR_KEY_CIPHER, suite)) + if (!msg) + goto fail2; + if (nla_put(key_msg, NL80211_KEY_DATA, key_len, key) || + nla_put_u32(key_msg, NL80211_KEY_CIPHER, suite)) goto fail; wpa_hexdump_key(MSG_DEBUG, "nl80211: KEY_DATA", key, key_len); } if (seq && seq_len) { - if (nla_put(msg, NL80211_ATTR_KEY_SEQ, seq_len, seq)) + if (nla_put(key_msg, NL80211_KEY_SEQ, seq_len, seq)) goto fail; wpa_hexdump(MSG_DEBUG, "nl80211: KEY_SEQ", seq, seq_len); } @@ -3076,7 +3082,7 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, if (alg != WPA_ALG_WEP && key_idx && !set_tx) { wpa_printf(MSG_DEBUG, " RSN IBSS RX GTK"); - if (nla_put_u32(msg, NL80211_ATTR_KEY_TYPE, + if (nla_put_u32(key_msg, NL80211_KEY_TYPE, NL80211_KEYTYPE_GROUP)) goto fail; } @@ -3085,13 +3091,14 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, wpa_printf(MSG_DEBUG, " broadcast key"); - types = nla_nest_start(msg, NL80211_ATTR_KEY_DEFAULT_TYPES); + types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); if (!types || - nla_put_flag(msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) + nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) goto fail; - nla_nest_end(msg, types); + nla_nest_end(key_msg, types); } - if (nla_put_u8(msg, NL80211_ATTR_KEY_IDX, key_idx)) + if (nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) || + nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) goto fail; ret = send_and_recv_msgs(drv, msg, NULL, key ? (void *) -1 : NULL); @@ -3111,34 +3118,43 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, !is_broadcast_ether_addr(addr)) return ret; + key_msg = nlmsg_alloc(); + if (!key_msg) + return -ENOBUFS; + msg = nl80211_ifindex_msg(drv, ifindex, 0, NL80211_CMD_SET_KEY); - if (!msg || - nla_put_u8(msg, NL80211_ATTR_KEY_IDX, key_idx) || - nla_put_flag(msg, (alg == WPA_ALG_IGTK || - alg == WPA_ALG_BIP_GMAC_128 || - alg == WPA_ALG_BIP_GMAC_256 || - alg == WPA_ALG_BIP_CMAC_256) ? - NL80211_ATTR_KEY_DEFAULT_MGMT : - NL80211_ATTR_KEY_DEFAULT)) + if (!msg) + goto fail2; + if (!key_msg || + nla_put_u8(key_msg, NL80211_KEY_IDX, key_idx) || + nla_put_flag(key_msg, (alg == WPA_ALG_IGTK || + alg == WPA_ALG_BIP_GMAC_128 || + alg == WPA_ALG_BIP_GMAC_256 || + alg == WPA_ALG_BIP_CMAC_256) ? + NL80211_KEY_DEFAULT_MGMT : + NL80211_KEY_DEFAULT)) goto fail; if (addr && is_broadcast_ether_addr(addr)) { struct nlattr *types; - types = nla_nest_start(msg, NL80211_ATTR_KEY_DEFAULT_TYPES); + types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); if (!types || - nla_put_flag(msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) + nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST)) goto fail; - nla_nest_end(msg, types); + nla_nest_end(key_msg, types); } else if (addr) { struct nlattr *types; - types = nla_nest_start(msg, NL80211_ATTR_KEY_DEFAULT_TYPES); + types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES); if (!types || - nla_put_flag(msg, NL80211_KEY_DEFAULT_TYPE_UNICAST)) + nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST)) goto fail; - nla_nest_end(msg, types); + nla_nest_end(key_msg, types); } + if (nla_put_nested(msg, NL80211_ATTR_KEY, key_msg)) + goto fail; + ret = send_and_recv_msgs(drv, msg, NULL, NULL); if (ret == -ENOENT) ret = 0; @@ -3150,6 +3166,9 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, fail: nl80211_nlmsg_clear(msg); nlmsg_free(msg); +fail2: + nl80211_nlmsg_clear(key_msg); + nlmsg_free(key_msg); return -ENOBUFS; }
Linux 2.6.32 (December 2009) introduced alternate netlink messages for setting and installing keys, deprecating the older ones. To allow hostapd/wpa_supplicant to use new features only provided via the new API this patch migrates all netlink messages to the current ones. Since the nl80211 driver was sometimes already using the new format this is only unifying the netlink API usage and not changing the minimal kernel requirement. The following netlink attributes have been retired for key installs: NL80211_ATTR_KEY_DATA NL80211_ATTR_KEY_TYPE NL80211_ATTR_KEY_SEQ NL80211_ATTR_KEY_IDX NL80211_ATTR_KEY_CIPHER NL80211_ATTR_KEY_DEFAULT NL80211_ATTR_KEY_DEFAULT_MGMT NL80211_ATTR_KEY_DEFAULT_TYPES And replaced by the following attributes nested in NL80211_ATTR_KEY: NL80211_KEY_DATA NL80211_KEY_TYPE NL80211_KEY_SEQ NL80211_KEY_IDX NL80211_KEY_CIPHER NL80211_KEY_DEFAULT NL80211_KEY_DEFAULT_MGMT NL80211_KEY_DEFAULT_TYPES When getting michael mic_failures notifications or querying a key sequence number the kernel continues to use the old attributes: NL80211_ATTR_KEY_TYPE NL80211_ATTR_KEY_SEQ NL80211_ATTR_KEY_IDX Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> --- There is not much to add to the commit log, most of it is straight forward. But I tried to keep the existing logic and therefore start an additional key netlink message which is then merged into the main one prior to sending the message. This allows to fill in the information with the existing logic and avoids the restructuring of everything to be able to use nla_nest_start(). src/drivers/driver_nl80211.c | 71 +++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 26 deletions(-)