Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
On Tue, 2018-08-28 at 21:02 +0200, Alexander Wetzel wrote: > My current preference is how the patch v6 is working and I'm quite sure > there is no acceptable way to trick the userspace. > Am I wrong here and we should try something different? No, you're probably right. I'll take this as it is per your decision, and will perhaps think about it (and if needed send an RFC patch) later. johannes
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Am 28.08.18 um 18:03 schrieb Johannes Berg: > On Tue, 2018-08-28 at 18:00 +0200, Alexander Wetzel wrote: > >>> If you have a flag here, why say "userspace must not" rather than just >>> outright prevent userspace from doing it? >> >> The userspace must not but currently of course is doing exactly that. >> Enforcing the new requirement would therefore cause user visible >> regressions till all drivers have been updated or the updated userspace >> software is deployed on all systems... Both will take years. >> >> So the current approach is keep backward compatibility to not break >> rekeys for users it's currently working for. > > Yeah but is it really working for them? They might have it "working", > but leak some frames in clear, like we said? So it might work for all > they notice, but leak frames once a while? I don't see how that's better > really. The %DISABLE_KEY and %SET_KEY commands are now much closer together and mac80211 is stopping handing over frames needing the key till %SET_KEY has been completed and we call flush() when the driver has implemented it. So to still leak clear text packets a driver must have botched up %DISABLE_KEY and not offering flush() to have a chance to still leak clear text and even then should only leak a fraction of the packets it would leak unpatched. Of course that's still far from ideal and I the secure decision would be to accept the user visible impact (e.g. prompting for the PSK again or needing ~30s for the wlan to reconnect). But then my understanding was, that the kernel never must break the userspace and we don't have that option. >From a high level point it looks like we only have these choices from where we are today 1) Accept rekeys even when the driver is not signaling support for it, allowing the drivers and the userspace to catch up to API changes. (And then start enforcing it when it looks safe to do so.) 2) Deny rekey, fundamentally changing how the issue looks for users currently using rekeys. (Which may be none or millions, no data available. But there are some EAP Enterprise systems from Cisco having that enabled by default.) 3) Do not fix anything - not even the parts we can - and continue to wait for "Extended Key ID for Individually Addressed Frames" to catch on. 4) Implement complex code able to trick different userspaces into the desired behavior (and probably failing for unknown ones). This is basically resuming the discussion we had with the v2 version of the patch, so here the relevant part of our older discussion: So I think we're probably better off accepting the set_key but not actually using it, and instead disconnecting... even if that's awkward and should come with a big comment :-) >>> >>> Instead of returning an error I'll change the code to accept the rekey >>> but do nothing with it. (Basically delete the new key and keep the old >>> active). >>> And of course calling ieee80211_set_disassoc() prior to return "success". >> >> Right. Did you handle/consider modes other than BSS/P2P client btw? > > I've tested that on a client only and it's already not working. Problem > is, that with ieee80211_set_disassoc() we just dump the association in > the kernel and are not informing the userspace, so the state machine is > stuck in "connected" but the STA is unable to communicate. > > I also tried ieee80211_mgd_deauth(): > While better this is basically the same behaviour as returning an error > on key replace. By setting the error code to inactivity at least > wpa_supplicant was actually starting to reconnect, unfortunatelly > set_key then failes since we purged the assosication in the kernel. (Or > that's my best interpretation from the logs). Networkmanager then again > prompted for the password... > > I started experimenting with just signals to the userspace, but then > paused... Trying to control the state machine with spoofed errors trying > to trigger an "desireable" action can't be the way forward, can it? > > Even if we get that working changes or different implementations in > userspace may well break it. > > I now think we only have two reasonable ways forward: > > 1) The user friendly one: If the userspace does not know about the new > API just print a error message and do the insecure key replace. With > potential clear text packet leaks and connection freezes. > > 2) The secure way: Report an error on key install and accept that users > will encounter new problems when they are using rekeys with the old API > with "old" userspace software. > > Since we have this issue in the kernel at least as long as we have > mac80211 I would vote for 1) here. Fix mac80211 and add a new way for > the userspace to to securely replace the keys and detect when this is > not possible. Then get the userspace software updated to act > accordingly, finally preventing the clear text packet leaks. > > After some time we can then decide to reject insecure rekeys, burning > only those who use kernels
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Am 28.08.18 um 10:47 schrieb Johannes Berg:
> On Tue, 2018-08-14 at 12:42 +0200, Alexander Wetzel wrote:
>> Drivers able to correctly replace a in-use key should set
>> NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE to allow the userspace (e.g.
>> hostapd or wpa_supplicant) to rekey PTK keys.
>>
>> The userspace must detect a PTK rekey attempt and only go ahead with the
>> rekey when the driver has set this flag. If the driver is not supporting
>> the feature the userspace either must not replace the PTK key or perform
>> a full re-association.
>>
>> Ignoring this flag and continuing to rekey the connection can still
>> work but has to be considered insecure and broken. It can leak cleartext
>> packets or freeze the connection and is only supported to allow the
>> userspace to be updated.
>>
>> Signed-off-by: Alexander Wetzel
>> ---
>> include/uapi/linux/nl80211.h | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
>> index 7acc16f34942..b41b9ade0449 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -5224,6 +5224,11 @@ enum nl80211_feature_flags {
>> * except for supported rates from the probe request content if requested
>> * by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
>> *
>> + * @NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE: Driver/device confirm that they
>> are
>> + * able to rekey an in-use key correctly. Userspace must not rekey PTK
>> keys
>> + * if this flag is not set. Ignoring this can leak clear text packets
>> and/or
>> + * freeze the connection.
>
>
> If you have a flag here, why say "userspace must not" rather than just
> outright prevent userspace from doing it?
The userspace must not but currently of course is doing exactly that.
Enforcing the new requirement would therefore cause user visible
regressions till all drivers have been updated or the updated userspace
software is deployed on all systems... Both will take years.
So the current approach is keep backward compatibility to not break
rekeys for users it's currently working for.
Alexander
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
On Tue, 2018-08-28 at 18:00 +0200, Alexander Wetzel wrote: > > If you have a flag here, why say "userspace must not" rather than just > > outright prevent userspace from doing it? > > The userspace must not but currently of course is doing exactly that. > Enforcing the new requirement would therefore cause user visible > regressions till all drivers have been updated or the updated userspace > software is deployed on all systems... Both will take years. > > So the current approach is keep backward compatibility to not break > rekeys for users it's currently working for. Yeah but is it really working for them? They might have it "working", but leak some frames in clear, like we said? So it might work for all they notice, but leak frames once a while? I don't see how that's better really. johannes
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Am 28.08.18 um 10:46 schrieb Johannes Berg: > On Sat, 2018-08-18 at 22:53 +0200, Alexander Wetzel wrote: > >>> This looks good to me from a userspace perspective. I will try to >>> implement support for this in iwd soon to give you a prototype to play >>> with. >> >> Sounds promising, thank you! >> >> I'm still unsure if we really need the API changes to fix that issue: >> "Tagging" the new requirements to current set_key calls would also work. >> With the downside that there would be no way to detect "broken" >> drivers... replace_key is basically only there to differentiate between >> audited/fixed drivers and those not. >> >> But since my current impression is, that ptk rekeys are mostly broken >> independent of mac80211 or even linux a driver flag signaling support >> for it sounds like a good idea regardless how we want to fix the issue >> in mac80211. Just wondering if we should name it differently for that >> and I'm considering renaming it to NL80211_EXT_FEATURE_CAN_REKEY_PTK0 in >> the next patch. > > And then keep set_key() for both, rather than adding replace_key()? > Seems reasonable to me, I guess. Exactly. The complete replace_key patch will be dropped. I've the patches for that nearly ready, only working on the commit messages and docu updates. (To code changes were trivial.) > >> As for mac80211 driver status: >> The only known "really broken" driver at the moment is ath9k. With >> iwlwifi, - and less thorough tested - ath10k to be ok from a driver >> point of view. (ath9k needs just a driver flush as minimal fix.) > > iwlwifi is also broken for CCMP-256/GCMP keys, so the situation is > slightly more complex. I was suspecting something like that, thanks for confirming. Alexander
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
On Tue, 2018-08-14 at 12:42 +0200, Alexander Wetzel wrote:
> Drivers able to correctly replace a in-use key should set
> NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE to allow the userspace (e.g.
> hostapd or wpa_supplicant) to rekey PTK keys.
>
> The userspace must detect a PTK rekey attempt and only go ahead with the
> rekey when the driver has set this flag. If the driver is not supporting
> the feature the userspace either must not replace the PTK key or perform
> a full re-association.
>
> Ignoring this flag and continuing to rekey the connection can still
> work but has to be considered insecure and broken. It can leak cleartext
> packets or freeze the connection and is only supported to allow the
> userspace to be updated.
>
> Signed-off-by: Alexander Wetzel
> ---
> include/uapi/linux/nl80211.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 7acc16f34942..b41b9ade0449 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -5224,6 +5224,11 @@ enum nl80211_feature_flags {
> * except for supported rates from the probe request content if requested
> * by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
> *
> + * @NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE: Driver/device confirm that they
> are
> + * able to rekey an in-use key correctly. Userspace must not rekey PTK
> keys
> + * if this flag is not set. Ignoring this can leak clear text packets
> and/or
> + * freeze the connection.
If you have a flag here, why say "userspace must not" rather than just
outright prevent userspace from doing it?
johannes
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
On Sat, 2018-08-18 at 22:53 +0200, Alexander Wetzel wrote: > > This looks good to me from a userspace perspective. I will try to > > implement support for this in iwd soon to give you a prototype to play > > with. > > Sounds promising, thank you! > > I'm still unsure if we really need the API changes to fix that issue: > "Tagging" the new requirements to current set_key calls would also work. > With the downside that there would be no way to detect "broken" > drivers... replace_key is basically only there to differentiate between > audited/fixed drivers and those not. > > But since my current impression is, that ptk rekeys are mostly broken > independent of mac80211 or even linux a driver flag signaling support > for it sounds like a good idea regardless how we want to fix the issue > in mac80211. Just wondering if we should name it differently for that > and I'm considering renaming it to NL80211_EXT_FEATURE_CAN_REKEY_PTK0 in > the next patch. And then keep set_key() for both, rather than adding replace_key()? Seems reasonable to me, I guess. > As for mac80211 driver status: > The only known "really broken" driver at the moment is ath9k. With > iwlwifi, - and less thorough tested - ath10k to be ok from a driver > point of view. (ath9k needs just a driver flush as minimal fix.) iwlwifi is also broken for CCMP-256/GCMP keys, so the situation is slightly more complex. johannes
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Hi Denis Am 16.08.18 um 18:30 schrieb Denis Kenzior: > Hi Alexander, > > On 08/14/2018 05:42 AM, Alexander Wetzel wrote: >> Drivers able to correctly replace a in-use key should set >> NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE to allow the userspace (e.g. >> hostapd or wpa_supplicant) to rekey PTK keys. >> >> The userspace must detect a PTK rekey attempt and only go ahead with the >> rekey when the driver has set this flag. If the driver is not supporting >> the feature the userspace either must not replace the PTK key or perform >> a full re-association. >> >> Ignoring this flag and continuing to rekey the connection can still >> work but has to be considered insecure and broken. It can leak cleartext >> packets or freeze the connection and is only supported to allow the >> userspace to be updated. >> >> Signed-off-by: Alexander Wetzel >> --- >> include/uapi/linux/nl80211.h | 6 ++ >> 1 file changed, 6 insertions(+) >> > > This looks good to me from a userspace perspective. I will try to > implement support for this in iwd soon to give you a prototype to play > with. Sounds promising, thank you! I'm still unsure if we really need the API changes to fix that issue: "Tagging" the new requirements to current set_key calls would also work. With the downside that there would be no way to detect "broken" drivers... replace_key is basically only there to differentiate between audited/fixed drivers and those not. But since my current impression is, that ptk rekeys are mostly broken independent of mac80211 or even linux a driver flag signaling support for it sounds like a good idea regardless how we want to fix the issue in mac80211. Just wondering if we should name it differently for that and I'm considering renaming it to NL80211_EXT_FEATURE_CAN_REKEY_PTK0 in the next patch. As for mac80211 driver status: The only known "really broken" driver at the moment is ath9k. With iwlwifi, - and less thorough tested - ath10k to be ok from a driver point of view. (ath9k needs just a driver flush as minimal fix.) rt2800usb is also working fine with this patch series, but I have not looked into the driver to figure out if this is due to the additional flush or not. > Reviewed-by: Denis Kenzior Again thanks. I've added that to my git tree and it will be in next patch version. I'll just wait some days for more feedback to hopefully accumulate more changes in the next series. Alexander
Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Hi Alexander, On 08/14/2018 05:42 AM, Alexander Wetzel wrote: Drivers able to correctly replace a in-use key should set NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE to allow the userspace (e.g. hostapd or wpa_supplicant) to rekey PTK keys. The userspace must detect a PTK rekey attempt and only go ahead with the rekey when the driver has set this flag. If the driver is not supporting the feature the userspace either must not replace the PTK key or perform a full re-association. Ignoring this flag and continuing to rekey the connection can still work but has to be considered insecure and broken. It can leak cleartext packets or freeze the connection and is only supported to allow the userspace to be updated. Signed-off-by: Alexander Wetzel --- include/uapi/linux/nl80211.h | 6 ++ 1 file changed, 6 insertions(+) This looks good to me from a userspace perspective. I will try to implement support for this in iwd soon to give you a prototype to play with. Reviewed-by: Denis Kenzior Regards, -Denis
[PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API
Drivers able to correctly replace a in-use key should set
NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE to allow the userspace (e.g.
hostapd or wpa_supplicant) to rekey PTK keys.
The userspace must detect a PTK rekey attempt and only go ahead with the
rekey when the driver has set this flag. If the driver is not supporting
the feature the userspace either must not replace the PTK key or perform
a full re-association.
Ignoring this flag and continuing to rekey the connection can still
work but has to be considered insecure and broken. It can leak cleartext
packets or freeze the connection and is only supported to allow the
userspace to be updated.
Signed-off-by: Alexander Wetzel
---
include/uapi/linux/nl80211.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 7acc16f34942..b41b9ade0449 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -5224,6 +5224,11 @@ enum nl80211_feature_flags {
* except for supported rates from the probe request content if requested
* by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
*
+ * @NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE: Driver/device confirm that they are
+ * able to rekey an in-use key correctly. Userspace must not rekey PTK
keys
+ * if this flag is not set. Ignoring this can leak clear text packets
and/or
+ * freeze the connection.
+ *
* @NUM_NL80211_EXT_FEATURES: number of extended features.
* @MAX_NL80211_EXT_FEATURES: highest extended feature index.
*/
@@ -5259,6 +5264,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_TXQS,
NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
+ NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
--
2.18.0
