Subject: Re: [PATCH v4 3/3] mac80211: Fix PTK rekey freezes and cleartext
 leaks
To: Ben Greear <greearb@candelatech.com>, johannes@sipsolutions.net
Cc: linux-wireless@vger.kernel.org, s.gottschall@dd-wrt.com,
        denkenz@gmail.com
References: <20180731201030.2619-1-alexander@wetzel-home.de>
 <20180731201030.2619-4-alexander@wetzel-home.de>
 <234090b7-f260-e5c9-d933-8ce5240d5689@candelatech.com>
From: Alexander Wetzel <alexander@wetzel-home.de>
Message-ID: <e7941f1c-4999-8cd5-75f5-2973a8ff8feb@wetzel-home.de> (sfid-20180801_192152_958788_32E5E472)
Date: Wed, 1 Aug 2018 19:01:11 +0200
MIME-Version: 1.0
In-Reply-To: <234090b7-f260-e5c9-d933-8ce5240d5689@candelatech.com>
Content-Type: text/plain; charset=windows-1252
Sender: linux-wireless-owner@vger.kernel.org

Hallo,

>> Rekeying a pairwise key with encryption offload and only keyid 0 had
>> multiple races. Two of them could freeze the wlan connection till
>> rekeyed again and the third could send out packets in clear which should
>> have been encrypted.
>>
>> ?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 while mac80211 was already
>> ??? operating on the new key.
>> ??? 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 later drop all packets
>> ??? really 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 (IV) value on the remote STA to
>> ??? an incorrect high number, which also froze the connection.
>>
>> ?3) Clear text leak:
>> ??? Removing encryption offload from the card cleared the encryption
>> ??? offload flag only after the card had removed the key. Packets handed
>> ??? over between that were send out unencrypted.
>>
>> To prevent those issues 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.
> 
> I guess the replace_key logic will have to flush the tx hardware/firmware
> queues and any other packets currently owned by the driver before it can
> replace the key?

That is one of the options, but looks like we can also avoid it with
some efforts. Question will be what's the best approach per driver and
how complex we want the code to become...
My current ath9k fix depends on flush - since it's simple - but it
definitely looks like it can be done without. (It looks like the
userspace updates are more urgent - assuming we can even agree on this
overall solution - and I've not done anything on it for weeks.)

There are two ways to establish the boundary we need here:
1) Make sure all old packets have been send/dropped or 2) continue to
use the old key for them.

Iwlwifi e.g. is handing over the key with the packet to the HW and the
driver simply uses the key it was told per packet. Implementing
replace_key is trivial, we can just call the existing set_key function
for deletion and addition from replace_key if we want. (I've tested that
quite extensive and it works flawless with my dvm card.)

Drivers uploading the keys to the HW and then just tell the HW to use
key ID X for encryption - like ath9, the only other driver I drilled
deeper so far - are more tricky. But replace_key is still allowed to
change the HW key ID for the new key if desired by the driver.

The driver can therefore only "deprecate" a key but keep it programmed
in HW, assign and program a new key and return to mac80211.
Any queued packets will still lookup and use the old key while new
packets will tell the HW to use the new one. Problem solved, if we
ignore the headache when and how to really free the old key id and
remove it from the HW. In the worst case we wait the max time any packet
can be stuck in the queue. Of course this may cause other issues in at
least some special cases. The one I can think of would be:

A busy AP with many clients connected and rekey enabled gets rebooted
during work hours. All clients reconnect at basically the same time.
Thus all of those will also rekey at nearly the same time and therefore
all need two PTK key slots in the HW for some seconds, potentially
exceeding the available ones. So you may get some strange effects after
the rekey interval expired, long after the reboot...

> ..
> 
>> +??????? /* Key is beeing removed */
> 
> Looks like 'being' is mis-spelled?

Thanks, I've fixed that in my local git tree. Guess I'll wait a few more
days till sending and hopefully accumulating more feedback in it.

Regards,

Alexander