Re: [PATCH v2] rt2x00: Fix MMIC Countermeasures.

[Michael Skeffington]

Sorry about that, I work on other projects that use spaces and I must have
missed that when double checking the patch.  I sent out a new patch with
the correct line length, removal of trailing '.' and indentation fix.

On Mon, Aug 7, 2017 at 2:31 AM, Stanislaw Gruszka  wrote:
> On Thu, Aug 03, 2017 at 11:31:21AM -0400, Michael Skeffingfon wrote:
>> @@ -136,10 +136,19 @@ void rt2800mmio_fill_rxdone(struct queue_entry *entry,
>>*/
>>   rxdesc->flags |= RX_FLAG_MMIC_STRIPPED;
>>
>> - if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS)
>> + if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) {
>>   rxdesc->flags |= RX_FLAG_DECRYPTED;
>> - else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC)
>> +} else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) {
>
> Not sure why this happened, but here and on some other places below,
> tab was replaced by spaces resulting in wrong indent.
>
> Stanislaw

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Michael Skeffington]

I traced through this code during MMIC failure and
ieee80211_rx_h_decrypt() drops the frame before getting to
ieee80211_rx_h_michael_mic_verify().  Johannes suggested this change
to me in response to a previous thread and I am offering this patch
after having conducted the proper testing on it.

On Wed, Aug 2, 2017 at 9:43 AM, Kalle Valo  wrote:
> Johannes Berg  writes:
>
>> On Wed, 2017-08-02 at 09:01 +0200, Stanislaw Gruszka wrote:
>>
>>> The relevant mac80211 code look like this:
>>>
>>> ieee80211_rx_result
>>> ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
>>
>> I believe that ieee80211_rx_h_decrypt() will drop the frames you're
>> looking at, and I do think the original patch is correct. If MMIC
>> validation was (and could be) done, then the frame must have been
>> decrypted properly.
>
> Just to avoid any confusion, with original patch you mean this one?
>
> rt2x00: Fix MMIC countermeasures.
> https://patchwork.kernel.org/patch/9875647/
>
> --
> Kalle Valo

[PATCH] rt2x00: Fix MMIC countermeasures.

[Michael Skeffington]

Mac80211 doesnt check MMIC failure until after falling through the
check for whether the packet is decrypted.  Therefore, this driver
never causes MMIC countermeasures to be initiated.

This change may (or may not) be relevant for rt2500usb, rt73usb, and
rt61pci as well but I don't have any of those devices to test with.

Signed-off-by: Michael Skeffington <m...@hellotwist.com>

---

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
index ee5276e233fa..ace91a2db756 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
@@ -136,10 +136,19 @@ void rt2800mmio_fill_rxdone(struct queue_entry *entry,
 */
rxdesc->flags |= RX_FLAG_MMIC_STRIPPED;

-   if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS)
+   if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) {
rxdesc->flags |= RX_FLAG_DECRYPTED;
-   else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC)
+} else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) {
+   /*
+* In order to check the Michael Mic, the
packet must have
+* been decrypted.  Mac80211 doesnt check the
MMIC failure
+* flag to initiate MMIC countermeasures if
the decoded flag
+* has not been set.
+*/
+   rxdesc->flags |= RX_FLAG_DECRYPTED;
+
rxdesc->flags |= RX_FLAG_MMIC_ERROR;
+}
}

if (rt2x00_get_field32(word, RXD_W3_MY_BSS))
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
index 685b8e0cd67d..7e5f397c37f9 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
@@ -697,11 +697,20 @@ static void rt2800usb_fill_rxdone(struct
queue_entry *entry,
 * stripped it from the frame. Signal this to mac80211.
 */
rxdesc->flags |= RX_FLAG_MMIC_STRIPPED;
-
-   if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS)
+
+   if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) {
+   rxdesc->flags |= RX_FLAG_DECRYPTED;
+} else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) {
+   /*
+* In order to check the Michael Mic, the
packet must have
+* been decrypted.  Mac80211 doesnt check the
MMIC failure
+* flag to initiate MMIC countermeasures if
the decoded flag
+* has not been set.
+*/
rxdesc->flags |= RX_FLAG_DECRYPTED;
-   else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC)
+
rxdesc->flags |= RX_FLAG_MMIC_ERROR;
+}
}

if (rt2x00_get_field32(word, RXD_W0_MY_BSS))

Re: [PATCH] mac80211: Validate michael MIC before attempting packet decode.

[Michael Skeffington]

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

Re: [PATCH] mac80211: Validate michael MIC before attempting packet decode.

[Michael Skeffington]

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 MalinenPGP id EFC895FA

[PATCH] mac80211: Validate michael MIC before attempting packet decode.

[Michael Skeffington]

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 <m...@hellotwist.com>

---

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 */
 #ifdef CONFIG_MAC80211_MESH
if (ieee80211_vif_is_mesh(>sdata->vif))

[no subject]

[Michael Skeffington]

subscribe linux-wireless