| Message ID | CAKXXJEzWfNrQxquCzA5dkD5=xCXWyeAC+_yBT35N1oDk-H_Buw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
On Tue, 2017-05-09 at 14:16 -0400, Michael Skeffington wrote: > In order to allow wpa_supplicant to correctly identify a perceived > WPA TKIP key > recovery attack the michael MIC must be checked before the packet > decode is > attempted. A packet with an invalid MIC will always fail a decrypt > check which > previously was being checked first. Therefore the MIC failure bit of > status flags > describing the error would remain unset. This isn't how the Michael MIC works. I have no idea what problem you're trying to solve, but this is not the solution. johannes
On Tue, May 09, 2017 at 02:16:31PM -0400, Michael Skeffington wrote: > In order to allow wpa_supplicant to correctly identify a perceived WPA TKIP key > recovery attack the michael MIC must be checked before the packet decode is > attempted. A packet with an invalid MIC will always fail a decrypt check which > previously was being checked first. Therefore the MIC failure bit of > status flags > describing the error would remain unset. Which driver and WLAN hardware are you using? Michael MIC is encrypted, so to be able to check that, the frame will obviously need to be decrypted first. If that WEP decryption fails, this frame needs to be dropped without indicating Michael MIC failure. WEP part here is completely independent of Michael MIC. It is possible that there is a driver that handles these steps in hardware/firmware and if so, that driver may have a bug if you do not see Michael MIC failures reported correctly. Anyway, as Johannes pointed out, this part in mac80211 is in the correct sequence and that cannot be changed since it would completely break TKIP for more or less all software-based cases.
I am using an rt5350 SoC using the rt2x00 driver. We were doing WiFi-alliance certification testing on our device and the it wasn't issuing countermeasures appropriately. Your assumption is correct. I had overlooked that devices using this driver have hardware decoding and the driver sets RX_FLAG_MMIC_ERROR. In retrospect, the change I proposed is totally broken. I'm running through the failure case again so I can identify where in the rx_decrypt function it falls through. It seems odd that it would drop the packet in rx_decrypt given that it doesn't actually do any decryption. I suspect thats related to the underlying bug. On Wed, May 10, 2017 at 8:24 AM, Jouni Malinen <j@w1.fi> wrote: > On Tue, May 09, 2017 at 02:16:31PM -0400, Michael Skeffington wrote: >> In order to allow wpa_supplicant to correctly identify a perceived WPA TKIP key >> recovery attack the michael MIC must be checked before the packet decode is >> attempted. A packet with an invalid MIC will always fail a decrypt check which >> previously was being checked first. Therefore the MIC failure bit of >> status flags >> describing the error would remain unset. > > Which driver and WLAN hardware are you using? Michael MIC is encrypted, > so to be able to check that, the frame will obviously need to be > decrypted first. If that WEP decryption fails, this frame needs to be > dropped without indicating Michael MIC failure. WEP part here is > completely independent of Michael MIC. > > It is possible that there is a driver that handles these steps in > hardware/firmware and if so, that driver may have a bug if you do not > see Michael MIC failures reported correctly. Anyway, as Johannes pointed > out, this part in mac80211 is in the correct sequence and that cannot be > changed since it would completely break TKIP for more or less all > software-based cases. > > -- > Jouni Malinen PGP id EFC895FA
On Thu, 2017-05-11 at 16:22 -0400, Michael Skeffington wrote: > I am using an rt5350 SoC using the rt2x00 driver. We were doing > WiFi-alliance certification testing on our device and the it wasn't > issuing countermeasures appropriately. > > Your assumption is correct. I had overlooked that devices using this > driver have hardware decoding and the driver sets RX_FLAG_MMIC_ERROR. > In retrospect, the change I proposed is totally broken. > > I'm running through the failure case again so I can identify where in > the rx_decrypt function it falls through. It seems odd that it would > drop the packet in rx_decrypt given that it doesn't actually do any > decryption. I suspect thats related to the underlying bug. Here's the driver code from rt2500usb (but it's similar in the others): rxdesc->flags |= RX_FLAG_MMIC_STRIPPED; if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) rxdesc->flags |= RX_FLAG_DECRYPTED; else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) rxdesc->flags |= RX_FLAG_MMIC_ERROR; I think if you just change it to be [...] else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) rxdesc->flags |= RX_FLAG_MMIC_ERROR | RX_FLAG_DECRYPTED; things will start working. This is arguably correct since to be able to check the MMIC, the frame has to have been decrypted (properly) before. johannes
Johannes, Thank you for that. I need to make a quick hack to send an invalid MIC packet from another device to test the countermeasures. Should I submit a new patch with this change when I've completed testing or are you already prepared to do so? Michael On Fri, May 12, 2017 at 4:52 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > Here's the driver code from rt2500usb (but it's similar in the others): > > rxdesc->flags |= RX_FLAG_MMIC_STRIPPED; > if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) > rxdesc->flags |= RX_FLAG_DECRYPTED; > else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) > rxdesc->flags |= RX_FLAG_MMIC_ERROR; > > I think if you just change it to be > > [...] > else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) > rxdesc->flags |= RX_FLAG_MMIC_ERROR | > RX_FLAG_DECRYPTED; > > things will start working. This is arguably correct since to be able to > check the MMIC, the frame has to have been decrypted (properly) before. > > johannes
On Tue, 2017-05-16 at 15:57 -0400, Michael Skeffington wrote: > Johannes, > > Thank you for that. I need to make a quick hack to send an invalid > MIC packet from another device to test the countermeasures. Should I > submit a new patch with this change when I've completed testing or > are you already prepared to do so? Please do, you're able to test it, and I'm not really all that familiar with that particular driver either, even if I maintain mac80211 :) johannes
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index bc08185..71f1a56 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -3176,9 +3176,10 @@ static void ieee80211_rx_handlers(struct ieee80211_rx_data *rx, CALL_RXH(ieee80211_rx_h_check_more_data) CALL_RXH(ieee80211_rx_h_uapsd_and_pspoll) CALL_RXH(ieee80211_rx_h_sta_process) + /* must be before decrypt so MIC failures are reported to netlink */ + CALL_RXH(ieee80211_rx_h_michael_mic_verify) CALL_RXH(ieee80211_rx_h_decrypt) CALL_RXH(ieee80211_rx_h_defragment) - CALL_RXH(ieee80211_rx_h_michael_mic_verify) /* must be after MMIC verify so header is counted in MPDU mic */
In order to allow wpa_supplicant to correctly identify a perceived WPA TKIP key recovery attack the michael MIC must be checked before the packet decode is attempted. A packet with an invalid MIC will always fail a decrypt check which previously was being checked first. Therefore the MIC failure bit of status flags describing the error would remain unset. Signed-off-by: Michael Skeffington <mike@hellotwist.com> --- #ifdef CONFIG_MAC80211_MESH if (ieee80211_vif_is_mesh(&rx->sdata->vif))