mac80211: validate key before MIC verify

RFC In review Stanislaw Gruszka 2 participants 3 messages Sep 21, 2012 mac80211

Share & tools

Stanislaw Gruszka

author

2012-09-21 12:42 UTC #0 lore raw

RFC 1/1 mac80211: validate key before MIC verify

I have strange crash on rt61pci hardware when switching off radio by rfkill switch:
https://bugzilla.redhat.com/attachment.cgi?id=615362

After debugging the issue, I figured out problem happens because key->u.ccmp.tfm of group key get corrupted. Corruption happen in

ieee80211_rx_h_michael_mic_verify():

        /* update IV in key information to be able to detect replays */
        rx->key->u.tkip.rx[rx->security_idx].iv32 = rx->tkip_iv32;
        rx->key->u.tkip.rx[rx->security_idx].iv16 = rx->tkip_iv16;

because rt61pci always set RX_FLAG_MMIC_STRIPPED and RX_FLAG_IV_STRIPPED flags.

This problem was introduces in:

816c04f mac80211: consolidate MIC failure report handling

which already has fixes of invalid usage of rx->key pointer:

1140afa mac80211: fix rx->key NULL pointer dereference in promiscuous mode a66b98d mac80211: fix rx->key NULL dereference during mic failure

This patch fix the problem by checking for key pointer is valid and if key type is TKIP, before doing any other MIC verification.

Cc [email protected]

Signed-off-by Stanislaw Gruszka <[email protected]>

---
I did not test patch ...

net/mac80211/wpa.c +11 ~~-10~~

index bdb53ab..6f800f7 100644
@@ -97,6 +97,14 @@ ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
 		return RX_CONTINUE;
 
 	/*
+	 * Some hardware seems to generate Michael MIC failure reports; even
+	 * though, the frame was not encrypted with TKIP and therefore has no
+	 * MIC. Ignore the flag them to avoid triggering countermeasures.
+	 */
+	if (!rx->key || rx->key->conf.cipher != WLAN_CIPHER_SUITE_TKIP)
+		return RX_CONTINUE;
+
+	/*
 	 * No way to verify the MIC if the hardware stripped it or
 	 * the IV with the key index. In this case we have solely rely
 	 * on the driver to set RX_FLAG_MMIC_ERROR in the event of a

@@ -106,19 +114,13 @@ ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
 		if (status->flag & RX_FLAG_MMIC_ERROR)
 			goto mic_fail;
 
-		if (!(status->flag & RX_FLAG_IV_STRIPPED) && rx->key)
+		if (!(status->flag & RX_FLAG_IV_STRIPPED))
 			goto update_iv;
 
 		return RX_CONTINUE;
 	}
 
-	/*
-	 * Some hardware seems to generate Michael MIC failure reports; even
-	 * though, the frame was not encrypted with TKIP and therefore has no
-	 * MIC. Ignore the flag them to avoid triggering countermeasures.
-	 */
-	if (!rx->key || rx->key->conf.cipher != WLAN_CIPHER_SUITE_TKIP ||
-	    !(status->flag & RX_FLAG_DECRYPTED))
+	if (!(status->flag & RX_FLAG_DECRYPTED))
 		return RX_CONTINUE;
 
 	if (rx->sdata->vif.type == NL80211_IFTYPE_AP && rx->key->conf.keyidx) {

@@ -165,8 +167,7 @@ mic_fail:
 	 * a driver that supports HW encryption. Send up the key idx only if
 	 * the key is set.
 	 */
-	mac80211_ev_michael_mic_failure(rx->sdata,
-					rx->key ? rx->key->conf.keyidx : -1,
+	mac80211_ev_michael_mic_failure(rx->sdata, rx->key->conf.keyidx,
 					(void *) skb->data, NULL, GFP_ATOMIC);
 	return RX_DROP_UNUSABLE;
 }

Johannes Berg

maintainer

2012-09-21 12:59 UTC #1 lore raw

Quoted 7 lines · Stanislaw Gruszka

net/mac80211/wpa.c +1 -0

@@ -97,6 +97,14 @@ ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
 		return RX_CONTINUE;
 
 	/*
+	 * Some hardware seems to generate Michael MIC failure reports; even

net/mac80211/wpa.c ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) · line 110

Quoted 10 lines

net/mac80211/wpa.c +7 -0

+	 * though, the frame was not encrypted with TKIP and therefore has no
+	 * MIC. Ignore the flag them to avoid triggering countermeasures.
+	 */
+	if (!rx->key || rx->key->conf.cipher != WLAN_CIPHER_SUITE_TKIP)
+		return RX_CONTINUE;
+
+	/*
 	 * No way to verify the MIC if the hardware stripped it or
 	 * the IV with the key index. In this case we have solely rely
 	 * on the driver to set RX_FLAG_MMIC_ERROR in the event of a

Hm, this doesn't seem _quite_ right, but I'm not sure: it seems that previously it was possible that we don't have a key pointer but the driver set all of RX_FLAG_MMIC_STRIPPED, RX_FLAG_IV_STRIPPED and RX_FLAG_MMIC_ERROR, in which case after your change the frame will be accepted rather than rejected.

johannes

Stanislaw Gruszka

author

2012-09-21 13:08 UTC #2 lore raw

Quoted 17 lines · Johannes Berg

Quoted 15 lines · Stanislaw Gruszka

net/mac80211/wpa.c +8 -0

@@ -97,6 +97,14 @@ ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
 		return RX_CONTINUE;
 
 	/*
+	 * Some hardware seems to generate Michael MIC failure reports; even
+	 * though, the frame was not encrypted with TKIP and therefore has no
+	 * MIC. Ignore the flag them to avoid triggering countermeasures.
+	 */
+	if (!rx->key || rx->key->conf.cipher != WLAN_CIPHER_SUITE_TKIP)
+		return RX_CONTINUE;
+
+	/*
 	 * No way to verify the MIC if the hardware stripped it or

Quoted 8 lines

net/mac80211/wpa.c ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) · line 110
Quoted 2 lines
net/mac80211/wpa.c +0 -0
 	 * the IV with the key index. In this case we have solely rely
 	 * on the driver to set RX_FLAG_MMIC_ERROR in the event of a
Hm, this doesn't seem _quite_ right, but I'm not sure: it seems that previously it was possible that we don't have a key pointer but the driver set all of RX_FLAG_MMIC_STRIPPED, RX_FLAG_IV_STRIPPED and RX_FLAG_MMIC_ERROR, in which case after your change the frame will be accepted rather than rejected.

I wanted to cleanup stuff, but yeah, that seem to be wrong. I guess I can just add check before rx->key->u.tkip.rx usage to fix the problem. Eventually fix flags setting in driver.

Stanislaw