Re: Proper SET_KEY usage?

Re: Proper SET_KEY usage?

[Johannes Berg]

Hi Denis,

Hope you don't mind me adding the list to the explanations, so they're
recorded. Your questions are completely reasonable, after all :-)

> So I've been poking around a bit more in nl80211/mac80211 stuff and I 
> was curious how exactly NEW_KEY/SET_KEY are supposed to be used.  The 
> documentation is lets say... less than stellar.

Yeah. This stuff grew out of WEXT mostly, and what things wpa_s did at
the time...

> So here's a excerpts from a capture of wpa_supplicant 2.6 connecting to 
> a PSK (CCMP + MFP-enabled) BSS.  So first thing it does is creates the 
> pairwise key via CMD_NEW_KEY:
> 
> < Request: New Key (0x0b) len 68 [ack]
>      Interface Index: 3 (0x00000003)
>      Key Data: len 16
>          [...]
>      Key Cipher: CCMP (00:0f:ac) suite  04
>      Key Sequence: len 6
>          00 00 00 00 00 00
>      MAC Address [...]
>      Key Index: 0 (0x00)
>  > Response: New Key (0x0b) len 4 [0x100]
>      Status: Success (0)
> 
> So far so good.

Yeah, that seems reasonable :-)

> The next thing it does is:
> < Request: Set Key (0x0a) len 28 [ack]
>      Interface Index: 3 (0x00000003)
>      Key Index: 0 (0x00)
>      Key Default: true
>      Key Default Types: len 4
>          Unicast: true
>  > Response: Set Key (0x0a) len 4 [0x100]
>      Status: Success (0)
> 
> This part is a bit bizarre.

Yeah. Maybe Jouni can chime in why this happens. I suspect the reason is
some legacy with wext or ancient drivers.

> First it seems to be a complete no-op on 
> mac80211 because mac80211 puts the key into sta->ptk, while 
> ieee80211_set_default_key is fully ignorant of the sta and only operates 
> on struct ieee80211_sub_if_data.  

Indeed. PTKs are also immediately selected for TX.

> Is this really intended to be used 
> this way?  Is SET_KEY useful / necessary in an STA context?  What is the 
> intended usage here?

Even if we implement, in the future, PTK rekeying properly with non-zero 
key index, I think we could still set the key for TX immediately, and
keep the other for RX, so it shouldn't be necessary in any way for a PTK
(or actually for a per-station key, which differs slightly) context.

(The difference between them is that in IBSS you will have per-station
GTKs, but that's also irrelevant because those are only used for RX -
your own GTK on the netdev/wdev/sdata/vif level is used for TX.)

> Okay, then wpa_s sets the Group key:
> < Request: New Key (0x0b) len 64 [ack]
>      Interface Index: 3 (0x00000003)
>      Key Data: len 16
>          [...]
>      Key Cipher: CCMP (00:0f:ac) suite  04
>      Key Sequence: len 6
>          c8 08 00 00 00 00
>      Key Default Types: len 4
>          Multicast: true
>      Key Index: 1 (0x01)
>  > Response: New Key (0x0b) len 4 [0x100]
>      Status: Success (0)
> 
> wpa_s doesn't use CMD_SET_KEY at all for the key created above.  Notice 
> the completely superfluous 'Key Default Types' attribute.  This will be 
> ignored by nl80211 when invoking the add_key driver method.

CMD_SET_KEY is only used in contexts where you might transmit with the
key, this isn't true for the GTK in client-mode.

The key types is an artifact of the IBSS I described I think, in this
case you don't have a station MAC address for the key, but for GTK in
IBSS you will have a MAC address (because the GTK for RX is per station)
but you still need to tag it as being a GTK and not PTK.

> Finally, the IGTK:
> < Request: New Key (0x0b) len 64 [ack]
>      Interface Index: 3 (0x00000003)
>      Key Data: len 16
>          06 2f e6 cf cf 3d ac 01 a4 4d 32 53 ef 45 2d df
>      Key Cipher: BIP (00:0f:ac) suite  06
>      Key Sequence: len 6
>          2e ae 8c 67 0d 33
>      Key Default Types: len 4
>          Multicast: true
>      Key Index: 4 (0x04)
> 
> Again, no SET_KEY and the superfluous 'Key Default Types'.

Same considerations as for the GTK.

> So I wonder what is the purpose of NL80211_ATTR_KEY_DEFAULT_MGMT?  And 
> how is it supposed to be used?

This should be used only in AP mode though I think mac80211 doesn't
because it just assumes once you get a new key you want to use it, but
in theory you could switch back to an old key with it I think? At least
from an API POV, perhaps not from an implementation POV.

> It seems this was intended for SET_KEY to 
> be used with that attribute to enable protected management frame 
> transmission (see commit 3cfcf6ac6d69d).  And default_mgmt_key is still 
> being referenced in ieee80211_tx_h_select_key().  However, at least 
> according to this behavior it is never set.

Not in client mode. But in client mode you don't TX with the management
key at all - you only RX with the multicast keys (GTK/IGTK).

> The same question is also relevant for default_multicast_key...

In AP/IBSS (and mesh?) modes this becomes relevant though, and then
these do get set. nl80211_set_key() will call through if the key is
default or default mgmt, and that will - in mac80211 - set these
pointers to use for TX.

Hope that helps a bit!

johannes

Re: Proper SET_KEY usage?

[Denis Kenzior]

Hi Johannes,

On 06/29/2018 05:01 AM, Johannes Berg wrote:
> Hi Denis,
> 
> Hope you don't mind me adding the list to the explanations, so they're
> recorded. Your questions are completely reasonable, after all :-)
> 

Absolutely do not mind.

>> So I've been poking around a bit more in nl80211/mac80211 stuff and I
>> was curious how exactly NEW_KEY/SET_KEY are supposed to be used.  The
>> documentation is lets say... less than stellar.
> 
> Yeah. This stuff grew out of WEXT mostly, and what things wpa_s did at
> the time...

Right.  My main intent with this was to see if I could understand things 
enough to actually start fixing some of the docs, see if we need to 
deprecate some things and/or maybe fix bits inside nl80211.

> 
>> So here's a excerpts from a capture of wpa_supplicant 2.6 connecting to
>> a PSK (CCMP + MFP-enabled) BSS.  So first thing it does is creates the
>> pairwise key via CMD_NEW_KEY:
>>
>> < Request: New Key (0x0b) len 68 [ack]
>>       Interface Index: 3 (0x00000003)
>>       Key Data: len 16
>>           [...]
>>       Key Cipher: CCMP (00:0f:ac) suite  04
>>       Key Sequence: len 6
>>           00 00 00 00 00 00
>>       MAC Address [...]
>>       Key Index: 0 (0x00)
>>   > Response: New Key (0x0b) len 4 [0x100]
>>       Status: Success (0)
>>
>> So far so good.
> 
> Yeah, that seems reasonable :-)
> 
>> The next thing it does is:
>> < Request: Set Key (0x0a) len 28 [ack]
>>       Interface Index: 3 (0x00000003)
>>       Key Index: 0 (0x00)
>>       Key Default: true
>>       Key Default Types: len 4
>>           Unicast: true
>>   > Response: Set Key (0x0a) len 4 [0x100]
>>       Status: Success (0)
>>
>> This part is a bit bizarre.
> 
> Yeah. Maybe Jouni can chime in why this happens. I suspect the reason is
> some legacy with wext or ancient drivers.
> 
>> First it seems to be a complete no-op on
>> mac80211 because mac80211 puts the key into sta->ptk, while
>> ieee80211_set_default_key is fully ignorant of the sta and only operates
>> on struct ieee80211_sub_if_data.
> 
> Indeed. PTKs are also immediately selected for TX.
> 
>> Is this really intended to be used
>> this way?  Is SET_KEY useful / necessary in an STA context?  What is the
>> intended usage here?
> 
> Even if we implement, in the future, PTK rekeying properly with non-zero
> key index, I think we could still set the key for TX immediately, and
> keep the other for RX, so it shouldn't be necessary in any way for a PTK
> (or actually for a per-station key, which differs slightly) context.

So it sounds like SET_KEY is completely unnecessary for a unicast key in 
an RSN/WPA association for an STA.  Should we update mac80211 (and 
possibly nl80211, though likely it doesn't have enough info) to issue a 
warning when someone tries to use set_key on such a key?

What about AP/IBSS?  Aren't unicast keys also per-station in this case? 
It sounds like SET_KEY isn't useful in this context either, right?

What about WEP?

Put another way, should we deprecate the whole 
NL80211_KEY_DEFAULT_TYPE_UNICAST attribute?

And since you went off on a tangent (maybe this needs a separate 
discussion?) but... Can you elaborate about PTK rekeying with a non-zero 
key index?  Are you saying that in IBSS/AP mode we can't support PTK rekey?

> 
> (The difference between them is that in IBSS you will have per-station
> GTKs, but that's also irrelevant because those are only used for RX -
> your own GTK on the netdev/wdev/sdata/vif level is used for TX.)

Okay, so for GTKs we would have a per-station RX GTK and a 'default' TX 
GTK.  So in this case a SET_KEY would be needed only on the 'default' TX 
GTK index.

Okay, maybe a tangent, but this brings up a question: Why do we have 
separate steps for this operation?  Can't we just issue a NEW_KEY and 
have the kernel figure this out?

> 
>> Okay, then wpa_s sets the Group key:
>> < Request: New Key (0x0b) len 64 [ack]
>>       Interface Index: 3 (0x00000003)
>>       Key Data: len 16
>>           [...]
>>       Key Cipher: CCMP (00:0f:ac) suite  04
>>       Key Sequence: len 6
>>           c8 08 00 00 00 00
>>       Key Default Types: len 4
>>           Multicast: true
>>       Key Index: 1 (0x01)
>>   > Response: New Key (0x0b) len 4 [0x100]
>>       Status: Success (0)
>>
>> wpa_s doesn't use CMD_SET_KEY at all for the key created above.  Notice
>> the completely superfluous 'Key Default Types' attribute.  This will be
>> ignored by nl80211 when invoking the add_key driver method.
> 
> CMD_SET_KEY is only used in contexts where you might transmit with the
> key, this isn't true for the GTK in client-mode.
> 
> The key types is an artifact of the IBSS I described I think, in this
> case you don't have a station MAC address for the key, but for GTK in
> IBSS you will have a MAC address (because the GTK for RX is per station)
> but you still need to tag it as being a GTK and not PTK.
> 

Well, that's what I was pointing out earlier, the whole Multicast 
attribute is ignored and should not be sent in the first place:

 >>       Key Default Types: len 4
 >>           Multicast: true

The proof :) follows:

add_key has the following signature:
         int     (*add_key)(struct wiphy *wiphy, struct net_device *netdev,
                            u8 key_index, bool pairwise, const u8 *mac_addr,
                            struct key_params *params);


and struct key_params is:
struct key_params {
         const u8 *key;
         const u8 *seq;
         int key_len;
         int seq_len;
         u32 cipher;
};

So while nl80211_new_key actually parses this information (stored in 
struct key_parse) it never forwards any of it to the driver.  The group 
key vs pairwise key determination seems to be predicated on presence of 
NL80211_ATTR_KEY_TYPE and the following fallback:

         if (key.type == -1) {
                 if (mac_addr)
                         key.type = NL80211_KEYTYPE_PAIRWISE;
                 else
                         key.type = NL80211_KEYTYPE_GROUP;
         }


>> Finally, the IGTK:
>> < Request: New Key (0x0b) len 64 [ack]
>>       Interface Index: 3 (0x00000003)
>>       Key Data: len 16
>>           06 2f e6 cf cf 3d ac 01 a4 4d 32 53 ef 45 2d df
>>       Key Cipher: BIP (00:0f:ac) suite  06
>>       Key Sequence: len 6
>>           2e ae 8c 67 0d 33
>>       Key Default Types: len 4
>>           Multicast: true
>>       Key Index: 4 (0x04)
>>
>> Again, no SET_KEY and the superfluous 'Key Default Types'.
> 
> Same considerations as for the GTK.
> 
>> So I wonder what is the purpose of NL80211_ATTR_KEY_DEFAULT_MGMT?  And
>> how is it supposed to be used?
> 
> This should be used only in AP mode though I think mac80211 doesn't
> because it just assumes once you get a new key you want to use it, but
> in theory you could switch back to an old key with it I think? At least
> from an API POV, perhaps not from an implementation POV.

Okay, so the drivers are expected to distinguish between a GTK and IGTK 
based on the cipher type, right?

So do we need a driver feature bit for NL80211_ATTR_KEY_DEFAULT_MGMT?

> 
>> It seems this was intended for SET_KEY to
>> be used with that attribute to enable protected management frame
>> transmission (see commit 3cfcf6ac6d69d).  And default_mgmt_key is still
>> being referenced in ieee80211_tx_h_select_key().  However, at least
>> according to this behavior it is never set.
> 
> Not in client mode. But in client mode you don't TX with the management
> key at all - you only RX with the multicast keys (GTK/IGTK).

Okay, that makes sense

> 
>> The same question is also relevant for default_multicast_key...
> 
> In AP/IBSS (and mesh?) modes this becomes relevant though, and then
> these do get set. nl80211_set_key() will call through if the key is
> default or default mgmt, and that will - in mac80211 - set these
> pointers to use for TX.
> 

Okay, so to summarize, SET_KEY should only be called on GTK/IGTK keys 
which are not per-MAC.  So in IBSS/AP mode we set this for our transmit 
GTK/IGTK key.

In IBSS mode, SET_KEY on peer's GTK shouldn't be issued.  In fact it may 
even be an error to do so since we might be messing with the wrong key 
unintentionally.

So I'd like to bring up a point I mentioned in the original message, 
should ieee80211_set_default_key() be made to return an error if 
userspace is trying to mess with a non-existent key?  That is 
effectively what happens in the STA log provided...

> Hope that helps a bit!
> 

Yep, thanks.

Re: Proper SET_KEY usage?

[Johannes Berg]

Hi,

> > Yeah. This stuff grew out of WEXT mostly, and what things wpa_s did at
> > the time...
> 
> Right.  My main intent with this was to see if I could understand things 
> enough to actually start fixing some of the docs, see if we need to 
> deprecate some things and/or maybe fix bits inside nl80211.

:-)
That'd be very welcome!

> So it sounds like SET_KEY is completely unnecessary for a unicast key in 
> an RSN/WPA association for an STA.  Should we update mac80211 (and 
> possibly nl80211, though likely it doesn't have enough info) to issue a 
> warning when someone tries to use set_key on such a key?

I'm not sure I see much point, since we'll have to deal with older wpa_s
basically forever?

> What about AP/IBSS?  Aren't unicast keys also per-station in this case? 
> It sounds like SET_KEY isn't useful in this context either, right?

Yes, same situation really.

> What about WEP?

WEP is a special case, you typically use the same key for all traffic,
and may switch around between them.

In theory though, and I think this API allows you to do it, you can have
multiple WEP keys and use different ones for unicast/multicast TX. Or
the same. Or switch them around as you like.

> Put another way, should we deprecate the whole 
> NL80211_KEY_DEFAULT_TYPE_UNICAST attribute?

Perhaps. I'm not sure I see much point in it. The WEP case above is
pretty fringe, and wext didn't support it, so ... probably not needed?

> And since you went off on a tangent (maybe this needs a separate 
> discussion?) but... Can you elaborate about PTK rekeying with a non-zero 
> key index?  Are you saying that in IBSS/AP mode we can't support PTK rekey?

We can't properly support PTK rekeying in any mode, see Alexander
Wetzel's patch and the whole discussion on the different versions I had
with him. I'll merge his patch soon I think, but it doesn't really work
properly.

Some time relatively recently (802.11-2016 I think) the spec has
introduced a method to indicate that you support what was previously not
allowed at all: using non-zero key index for a PTK. This way, you can do
transparent PTK rekeying like you do with GTK now, by switching to a new
key index when you rekey.

We don't - currently - support this in mac80211 or even wpa_supplicant,
and I expect many devices would have issues with this with hardware
offload. I'm also not aware of any certification program for it, but I
also haven't followed the WPA3 efforts (but I don't think they're
focused here, they're focused on algorithms and higher layers IIRC.)

> > (The difference between them is that in IBSS you will have per-station
> > GTKs, but that's also irrelevant because those are only used for RX -
> > your own GTK on the netdev/wdev/sdata/vif level is used for TX.)
> 
> Okay, so for GTKs we would have a per-station RX GTK and a 'default' TX 
> GTK.  So in this case a SET_KEY would be needed only on the 'default' TX 
> GTK index.
> 
> Okay, maybe a tangent, but this brings up a question: Why do we have 
> separate steps for this operation?  Can't we just issue a NEW_KEY and 
> have the kernel figure this out?

I think the only case would be WEP?

> Well, that's what I was pointing out earlier, the whole Multicast 
> attribute is ignored and should not be sent in the first place:
> 
>  >>       Key Default Types: len 4
>  >>           Multicast: true
> 
> The proof :) follows:

:)

> So while nl80211_new_key actually parses this information (stored in 
> struct key_parse) it never forwards any of it to the driver.  The group 
> key vs pairwise key determination seems to be predicated on presence of 
> NL80211_ATTR_KEY_TYPE and the following fallback:
> 
>          if (key.type == -1) {
>                  if (mac_addr)
>                          key.type = NL80211_KEYTYPE_PAIRWISE;
>                  else
>                          key.type = NL80211_KEYTYPE_GROUP;
>          }

Yeah.

> Okay, so the drivers are expected to distinguish between a GTK and IGTK 
> based on the cipher type, right?

Not needed, IGTK also uses key index 4/5, while GTK uses 0-3.

> So do we need a driver feature bit for NL80211_ATTR_KEY_DEFAULT_MGMT?

I don't see why, the driver should support it if it has support for IGTK
in the first place? And for AP mode, since otherwise it doesn't matter
(IGTK like GTK is RX-only for the client)

> Okay, so to summarize, SET_KEY should only be called on GTK/IGTK keys 
> which are not per-MAC.  So in IBSS/AP mode we set this for our transmit 
> GTK/IGTK key.

I suspect you don't even *need* SET_KEY on GTK/IGTK since they're set to
be TX when installed, I think?

> In IBSS mode, SET_KEY on peer's GTK shouldn't be issued.  In fact it may 
> even be an error to do so since we might be messing with the wrong key 
> unintentionally.

I don't think we would mess with the wrong key, since we need to give
the station MAC address and then it's not our TX key. But certainly
doing SET_KEY for a GTK that you expect to *receive* with isn't useful.

> So I'd like to bring up a point I mentioned in the original message, 
> should ieee80211_set_default_key() be made to return an error if 
> userspace is trying to mess with a non-existent key?  That is 
> effectively what happens in the STA log provided...

Perhaps, though that might break wpa_s?

johannes

Re: Proper SET_KEY usage?

[Denis Kenzior]

Hi Johannes,

On 07/06/2018 07:17 AM, Johannes Berg wrote:
> Hi,
> 
>>> Yeah. This stuff grew out of WEXT mostly, and what things wpa_s did at
>>> the time...
>>
>> Right.  My main intent with this was to see if I could understand things
>> enough to actually start fixing some of the docs, see if we need to
>> deprecate some things and/or maybe fix bits inside nl80211.
> 
> :-)
> That'd be very welcome!
> 
>> So it sounds like SET_KEY is completely unnecessary for a unicast key in
>> an RSN/WPA association for an STA.  Should we update mac80211 (and
>> possibly nl80211, though likely it doesn't have enough info) to issue a
>> warning when someone tries to use set_key on such a key?
> 
> I'm not sure I see much point, since we'll have to deal with older wpa_s
> basically forever?

Do we though?  I can understand not breaking userspace APIs, but it 
seems like this API had only a single user (up until recently) and it 
'should' be acceptable to break ancient versions of it.  Whether we can 
or not is a question for the wider community though...

> 
>> What about AP/IBSS?  Aren't unicast keys also per-station in this case?
>> It sounds like SET_KEY isn't useful in this context either, right?
> 
> Yes, same situation really.
> 
>> What about WEP?
> 
> WEP is a special case, you typically use the same key for all traffic,
> and may switch around between them.
> 
> In theory though, and I think this API allows you to do it, you can have
> multiple WEP keys and use different ones for unicast/multicast TX. Or
> the same. Or switch them around as you like.
> 
>> Put another way, should we deprecate the whole
>> NL80211_KEY_DEFAULT_TYPE_UNICAST attribute?
> 
> Perhaps. I'm not sure I see much point in it. The WEP case above is
> pretty fringe, and wext didn't support it, so ... probably not needed?

Should we at least take this out of the driver API then and have it 
ignored by nl80211?  And maybe mark this deprecated in the docs?

> 
>> And since you went off on a tangent (maybe this needs a separate
>> discussion?) but... Can you elaborate about PTK rekeying with a non-zero
>> key index?  Are you saying that in IBSS/AP mode we can't support PTK rekey?
> 
> We can't properly support PTK rekeying in any mode, see Alexander
> Wetzel's patch and the whole discussion on the different versions I had
> with him. I'll merge his patch soon I think, but it doesn't really work
> properly.

I think we had this conversation before.  Up to 802.11-2012, PTK Rekey 
was not really explicitly mentioned as possible.  There were hints and 
stuff, but no explicit language.

I think in 802.11-2016 they finally explicitly say that this is possible.

However, we seem to have networks that perform PTK Rekey and even full 
802.1X re-auth every hour (eduroam for example).  How is this working? 
Or is it a case of it not always working?

> 
> Some time relatively recently (802.11-2016 I think) the spec has
> introduced a method to indicate that you support what was previously not
> allowed at all: using non-zero key index for a PTK. This way, you can do
> transparent PTK rekeying like you do with GTK now, by switching to a new
> key index when you rekey.

And keep the old one for whatever packets are in process, right?  That 
makes more sense.

> 
> We don't - currently - support this in mac80211 or even wpa_supplicant,
> and I expect many devices would have issues with this with hardware
> offload. I'm also not aware of any certification program for it, but I
> also haven't followed the WPA3 efforts (but I don't think they're
> focused here, they're focused on algorithms and higher layers IIRC.)
> 

Does the recent PMK handshake offload support proposal take into 
consideration PTK rekeying?

>>> (The difference between them is that in IBSS you will have per-station
>>> GTKs, but that's also irrelevant because those are only used for RX -
>>> your own GTK on the netdev/wdev/sdata/vif level is used for TX.)
>>
>> Okay, so for GTKs we would have a per-station RX GTK and a 'default' TX
>> GTK.  So in this case a SET_KEY would be needed only on the 'default' TX
>> GTK index.
>>
>> Okay, maybe a tangent, but this brings up a question: Why do we have
>> separate steps for this operation?  Can't we just issue a NEW_KEY and
>> have the kernel figure this out?
> 
> I think the only case would be WEP?

So can we expect all drivers to operate this way?  Or should we maybe 
also have nl80211 call set_key operation on a key with no associated STA 
implicitly?

> 
>> Well, that's what I was pointing out earlier, the whole Multicast
>> attribute is ignored and should not be sent in the first place:
>>
>>   >>       Key Default Types: len 4
>>   >>           Multicast: true
>>
>> The proof :) follows:
> 
> :)
> 
>> So while nl80211_new_key actually parses this information (stored in
>> struct key_parse) it never forwards any of it to the driver.  The group
>> key vs pairwise key determination seems to be predicated on presence of
>> NL80211_ATTR_KEY_TYPE and the following fallback:
>>
>>           if (key.type == -1) {
>>                   if (mac_addr)
>>                           key.type = NL80211_KEYTYPE_PAIRWISE;
>>                   else
>>                           key.type = NL80211_KEYTYPE_GROUP;
>>           }
> 
> Yeah.
> 
>> Okay, so the drivers are expected to distinguish between a GTK and IGTK
>> based on the cipher type, right?
> 
> Not needed, IGTK also uses key index 4/5, while GTK uses 0-3.
> 
>> So do we need a driver feature bit for NL80211_ATTR_KEY_DEFAULT_MGMT?
> 
> I don't see why, the driver should support it if it has support for IGTK
> in the first place? And for AP mode, since otherwise it doesn't matter
> (IGTK like GTK is RX-only for the client)
> 
>> Okay, so to summarize, SET_KEY should only be called on GTK/IGTK keys
>> which are not per-MAC.  So in IBSS/AP mode we set this for our transmit
>> GTK/IGTK key.
> 
> I suspect you don't even *need* SET_KEY on GTK/IGTK since they're set to
> be TX when installed, I think?
> 
>> In IBSS mode, SET_KEY on peer's GTK shouldn't be issued.  In fact it may
>> even be an error to do so since we might be messing with the wrong key
>> unintentionally.
> 
> I don't think we would mess with the wrong key, since we need to give
> the station MAC address and then it's not our TX key. But certainly
> doing SET_KEY for a GTK that you expect to *receive* with isn't useful.

Right.

> 
>> So I'd like to bring up a point I mentioned in the original message,
>> should ieee80211_set_default_key() be made to return an error if
>> userspace is trying to mess with a non-existent key?  That is
>> effectively what happens in the STA log provided...
> 
> Perhaps, though that might break wpa_s?
> 

Doesn't wpa_s ignore most errors anyway? :)

Regards,
-Denis

Re: Proper SET_KEY usage?

[Adrian Chadd]

On Tue, 10 Jul 2018 at 12:16, Denis Kenzior <denkenz@gmail.com> wrote:

> I think we had this conversation before.  Up to 802.11-2012, PTK Rekey
> was not really explicitly mentioned as possible.  There were hints and
> stuff, but no explicit language.
>
> I think in 802.11-2016 they finally explicitly say that this is possible.
>
> However, we seem to have networks that perform PTK Rekey and even full
> 802.1X re-auth every hour (eduroam for example).  How is this working?
> Or is it a case of it not always working?

It's ... buggy. Right now I'm hitting race conditions (which someone
is actively working on now, yay!) where frames are going out in a
narrow window between the hardware key being rekeyed (and the RX PN
being set to 0) and an older frame going out with a larger PN with the
new key. The receiver sees the frame with the old, large PN but the
new key and .. well, subsequent traffic hangs. I know it's buggy on
ath9k (what we're using at work.) ath10k seems to fare better - it at
least is doing key programming and PN assignment in firmware, so it
has a chance to keep it in sync.



-adrian

Re: Proper SET_KEY usage?

[Johannes Berg]

On Tue, 2018-07-10 at 14:16 -0500, Denis Kenzior wrote:

> > I'm not sure I see much point, since we'll have to deal with older wpa_s
> > basically forever?
> 
> Do we though?  I can understand not breaking userspace APIs, but it 
> seems like this API had only a single user (up until recently) and it 
> 'should' be acceptable to break ancient versions of it.  Whether we can 
> or not is a question for the wider community though...

I don't think so. It'll be hurting various consumers like Android,
Chrome, etc.

"Ancient" versions is also relative - there hasn't been a release of
wpa_s in a long time, for example.

> > > Put another way, should we deprecate the whole
> > > NL80211_KEY_DEFAULT_TYPE_UNICAST attribute?
> > 
> > Perhaps. I'm not sure I see much point in it. The WEP case above is
> > pretty fringe, and wext didn't support it, so ... probably not needed?
> 
> Should we at least take this out of the driver API then and have it 
> ignored by nl80211?  And maybe mark this deprecated in the docs?

I guess we could do that, yeah.

> > > And since you went off on a tangent (maybe this needs a separate
> > > discussion?) but... Can you elaborate about PTK rekeying with a non-zero
> > > key index?  Are you saying that in IBSS/AP mode we can't support PTK rekey?
> > 
> > We can't properly support PTK rekeying in any mode, see Alexander
> > Wetzel's patch and the whole discussion on the different versions I had
> > with him. I'll merge his patch soon I think, but it doesn't really work
> > properly.
> 
> I think we had this conversation before.  Up to 802.11-2012, PTK Rekey 
> was not really explicitly mentioned as possible.  There were hints and 
> stuff, but no explicit language.

Actually it was sort of forbidden - key index had to be 0 for PTK, and
so you couldn't do a proper hand-over.

> I think in 802.11-2016 they finally explicitly say that this is possible.

Yes.

> However, we seem to have networks that perform PTK Rekey and even full 
> 802.1X re-auth every hour (eduroam for example).  How is this working? 
> Or is it a case of it not always working?

It's not working well in practice, see Alexander's patchset.

> > Some time relatively recently (802.11-2016 I think) the spec has
> > introduced a method to indicate that you support what was previously not
> > allowed at all: using non-zero key index for a PTK. This way, you can do
> > transparent PTK rekeying like you do with GTK now, by switching to a new
> > key index when you rekey.
> 
> And keep the old one for whatever packets are in process, right?  That 
> makes more sense.

Like GTK - you just keep it "forever" (until the next rekey round) for
RX, but nobody uses it for TX anymore.

> > We don't - currently - support this in mac80211 or even wpa_supplicant,
> > and I expect many devices would have issues with this with hardware
> > offload. I'm also not aware of any certification program for it, but I
> > also haven't followed the WPA3 efforts (but I don't think they're
> > focused here, they're focused on algorithms and higher layers IIRC.)
> 
> Does the recent PMK handshake offload support proposal take into 
> consideration PTK rekeying?

No, I don't think the two are related? If you offload the handshake, you
may or may not implement PTK rekeying in the 802.11-2016 or in the non-
working way ...

> > > > (The difference between them is that in IBSS you will have per-station
> > > > GTKs, but that's also irrelevant because those are only used for RX -
> > > > your own GTK on the netdev/wdev/sdata/vif level is used for TX.)
> > > 
> > > Okay, so for GTKs we would have a per-station RX GTK and a 'default' TX
> > > GTK.  So in this case a SET_KEY would be needed only on the 'default' TX
> > > GTK index.
> > > 
> > > Okay, maybe a tangent, but this brings up a question: Why do we have
> > > separate steps for this operation?  Can't we just issue a NEW_KEY and
> > > have the kernel figure this out?
> > 
> > I think the only case would be WEP?
> 
> So can we expect all drivers to operate this way?  Or should we maybe 
> also have nl80211 call set_key operation on a key with no associated STA 
> implicitly?

I don't know what drivers do, but I guess you could find out pretty
easily and then do that, yeah. I suspect they just all ignore it because
they couldn't figure out what the point was :)

> > > So I'd like to bring up a point I mentioned in the original message,
> > > should ieee80211_set_default_key() be made to return an error if
> > > userspace is trying to mess with a non-existent key?  That is
> > > effectively what happens in the STA log provided...
> > 
> > Perhaps, though that might break wpa_s?
> > 
> 
> Doesn't wpa_s ignore most errors anyway? :)

Many, but you never really know without trying :)

johannes