[PATCH v6 0/3] Fix PTK rekey freezes and cleartext leaks

[PATCH v6 0/3] Fix PTK rekey freezes and cleartext leaks

[Alexander Wetzel]

This patch series addresses a long standing issue how mac80211 handles
PTK rekeys. The current implementation can only work with SW crypto or
by chance, e.g. if there are no frames transmitted while the STAs are
rekeying or you hit just the right combination of cards/drivers.

Any ongoing transmission while rekeying will very likely freeze the
connection till the connection is rekeyed again or the user manually
reconnects. Without any indication why, even in a kernel trace.

The multiple ways how this can go wrong are outlined in the commit
message from the last patch in this series, but here a short overview:

The main culprit for that is encryption offloading to the card while
handling the PN (IV) in mac80211 without any synchronization in between.
This allows the replay detection code to account packets intended for
the old key against the new one, which sets the PN to a value which was
correct for the old key but will drop all packets send with a PN
belonging to the new key.
The solution is of course to make sure packets prepared for the old key
are never checked against the PN (IV) of the new key, thus preventing
the invalid carry over of the old PN value to the new key.

The issue is complicated by the fact that at last some drivers do not
expect to be asked to replace a key which may be actively in use for
transmitting packets. Ath9k is e.g. simply removing the key and then
sends out the queued packets in clear till the new key is installed.

As a conclusion we therefore have to assume that all drivers which do
not actively tell us that they can handle replacing an in-use key must
not be asked to do so. Unfortunately the rekey decision is solely the
responsibility of the userspace and when the kernel refuses to replace
a key those programs are suddenly exposed to an new error condition.
At least wpa_supplicant currently reacts badly to that and assumes the
PSK is wrong instead of simply reconnecting when trying that.

We also do not have an established way to inform the userspace that the
rekey operation is not supported and it must not use it.

As a way forward this patch series makes the needed changes to correctly
rekey connections and allowing the userspace to check if PTK rekeys can
be used at all. While enforcing this restriction would probably be ok
there are some constellations where it can work. So instead of reporting
an error back to the userspace we now only print out a warning and fall
back to a best-we-can-do approach to maintain backward compatibility.

Downside here is, that till the userspace catches up - or all drivers
are supporting the new API for in-use key replaces - users may still
suffer connection freezes and leak cleartext frames. But only a fraction
of what it would be without this patch.

It's also worth mentioning that most of the pitfalls reking a PTK key
has could have been avoided if the first IEEE 802.11 standards would
already have had the option introduced in the 2012 version, named
"Extended Key ID for Individually Addressed Frames". This basically
drills down to using key ID 0 and 1 for PTK keys (and shift GTKs to 2
and 3), allowing to rollover PTK keys the same way it has been
established for GTK keys.
Supporting this addition will be the ultimate solution for the issues,
but since it only can be used if both sides are supporting it we still
have to handle PTK keys only using the key ID 0.

Here a quick overview of the patches in the series:
1) nl80211: Add ATOMIC_KEY_REPLACE API
   This adds support for NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE
   to nl80211. We expect the userspace (hostap, wpa_supplicant, iwd ...)
   to check this flag and only rekey a connection when this flag is
   present. When the flag is not set a rekey has to be "emulated" by a
   full de-association and a reconnect if it can't be avoided by the
   userspace.

2) mac80211: Define new driver callback replace_key
   This is just adding the new driver callback replace_key in
   mac80211 the last patch of the set can then use when needed.
   It's needed to provide a clear demarcation line between old
   packets which absolutely must not be send out with new key and new
   ones which should use the new key.
   By giving the driver the option to directly replace a running key
   with a updated one many drivers should be able to implement an atomic
   switch which is either using the old or the new key, avoiding the
   headach how to prevent some packets to be send out unencrypted.

3) mac80211: Fix PTK rekey freezes and cleartext leaks
   This finally changes how mac80211 handles the rekey and starts using
   the replace_key and sets NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE for
   drivers using mac80211 for it. GTK rekey is basically unchanged and
   only PTK rekey fixed to allow replacing the key with it while receiving
   and sending packets with HW encryption offloading and the races this
   causes to be factored in.
   When the driver is not supporting replace_key the code still fix the
   cleartext packet leak in mac80211 and stops RX and TX aggregation to
   prevent those to freeze the connection but besides that falls back to
   the old - know to be broken - sequence for backward compatibility. In
   that case mac80211 will print our a error message, alerting users
   that they still could leak clear text packets and may experience
   connection freezes till they either disable rekey or upgrade the
   userspace. (There is currently no updated userspace available.)

Version history:
V6 Fix PTK rekey freezes and cleartext leaks
    - typo fix in comment (beeing -> being)

V5 Fix PTK rekey freezes and cleartext leaks
    - rewritten most of the cover letter to give a better overview
    - Make "HW installs key prior to mac80211" the default for all
      key installs. (Cleaner, better understandable code.)
    - best-we-can-do approach for drivers not implementing replace_key
      which should work for many drivers.

V4 Fix PTK rekey freezes and cleartext leaks
    - Switched over to a small patch series.
    - Allows insecure rekeys again for compatibility
    - Allows the userspace to check if rekeys are safe by extending
      nl80211.

V3 mac80211: Fix PTK rekey freezes and cleartext leaks
    Updates the mac80211 API. When the driver is implementing the new
    callback replace_key mac80211 allows PTK rekeys. If not it returns
    an error on key install to the requester.

V2 mac80211: Fix wlan freezes under load at rekey
    This fixes the issue in mac80211 only without API changes on a
    best-can-do approach. Drivers still can freeze the connection and if
    so have to be fixed.

V1 mac80211: Fix wlan freezes under load at rekey
    Very simple approach, only fixing the freezes and using a not
    guaranteed to be working fallback to SW encryption.

Alexander Wetzel (3):
  nl80211: Add ATOMIC_KEY_REPLACE API
  mac80211: Define new driver callback replace_key
  mac80211: Fix PTK rekey freezes and cleartext leaks

 include/net/mac80211.h       |  15 ++++
 include/uapi/linux/nl80211.h |   6 ++
 net/mac80211/driver-ops.h    |  20 ++++++
 net/mac80211/key.c           | 131 +++++++++++++++++++++++++++++------
 net/mac80211/main.c          |   5 ++
 net/mac80211/trace.h         |  39 +++++++++++
 net/mac80211/tx.c            |   4 ++
 7 files changed, 200 insertions(+), 20 deletions(-)

-- 
2.18.0

[PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[Alexander Wetzel]

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 <alexander@wetzel-home.de>
---
 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

[PATCH v6 2/3] mac80211: Define new driver callback replace_key

[Alexander Wetzel]

Define the new driver callback replace_key in mac80211 for future use.
Drivers able to replace a in-use key should implement this new callback
to allow mac80211 drivers to securely use PTK rekeying.

On return of the function drivers must guarantee they:
 - Did not send out any frames for the key unencrypted during the
   replace,
 - will not send out packets queued to them prior to the call encrypted
   with the new key
 - and will no longer hand over any frames which were decrypted by the
   old key to mac80211 when not also handling PN (IV) in the driver.

Packets handed over to the driver after the callback has returned are
expected to be send out encrypted with the new key and retransmissions
must either be dropped or continue to use the old key.

Mac80211 will not hand over packets for the key being replaced while the
callback is running. All other packets will still be handed over.
If the driver can't handle that the driver is allowed to call functions
like ieee80211_stop_queues from the callback.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 include/net/mac80211.h    | 15 +++++++++++++++
 net/mac80211/driver-ops.h | 20 ++++++++++++++++++++
 net/mac80211/main.c       |  5 +++++
 net/mac80211/trace.h      | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5790f55c241d..7d13cd10b7d7 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3137,6 +3137,17 @@ enum ieee80211_reconfig_type {
  *	Returns a negative error code if the key can't be added.
  *	The callback can sleep.
  *
+ * @replace_key: Replace an exiting in use key with a new one while guaranteeing
+ * 	to not leak clear text packets. Implementing this callback will enable
+ * 	mac80211 to announce NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE.
+ * 	Packets already queued must not be send out encrypted with the new key
+ * 	and packets decoded with the old key must not be handed over to mac80211
+ * 	when the driver is not checking IV/ICV itself once the callback has been
+ * 	completed.
+ * 	Mac80211 will log an error when asked to use replace a PTK key
+ * 	without replace_key but will still perform the then potentially
+ * 	insecure action via set_key for backward compatibility for now.
+ *
  * @update_tkip_key: See the section "Hardware crypto acceleration"
  * 	This callback will be called in the context of Rx. Called for drivers
  * 	which set IEEE80211_KEY_FLAG_TKIP_REQ_RX_P1_KEY.
@@ -3585,6 +3596,10 @@ struct ieee80211_ops {
 	int (*set_key)(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 		       struct ieee80211_vif *vif, struct ieee80211_sta *sta,
 		       struct ieee80211_key_conf *key);
+	int (*replace_key)(struct ieee80211_hw *hw,
+		       struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+		       struct ieee80211_key_conf *old,
+		       struct ieee80211_key_conf *new);
 	void (*update_tkip_key)(struct ieee80211_hw *hw,
 				struct ieee80211_vif *vif,
 				struct ieee80211_key_conf *conf,
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 8f6998091d26..ebd7f1463336 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -255,6 +255,26 @@ static inline int drv_set_key(struct ieee80211_local *local,
 	return ret;
 }
 
+static inline int drv_replace_key(struct ieee80211_local *local,
+			      struct ieee80211_sub_if_data *sdata,
+			      struct ieee80211_sta *sta,
+			      struct ieee80211_key_conf *old_key,
+			      struct ieee80211_key_conf *new_key)
+{
+	int ret;
+
+	might_sleep();
+
+	sdata = get_bss_sdata(sdata);
+	if (!check_sdata_in_driver(sdata))
+		return -EIO;
+
+	trace_drv_replace_key(local, sdata, sta, old_key, new_key);
+	ret = local->ops->replace_key(&local->hw, &sdata->vif, sta, old_key, new_key);
+	trace_drv_return_int(local, ret);
+	return ret;
+}
+
 static inline void drv_update_tkip_key(struct ieee80211_local *local,
 				       struct ieee80211_sub_if_data *sdata,
 				       struct ieee80211_key_conf *conf,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 4fb2709cb527..84cc8005c19a 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -572,9 +572,14 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 				      NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT);
 	}
 
+	if (ops->replace_key)
+		wiphy_ext_feature_set(wiphy,
+				      NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE);
+
 	if (!ops->set_key)
 		wiphy->flags |= WIPHY_FLAG_IBSS_RSN;
 
+
 	if (ops->wake_tx_queue)
 		wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_TXQS);
 
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 0ab69a1964f8..f93e00f1ae4d 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -603,6 +603,45 @@ TRACE_EVENT(drv_set_key,
 	)
 );
 
+TRACE_EVENT(drv_replace_key,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_sub_if_data *sdata,
+		 struct ieee80211_sta *sta,
+		 struct ieee80211_key_conf *old_key,
+		 struct ieee80211_key_conf *new_key),
+
+	TP_ARGS(local, sdata, sta, old_key, new_key),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		VIF_ENTRY
+		STA_ENTRY
+		KEY_ENTRY
+		__field(u32, cipher2)
+		__field(u8, hw_key_idx2)
+		__field(u8, flags2)
+		__field(s8, keyidx2)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		VIF_ASSIGN;
+		STA_ASSIGN;
+		KEY_ASSIGN(old_key);
+		__entry->cipher2 = new_key->cipher;
+		__entry->flags2 = new_key->flags;
+		__entry->keyidx2 = new_key->keyidx;
+		__entry->hw_key_idx2 = new_key->hw_key_idx;
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT  VIF_PR_FMT  STA_PR_FMT KEY_PR_FMT
+		" cipher2:0x%x, flags2=%#x, keyidx2=%d, hw_key_idx2=%d",
+		LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, KEY_PR_ARG,
+		__entry->cipher2, __entry->flags2, __entry->keyidx2, __entry->hw_key_idx2
+	)
+);
+
 TRACE_EVENT(drv_update_tkip_key,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata,
-- 
2.18.0

[PATCH v6 3/3] mac80211: Fix PTK rekey freezes and cleartext leaks

[Alexander Wetzel]

Rekeying PTK keys without "Extended Key ID for Individually Addressed
Frames" did use a procedure not suitable to replace in-use keys and
could caused the following issues:

 1) Freeze caused by incoming packets:
    If the local STA installed the key prior to the remote STA we still
    had the old key active in the hardware with mac80211 already
    operating on the new key.
    Therefore there was a window where the card could still hand over
    packets decoded with the old key to mac80211, bumping the new PN (IV)
    value to an incorrect high number and tricking the local replay
    detection to drop all packets sent with the new key.

 2) Freeze caused by outgoing packets:
    If mac80211 was providing the PN (IV) and handed over a cleartext
    packets for encryption to the hardware prior to a key change the
    driver/card could have processed the queued packets after switching
    to the new key. This immediately bumped the PN value on the remote
    STA to an incorrect high number, which then discarded all packets
    using correct PNs we sent after that.

 3) Freeze caused by RX aggregation reorder buffer:
    An aggregation session started with the old key and ending after the
    switch to the new key also bumped the PN to an incorrect high number,
    freezing the connection quite similar to 1).

 4) Freeze caused by repeating lost frames in an aggregation session:
    A driver could repeat a lost frame and encrypt it with the new key
    while in a TX aggregation session without updating the PN for the
    new key. This also could freeze connections similar to 2).

 5) Clear text leak:
    Removing encryption offload from the card cleared the encryption
    offload flag only after the card had removed the key. The
    driver/card could therefore get unencrypted packets from mac80211 while
    no longer be instructed to encrypt them.

To prevent those issues the key install logic has been overhauled.
We now stop queuing packets to the driver while replacing the key,
replace the key first in the HW and stop/block new aggregation sessions
during the rekey.

This will only work correctly with drivers implementing replace_key or
with a userspace honoring NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE.

If the driver is not implementing replace_key all three issues can still
occur and the userspace must not try to rekey the PTK key. If asked to
do that anyhow mac80211 will fall back to an best-we-can-do approach
which should avoid the issues for at least some drivers and inform the
user about the potential issues via a kernel message.

The core of the fix changes the key install procedure from:
 - atomic switch over to the new key in mac80211
 - remove the old key in the HW (stops encryption offloading, fall back
   to SW encryption with a potential clear text packet leak in between)
 - delete the inactive old key in mac80211
 - enable HW for encryption offloading (ending software encryption)
to:
 - if it's a PTK mark the old key as tainted to drop TX packets with the
   outgoing key
 - replace the key in HW with the new one, using the new driver callback
   "replace_key" if it's availabe or by simulating it with existing
   calls if not
 - atomic switch over to the new key in mac80211 (allow TX with the key
   again)
 - delete the inactive old key in mac80211

The logic behind the updated key install sequence:
With the new sequence the HW will be unable to decode packets encrypted
with the old key prior to switching to the new key in mac80211.
Locking down TX during the rekey makes sure that there are no outgoing
packets while the driver and card are switching to the new key.

The driver is allowed to hand over packets decrypted with either the new
or the old key till "replace_key" returns. But all packets queued prior
to calling the callback must be either still be send out encrypted with
the old key or be dropped.

Even with that a RX aggregation session started prior to the rekey and
completed after can still dump frames received with the old key at
mac80211 after it switched over to the new key. This is side stepped by
stopping all aggregation sessions when we replace a PTK key and are
using key offloading. (The only alternative seems be to discard all
frames of the aggregation session running during rekey. Due to
retransmits and reorder we can't differenciate between packets send with
the old or the new key without deploying extreme efforts.)

Stopping TX aggregation sessions - instead of only RX - also avoides the
need to get the PNs (IVs) updated in frames prepared for the old key
and retransmitted after the switch to the new key. And it improves the
combatibility when the remote STA is not handling rekeys as it should.

When using SW crypto aggregation sessions are not stopped. Mac80211
won't be able to decode the dangerous packets and discard them. At least
as long as the remote STA is not also messing up and indeed updating the
PN.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 net/mac80211/key.c | 131 ++++++++++++++++++++++++++++++++++++++-------
 net/mac80211/tx.c  |   4 ++
 2 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index c054ac85793c..c77f32fe0473 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -248,6 +248,7 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
 	      (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
 		increment_tailroom_need_count(sdata);
 
+	key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
 	ret = drv_set_key(key->local, DISABLE_KEY, sdata,
 			  sta ? &sta->sta : NULL, &key->conf);
 
@@ -256,8 +257,86 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
 			  "failed to remove key (%d, %pM) from hardware (%d)\n",
 			  key->conf.keyidx,
 			  sta ? sta->sta.addr : bcast_addr, ret);
+}
 
-	key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
+static int ieee80211_hw_key_replace(struct ieee80211_key *old_key,
+				    struct ieee80211_key *new_key,
+				    bool ptk0rekey)
+{
+	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_local *local;
+	struct sta_info *sta;
+	int ret;
+
+	/* Aggregation sessions are also ok when running on SW crypto.
+	 * A broken remote STA may have issues not observed with HW
+	 * crypto, though. Nevertheless bypass the complete procedure.
+	 */
+	if (!(old_key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
+		return 0;
+
+	assert_key_lock(old_key->local);
+	sta = old_key->sta;
+
+	/* PTK only using key ID 0 needs special handling on rekey */
+	if (new_key && sta && ptk0rekey) {
+		local = old_key->local;
+		sdata = old_key->sdata;
+
+		/* Stop TX till we are on the new key */
+		old_key->flags |= KEY_FLAG_TAINTED;
+		ieee80211_clear_fast_xmit(sta);
+
+		/* Aggregation sessions during rekey are complicated due to
+		 * the reorder buffer. Side step that by blocking aggregation
+		 * and tear down running connections.
+		 */
+		if (ieee80211_hw_check(&local->hw, AMPDU_AGGREGATION)) {
+			set_sta_flag(sta, WLAN_STA_BLOCK_BA);
+			ieee80211_sta_tear_down_BA_sessions(sta,
+							    AGG_STOP_LOCAL_REQUEST);
+		}
+
+		if (new_key->local->ops->replace_key) {
+			ret = drv_replace_key(old_key->local, sdata,
+					      &sta->sta, &old_key->conf,
+					      &new_key->conf);
+			if (!ret)
+				new_key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+			else
+				sdata_err(sdata,
+					  "failed to replace key (%d) for " \
+					  "STA (%pM) in hardware: ret=(%d)\n",
+					  old_key->conf.keyidx,
+					  sta->sta.addr,
+					  ret);
+
+			old_key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
+		} else {
+			sdata_info(sdata,
+				   "Userspace requested a PTK rekey for STA " \
+				   "%pM while feature not supported! " \
+				   "This may leak clear text packets or " \
+				   "freeze the connection.",
+				   sta->sta.addr);
+			/* Flushing the driver queues *may* prevent cleartext
+			 * leaks and freezes if supported by the driver.
+			 */
+			ieee80211_flush_queues(old_key->local,
+					       old_key->sdata,
+					       false);
+			ieee80211_key_disable_hw_accel(old_key);
+			ret = ieee80211_key_enable_hw_accel(new_key);
+		}
+	} else {
+		ieee80211_key_disable_hw_accel(old_key);
+		if (new_key)
+			ret = ieee80211_key_enable_hw_accel(new_key);
+		else
+			ret = 0;
+	}
+
+	return ret;
 }
 
 static void __ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata,
@@ -316,38 +395,56 @@ void ieee80211_set_default_mgmt_key(struct ieee80211_sub_if_data *sdata,
 }
 
 
-static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
+static int ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 				  struct sta_info *sta,
 				  bool pairwise,
 				  struct ieee80211_key *old,
 				  struct ieee80211_key *new)
 {
 	int idx;
+	int ret;
 	bool defunikey, defmultikey, defmgmtkey;
 
 	/* caller must provide at least one old/new */
 	if (WARN_ON(!new && !old))
-		return;
+		return 0;
 
 	if (new)
 		list_add_tail_rcu(&new->list, &sdata->key_list);
 
 	WARN_ON(new && old && new->conf.keyidx != old->conf.keyidx);
 
-	if (old)
+	if (old) {
 		idx = old->conf.keyidx;
-	else
+		/* TODO: proper implement and test "Extended Key ID for
+		 * Individually Addressed Frames" from IEEE 802.11-2016.
+		 * Till then always assume only key ID 0 is used for
+		 * pairwise keys.*/
+		ret = ieee80211_hw_key_replace(old, new, pairwise);
+	} else {
 		idx = new->conf.keyidx;
+		if (new && !new->local->wowlan)
+			ret = ieee80211_key_enable_hw_accel(new);
+		else
+			ret = 0;
+	}
+
+	if (ret)
+		return ret;
 
 	if (sta) {
 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
 			sta->ptk_idx = idx;
-			ieee80211_check_fast_xmit(sta);
+			if (new) {
+				clear_sta_flag(sta, WLAN_STA_BLOCK_BA);
+				ieee80211_check_fast_xmit(sta);
+			}
 		} else {
 			rcu_assign_pointer(sta->gtk[idx], new);
 		}
-		ieee80211_check_fast_rx(sta);
+		if (new)
+			ieee80211_check_fast_rx(sta);
 	} else {
 		defunikey = old &&
 			old == key_mtx_dereference(sdata->local,
@@ -380,6 +477,8 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 
 	if (old)
 		list_del_rcu(&old->list);
+
+	return 0;
 }
 
 struct ieee80211_key *
@@ -575,9 +674,6 @@ static void ieee80211_key_free_common(struct ieee80211_key *key)
 static void __ieee80211_key_destroy(struct ieee80211_key *key,
 				    bool delay_tailroom)
 {
-	if (key->local)
-		ieee80211_key_disable_hw_accel(key);
-
 	if (key->local) {
 		struct ieee80211_sub_if_data *sdata = key->sdata;
 
@@ -654,7 +750,6 @@ int ieee80211_key_link(struct ieee80211_key *key,
 		       struct ieee80211_sub_if_data *sdata,
 		       struct sta_info *sta)
 {
-	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_key *old_key;
 	int idx = key->conf.keyidx;
 	bool pairwise = key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE;
@@ -691,17 +786,13 @@ int ieee80211_key_link(struct ieee80211_key *key,
 
 	increment_tailroom_need_count(sdata);
 
-	ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
-	ieee80211_key_destroy(old_key, delay_tailroom);
-
-	ieee80211_debugfs_key_add(key);
+	ret = ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
 
-	if (!local->wowlan) {
-		ret = ieee80211_key_enable_hw_accel(key);
-		if (ret)
-			ieee80211_key_free(key, delay_tailroom);
+	if (!ret) {
+		ieee80211_debugfs_key_add(key);
+		ieee80211_key_destroy(old_key, delay_tailroom);
 	} else {
-		ret = 0;
+		ieee80211_key_free(key, delay_tailroom);
 	}
 
  out:
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index cd332e3e1134..5c79b3c2a7e1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2951,6 +2951,10 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
 		if (!(build.key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
 			goto out;
 
+		/* Key is being removed */
+		if (build.key->flags & KEY_FLAG_TAINTED)
+			goto out;
+
 		switch (build.key->conf.cipher) {
 		case WLAN_CIPHER_SUITE_CCMP:
 		case WLAN_CIPHER_SUITE_CCMP_256:
-- 
2.18.0

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[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 <alexander@wetzel-home.de>
> ---
>   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 <denkenz@gmail.com>

Regards,
-Denis

Re: [PATCH v6 2/3] mac80211: Define new driver callback replace_key

[Denis Kenzior]

Hi Alexander,

Just minor nitpicks:

> + * @replace_key: Replace an exiting in use key with a new one while guaranteeing
> + * 	to not leak clear text packets. Implementing this callback will enable
> + * 	mac80211 to announce NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE.
> + * 	Packets already queued must not be send out encrypted with the new key
send out -> sent out
> + * 	and packets decoded with the old key must not be handed over to mac80211
> + * 	when the driver is not checking IV/ICV itself once the callback has been
> + * 	completed.
> + * 	Mac80211 will log an error when asked to use replace a PTK key
> + * 	without replace_key but will still perform the then potentially
> + * 	insecure action via set_key for backward compatibility for now.
> + *

Not sure this part really belongs in the driver method description?

>    * @update_tkip_key: See the section "Hardware crypto acceleration"
>    * 	This callback will be called in the context of Rx. Called for drivers
>    * 	which set IEEE80211_KEY_FLAG_TKIP_REQ_RX_P1_KEY.

<snip>

> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 4fb2709cb527..84cc8005c19a 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -572,9 +572,14 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
>   				      NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT);
>   	}
>   
> +	if (ops->replace_key)
> +		wiphy_ext_feature_set(wiphy,
> +				      NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE);
> +
>   	if (!ops->set_key)
>   		wiphy->flags |= WIPHY_FLAG_IBSS_RSN;
>   
> +

Stray whitespace?

>   	if (ops->wake_tx_queue)
>   		wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_TXQS);
>   

Regards,
-Denis

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[Alexander Wetzel]

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 <alexander@wetzel-home.de>
>> ---
>>   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 <denkenz@gmail.com>
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 2/3] mac80211: Define new driver callback replace_key

[Alexander Wetzel]

Hi,
> Hi Alexander,
> 
> Just minor nitpicks:
> 
>> + * @replace_key: Replace an exiting in use key with a new one while
>> guaranteeing
>> + *     to not leak clear text packets. Implementing this callback
>> will enable
>> + *     mac80211 to announce NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE.
>> + *     Packets already queued must not be send out encrypted with the
>> new key
> send out -> sent out

fixed in local git. Will wait some days for more feedback and then post v7.

>> + *     and packets decoded with the old key must not be handed over
>> to mac80211
>> + *     when the driver is not checking IV/ICV itself once the
>> callback has been
>> + *     completed.
>> + *     Mac80211 will log an error when asked to use replace a PTK key
>> + *     without replace_key but will still perform the then potentially
>> + *     insecure action via set_key for backward compatibility for now.
>> + *
> 
> Not sure this part really belongs in the driver method description?
>
>>    * @update_tkip_key: See the section "Hardware crypto acceleration"
>>    *     This callback will be called in the context of Rx. Called for
>> drivers
>>    *     which set IEEE80211_KEY_FLAG_TKIP_REQ_RX_P1_KEY.
> 
> <snip>
>

I'll update that and move parts of it to the "Hardware crypto
acceleration" DOC section.

>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index 4fb2709cb527..84cc8005c19a 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -572,9 +572,14 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t
>> priv_data_len,
>>                         NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT);
>>       }
>>   +    if (ops->replace_key)
>> +        wiphy_ext_feature_set(wiphy,
>> +                      NL80211_EXT_FEATURE_ATOMIC_KEY_REPLACE);
>> +
>>       if (!ops->set_key)
>>           wiphy->flags |= WIPHY_FLAG_IBSS_RSN;
>>   +
> 
> Stray whitespace?
> 
Yup, the new line makes zero sense. will also be fixed in v7.

>>       if (ops->wake_tx_queue)
>>           wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_TXQS);
>>   
> 
> Regards,
> -Denis

Alexander

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[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.

> 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

[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 <alexander@wetzel-home.de>
> ---
>  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 3/3] mac80211: Fix PTK rekey freezes and cleartext leaks

[Johannes Berg]

On Tue, 2018-08-14 at 12:42 +0200, Alexander Wetzel wrote:
> 
> +	/* PTK only using key ID 0 needs special handling on rekey */
> +	if (new_key && sta && ptk0rekey) {
> +		local = old_key->local;
> +		sdata = old_key->sdata;
> +
> +		/* Stop TX till we are on the new key */
> +		old_key->flags |= KEY_FLAG_TAINTED;
> +		ieee80211_clear_fast_xmit(sta);
> +
> +		/* Aggregation sessions during rekey are complicated due to
> +		 * the reorder buffer. Side step that by blocking aggregation
> +		 * and tear down running connections.
> +		 */
> +		if (ieee80211_hw_check(&local->hw, AMPDU_AGGREGATION)) {
> +			set_sta_flag(sta, WLAN_STA_BLOCK_BA);
> +			ieee80211_sta_tear_down_BA_sessions(sta,
> +							    AGG_STOP_LOCAL_REQUEST);
> +		}
> +
> +		if (new_key->local->ops->replace_key) {
> +			ret = drv_replace_key(old_key->local, sdata,
> +					      &sta->sta, &old_key->conf,
> +					      &new_key->conf);
> +			if (!ret)
> +				new_key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
> +			else
> +				sdata_err(sdata,
> +					  "failed to replace key (%d) for " \
> +					  "STA (%pM) in hardware: ret=(%d)\n",
> +					  old_key->conf.keyidx,
> +					  sta->sta.addr,
> +					  ret);
> +
> +			old_key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
> +		} else {
> +			sdata_info(sdata,
> +				   "Userspace requested a PTK rekey for STA " \
> +				   "%pM while feature not supported! " \
> +				   "This may leak clear text packets or " \
> +				   "freeze the connection.",
> +				   sta->sta.addr);

This seems a bit weird - we know a likely dangerous thing is happening
and only print an info message? Why not just prevent this in the first
place?

johannes

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[Alexander Wetzel]

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

[Alexander Wetzel]

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 <alexander@wetzel-home.de>
>> ---
>>  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

[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.

johannes

Re: [PATCH v6 3/3] mac80211: Fix PTK rekey freezes and cleartext leaks

[Alexander Wetzel]

Am 28.08.18 um 10:48 schrieb Johannes Berg:
> On Tue, 2018-08-14 at 12:42 +0200, Alexander Wetzel wrote:
>>
>> +	/* PTK only using key ID 0 needs special handling on rekey */
>> +	if (new_key && sta && ptk0rekey) {
>> +		local = old_key->local;
>> +		sdata = old_key->sdata;
>> +
>> +		/* Stop TX till we are on the new key */
>> +		old_key->flags |= KEY_FLAG_TAINTED;
>> +		ieee80211_clear_fast_xmit(sta);
>> +
>> +		/* Aggregation sessions during rekey are complicated due to
>> +		 * the reorder buffer. Side step that by blocking aggregation
>> +		 * and tear down running connections.
>> +		 */
>> +		if (ieee80211_hw_check(&local->hw, AMPDU_AGGREGATION)) {
>> +			set_sta_flag(sta, WLAN_STA_BLOCK_BA);
>> +			ieee80211_sta_tear_down_BA_sessions(sta,
>> +							    AGG_STOP_LOCAL_REQUEST);
>> +		}
>> +
>> +		if (new_key->local->ops->replace_key) {
>> +			ret = drv_replace_key(old_key->local, sdata,
>> +					      &sta->sta, &old_key->conf,
>> +					      &new_key->conf);
>> +			if (!ret)
>> +				new_key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
>> +			else
>> +				sdata_err(sdata,
>> +					  "failed to replace key (%d) for " \
>> +					  "STA (%pM) in hardware: ret=(%d)\n",
>> +					  old_key->conf.keyidx,
>> +					  sta->sta.addr,
>> +					  ret);
>> +
>> +			old_key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
>> +		} else {
>> +			sdata_info(sdata,
>> +				   "Userspace requested a PTK rekey for STA " \
>> +				   "%pM while feature not supported! " \
>> +				   "This may leak clear text packets or " \
>> +				   "freeze the connection.",
>> +				   sta->sta.addr);
> 
> This seems a bit weird - we know a likely dangerous thing is happening
> and only print an info message? Why not just prevent this in the first
> place?

The next version will upgrade that to a warning.

Rejecting it outright will break things for users with card/drivers
where rekey currently is working. There is no error error message for
"please re-associate as quick as you can" and tricking userspace to do
that across versions and implementations would need code at I do not see
belonging into the kernel. Here what I wrote in the cover letter of the
v4 version of the patch about this decision:

> I do not see an acceptable way from the kernel to trigger a fast
> reconnect. wpa_supplicant e.g. has some special code handling errors
> during a 4-way-handshake differently. I assume we would have to add
> code detecting if we are running as AP, Station Infrastructure, Ad-Hoc
> or Mesh and then find and spoof an error which allows the userspace to
> reconnect fast. For multiple different userspace implementations and
> the different versions of them.
> And we'll have that mess in the kernel for basically forever...
> Seems to be a clear no go, correct?
>
> So this patch (series) is giving up on a quick fix and will allow
> broken rekeys to continue. It is instead providing an updated API for
> both the userspace and the drivers to also do it correctly. Users
> running a userspace not aware of the new API will get some
> improvements but may
> still leak cleartext packets and have connection freezes till we get
> an updated userspace rolled out. On the plus side users running
> updated userspace won't be able to rekey connections any longer,
> avoiding the issues even with unpatched kernels.

Of course I may miss something and there may be a good way to get that
working anyhow. But for me it looks like we either have to accept
something which looks like a regression to users or accept that some
drivers may not be fixed with this patch alone and will need either an
updated userspace or drivers.

Alexander

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[Alexander Wetzel]

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 much newer than the userspace.
>
> Does this sound like an reasonable approch or should I go back figuring
> out how to trick the userspace to reconnect without user
> notification/intervention?

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?

Alexander

Re: [PATCH v6 3/3] mac80211: Fix PTK rekey freezes and cleartext leaks

[Johannes Berg]

On Tue, 2018-08-28 at 18:27 +0200, Alexander Wetzel wrote:

> > This seems a bit weird - we know a likely dangerous thing is happening
> > and only print an info message? Why not just prevent this in the first
> > place?
> 
> The next version will upgrade that to a warning.

Maybe make it rate limited, and certainly not a WARN_ON() though.

> Rejecting it outright will break things for users with card/drivers
> where rekey currently is working. There is no error error message for
> "please re-associate as quick as you can" and tricking userspace to do
> that across versions and implementations would need code at I do not see
> belonging into the kernel. Here what I wrote in the cover letter of the
> v4 version of the patch about this decision:
> 
> > I do not see an acceptable way from the kernel to trigger a fast
> > reconnect. wpa_supplicant e.g. has some special code handling errors
> > during a 4-way-handshake differently. I assume we would have to add
> > code detecting if we are running as AP, Station Infrastructure, Ad-Hoc
> > or Mesh and then find and spoof an error which allows the userspace to
> > reconnect fast. For multiple different userspace implementations and
> > the different versions of them.
> > And we'll have that mess in the kernel for basically forever...
> > Seems to be a clear no go, correct?
> > 
> > So this patch (series) is giving up on a quick fix and will allow
> > broken rekeys to continue. It is instead providing an updated API for
> > both the userspace and the drivers to also do it correctly. Users
> > running a userspace not aware of the new API will get some
> > improvements but may
> > still leak cleartext packets and have connection freezes till we get
> > an updated userspace rolled out. On the plus side users running
> > updated userspace won't be able to rekey connections any longer,
> > avoiding the issues even with unpatched kernels.
> 
> Of course I may miss something and there may be a good way to get that
> working anyhow. But for me it looks like we either have to accept
> something which looks like a regression to users or accept that some
> drivers may not be fixed with this patch alone and will need either an
> updated userspace or drivers.

Ok, fair enough. I may be willing to accept something as a regression,
but at the same time perhaps it doesn't really belong into this patch
anyway, so we can always do it as a later one.

johannes

Re: [PATCH v6 1/3] nl80211: Add ATOMIC_KEY_REPLACE API

[Johannes Berg]

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