[PATCHv2] mac80211: dynamic wep

[PATCHv2] mac80211: dynamic wep

[Volker Braun]

This patch fixes my problems with "dynamic wep" (widely used in
universities), and I can now successfully associate and transfer
data using mac80211+iwl4965. Main changes: 
1) Allow privacy mismatch until associated 
2) Decrypt unicast frames with the per-STA key, not making any
   assumptions about it being key index 0.

Signed-off-by: Volker Braun <volker.braun@physik.hu-berlin.de>
---

Probably minor conflicts with the patch posted by Johannes Berg 
3 hours ago (in ieee80211_rx_h_check), sorry.


diff -pru linux+mac80211-9.0.3/include/linux/ieee80211.h linux-2.6.22.1/include/linux/ieee80211.h
--- linux+mac80211-9.0.3/include/linux/ieee80211.h	2007-08-06 16:28:48.000000000 -0400
+++ linux-2.6.22.1/include/linux/ieee80211.h	2007-08-07 13:35:18.000000000 -0400
@@ -357,7 +357,7 @@ struct ieee80211_cts {
 #define WLAN_CAPABILITY_IBSS		(1<<1)
 #define WLAN_CAPABILITY_CF_POLLABLE	(1<<2)
 #define WLAN_CAPABILITY_CF_POLL_REQUEST	(1<<3)
-#define WLAN_CAPABILITY_PRIVACY		(1<<4)
+#define WLAN_CAPABILITY_PRIVACY		(1<<4) /* Force WEP on data packets */
 #define WLAN_CAPABILITY_SHORT_PREAMBLE	(1<<5)
 #define WLAN_CAPABILITY_PBCC		(1<<6)
 #define WLAN_CAPABILITY_CHANNEL_AGILITY	(1<<7)
diff -pru linux+mac80211-9.0.3/net/mac80211/ieee80211.c linux-2.6.22.1/net/mac80211/ieee80211.c
--- linux+mac80211-9.0.3/net/mac80211/ieee80211.c	2007-08-06 16:28:48.000000000 -0400
+++ linux-2.6.22.1/net/mac80211/ieee80211.c	2007-08-09 20:03:07.000000000 -0400
@@ -3488,7 +3488,6 @@ static ieee80211_txrx_result
 ieee80211_rx_h_check(struct ieee80211_txrx_data *rx)
 {
 	struct ieee80211_hdr *hdr;
-	int always_sta_key;
 	hdr = (struct ieee80211_hdr *) rx->skb->data;
 
 	/* Drop duplicate 802.11 retransmissions (IEEE 802.11 Chap. 9.2.9) */
@@ -3556,29 +3555,23 @@ ieee80211_rx_h_check(struct ieee80211_tx
 		return TXRX_QUEUED;
 	}
 
-	if (rx->sdata->type == IEEE80211_IF_TYPE_STA)
-		always_sta_key = 0;
-	else
-		always_sta_key = 1;
+	if (rx->fc & IEEE80211_FCTL_PROTECTED && /* WEP */
+	    (rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV)) {
+
+		if (rx->skb->pkt_type == PACKET_HOST && 
+		    rx->sta && rx->sta->key) {
 
-	if (rx->sta && rx->sta->key && always_sta_key) {
-		rx->key = rx->sta->key;
-	} else {
-		if (rx->sta && rx->sta->key)
 			rx->key = rx->sta->key;
-		else
-			rx->key = rx->sdata->default_key;
 
-		if ((rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV) &&
-		    rx->fc & IEEE80211_FCTL_PROTECTED) {
+		} else { 
 			int keyidx = ieee80211_wep_get_keyidx(rx->skb);
+			if (keyidx < 0 || keyidx >= NUM_DEFAULT_KEYS)
+				return TXRX_DROP;
+		
+			rx->key = rx->sdata->keys[keyidx];
 
-			if (keyidx >= 0 && keyidx < NUM_DEFAULT_KEYS &&
-			    (!rx->sta || !rx->sta->key || keyidx > 0))
-				rx->key = rx->sdata->keys[keyidx];
-
-			if (!rx->key) {
-				if (!rx->u.rx.ra_match)
+			if (unlikely(!rx->key)) {
+				if (!rx->u.rx.ra_match) 
 					return TXRX_DROP;
 				printk(KERN_DEBUG "%s: RX WEP frame with "
 				       "unknown keyidx %d (A1=" MAC_FMT " A2="
@@ -3587,14 +3580,21 @@ ieee80211_rx_h_check(struct ieee80211_tx
 				       MAC_ARG(hdr->addr1),
 				       MAC_ARG(hdr->addr2),
 				       MAC_ARG(hdr->addr3));
-				if (!rx->local->apdev)
+				if (!rx->local->apdev) {
+					rx->local->dot11WEPUndecryptableCount++;
 					return TXRX_DROP;
+				}
 				ieee80211_rx_mgmt(
 					rx->local, rx->skb, rx->u.rx.status,
 					ieee80211_msg_wep_frame_unknown_key);
 				return TXRX_QUEUED;
 			}
 		}
+	} else { /* No WEP */
+		if (rx->sta && rx->sta->key)
+			rx->key = rx->sta->key;
+		else
+			rx->key = rx->sdata->default_key;
 	}
 
 	if (rx->fc & IEEE80211_FCTL_PROTECTED && rx->key && rx->u.rx.ra_match) {
diff -pru linux+mac80211-9.0.3/net/mac80211/ieee80211_ioctl.c linux-2.6.22.1/net/mac80211/ieee80211_ioctl.c
--- linux+mac80211-9.0.3/net/mac80211/ieee80211_ioctl.c	2007-08-06 16:28:48.000000000 -0400
+++ linux-2.6.22.1/net/mac80211/ieee80211_ioctl.c	2007-08-09 19:01:56.000000000 -0400
@@ -479,13 +479,14 @@ static int ieee80211_set_encryption(stru
 
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
-	if (is_broadcast_ether_addr(sta_addr)) {
+	if (idx < 0 || idx >= NUM_DEFAULT_KEYS) {
+		printk(KERN_DEBUG "%s: set_encrypt - invalid idx = %d\n",
+		       dev->name, idx);
+		return -EINVAL;
+	}
+
+	if (is_multicast_ether_addr(sta_addr)) {
 		sta = NULL;
-		if (idx >= NUM_DEFAULT_KEYS) {
-			printk(KERN_DEBUG "%s: set_encrypt - invalid idx=%d\n",
-			       dev->name, idx);
-			return -EINVAL;
-		}
 		key = sdata->keys[idx];
 
 		/* TODO: consider adding hwaccel support for these; at least
@@ -499,7 +500,7 @@ static int ieee80211_set_encryption(stru
 		 * being, this can be only set at compile time. */
 	} else {
 		set_tx_key = 0;
-		if (idx != 0) {
+		if (idx != 0 && alg != ALG_WEP) {
 			printk(KERN_DEBUG "%s: set_encrypt - non-zero idx for "
 			       "individual key\n", dev->name);
 			return -EINVAL;
diff -pru linux+mac80211-9.0.3/net/mac80211/ieee80211_sta.c linux-2.6.22.1/net/mac80211/ieee80211_sta.c
--- linux+mac80211-9.0.3/net/mac80211/ieee80211_sta.c	2007-08-06 16:28:48.000000000 -0400
+++ linux-2.6.22.1/net/mac80211/ieee80211_sta.c	2007-08-07 15:01:31.000000000 -0400
@@ -1131,10 +1131,11 @@ static int ieee80211_privacy_mismatch(st
 	bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
 	if (!bss)
 		return 0;
-
-	if (ieee80211_sta_wep_configured(dev) !=
-	    !!(bss->capability & WLAN_CAPABILITY_PRIVACY))
-		res = 1;
+	
+	if (ifsta->associated && ieee80211_sta_wep_configured(dev) != 
+	    !!(bss->capability & WLAN_CAPABILITY_PRIVACY)) {
+		res = 1;  /* associated and WEP encryption mismatch */
+	}
 
 	ieee80211_rx_bss_put(dev, bss);

Re: [PATCHv2] mac80211: dynamic wep

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Friday 10 August 2007 16:31, Volker Braun wrote:
> This patch fixes my problems with "dynamic wep" (widely used in
> universities), and I can now successfully associate and transfer
> data using mac80211+iwl4965. Main changes:
> 1) Allow privacy mismatch until associated
> 2) Decrypt unicast frames with the per-STA key, not making any
>    assumptions about it being key index 0.
>
> Signed-off-by: Volker Braun <volker.braun@physik.hu-berlin.de>
> ---
Hm, you somehow got a copy of this to me without actually putting me on the CC 
list.. you should probably use CC instead.

> -0400 @@ -357,7 +357,7 @@ struct ieee80211_cts {
>  #define WLAN_CAPABILITY_IBSS		(1<<1)
>  #define WLAN_CAPABILITY_CF_POLLABLE	(1<<2)
>  #define WLAN_CAPABILITY_CF_POLL_REQUEST	(1<<3)
> -#define WLAN_CAPABILITY_PRIVACY		(1<<4)
> +#define WLAN_CAPABILITY_PRIVACY		(1<<4) /* Force WEP on data packets */
Anyone interested in what this bit is can read the spec. Also, your comment is 
inaccurate, as this bit is not specific to WEP.

> +	if (rx->fc & IEEE80211_FCTL_PROTECTED && /* WEP */
Inaccurate and unnecessary comment.

> -			if (keyidx >= 0 && keyidx < NUM_DEFAULT_KEYS &&
> -			    (!rx->sta || !rx->sta->key || keyidx > 0))
> -				rx->key = rx->sdata->keys[keyidx];
So it looks like the keyidx > 0 check here is the source of the RX problems. I 
think we can store the keyidx of the individual key to compare against 
instead of assuming all individual keys have a keyidx of 0.

> -0400 @@ -1131,10 +1131,11 @@ static int ieee80211_privacy_mismatch(st
>  	bss = ieee80211_rx_bss_get(dev, ifsta->bssid);
>  	if (!bss)
>  		return 0;
> -
> -	if (ieee80211_sta_wep_configured(dev) !=
> -	    !!(bss->capability & WLAN_CAPABILITY_PRIVACY))
> -		res = 1;
> +
> +	if (ifsta->associated && ieee80211_sta_wep_configured(dev) !=
> +	    !!(bss->capability & WLAN_CAPABILITY_PRIVACY)) {
> +		res = 1;  /* associated and WEP encryption mismatch */
> +	}
Another unneeded comment. Don't add braces when it's not needed.

This part seems fairly evil. I suspect a better solution here is to allow 
unencrypted frames when the interface is dormant (netif_dormant()) and then 
we might be able to get rid of the specific check for WEP.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCHv2] mac80211: dynamic wep

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Mon, 2007-08-13 at 00:54 -0700, Michael Wu wrote:

> This part seems fairly evil. I suspect a better solution here is to allow 
> unencrypted frames when the interface is dormant (netif_dormant()) and then 
> we might be able to get rid of the specific check for WEP.

Might that address Tomas's concern with EAPOL too?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv2] mac80211: dynamic wep

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Monday 13 August 2007 01:54, Johannes Berg wrote:
> On Mon, 2007-08-13 at 00:54 -0700, Michael Wu wrote:
> > This part seems fairly evil. I suspect a better solution here is to allow
> > unencrypted frames when the interface is dormant (netif_dormant()) and
> > then we might be able to get rid of the specific check for WEP.
>
> Might that address Tomas's concern with EAPOL too?
>
Nope, this is just for the privacy mismatch check.

-Michael Wu

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCHv2] mac80211: dynamic wep

[Volker Braun]

I completely missed that privacy==confidentiality and includes all
encryption schemes, thanks. 

On Mon, 2007-08-13 at 00:54 -0700, Michael Wu wrote:
> > -			if (keyidx >= 0 && keyidx < NUM_DEFAULT_KEYS &&
> > -			    (!rx->sta || !rx->sta->key || keyidx > 0))
> > -				rx->key = rx->sdata->keys[keyidx];
> So it looks like the keyidx > 0 check here is the source of the RX problems. I 
> think we can store the keyidx of the individual key to compare against 
> instead of assuming all individual keys have a keyidx of 0.

Correct me if I'm wrong, but I'm reading the standard like this: For WEP
keys one can have up to 4 default keys (rx->sdata->keys[keyidx]), and a
varying number (but at least 10) per-STA keys (rx->sta->key). On
receiving a unicast packet, you use the per-STA key if available (and
discard keyidx). Only if there is no per-STA key, or the packet is
multicast, you use the keyidx-th default key. 

So you cannot rely on keyidx and you must have a check for unicast vs.
multicast somewhere.

Volker

Re: [PATCHv2] mac80211: dynamic wep

[Johannes Berg]

On Fri, 2007-08-10 at 23:31 +0000, Volker Braun wrote:

[...]

Your key setting change looks weird, you're right that key indexes 0
through 3 should all be valid for default keys but now you're changing
the code to actually look at the key index that is being set while
setting a unicast (key-mapping) key! In another mail you said:

> 2) The Cisco AP sets the unicast key to some index higher than
> 0. While outright weird, this is probably legal. However, mac80211
> does not allow this setting. I patched ieee80211_set_encryption to
> remove this invalid restriction of the key index.

Which seems to be a weird statement. How can the Cisco AP set the
unicast key index at all, that is on your side after all, no? In fact,
the standard has something to say against that:

     When a key-mapping key for an address pair is
     present, the WEP Key ID subfield in the MPDU shall
     be set to 0 on transmit and ignored on receive.

So there's a requirement to set the key index to 0. However, by the same
standard text, the AP mustn't care which key index is used for that
key-mapping key.

What happens when you don't patch the ioctls?

Also, you said:

> I see some similar (wrong) assumptions about keyidx==0 being the
> unicast key in ieee80211_rx_michael_mic_report. The statistics
> gathering is probably still broken in that regard.

In fact, that isn't wrong. Well, the key index must be ignored on
receive, but that doesn't change the fact that the sender did something
wrong if it's nonzero.

> +       if (rx->fc & IEEE80211_FCTL_PROTECTED && /* WEP */
> +           (rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV)) {
> +
> +               if (rx->skb->pkt_type == PACKET_HOST && 
> +                   rx->sta && rx->sta->key) {

That doesn't look too good. Now you're only loading up keys at all if
the hardware includes the IV.

> +       } else { /* No WEP */
> +               if (rx->sta && rx->sta->key)
> +                       rx->key = rx->sta->key;
> +               else
> +                       rx->key = rx->sdata->default_key;

That looks plain bogus. Why haven't you loaded the key in load_key?

Can you try this along with your other changes?

--- wireless-dev.orig/net/mac80211/rx.c	2007-08-15 02:51:32.430200043 +0200
+++ wireless-dev/net/mac80211/rx.c	2007-08-15 02:55:45.960200043 +0200
@@ -316,52 +316,62 @@ static ieee80211_txrx_result
 ieee80211_rx_h_load_key(struct ieee80211_txrx_data *rx)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
-	int always_sta_key;
-
-	if (rx->sdata->type == IEEE80211_IF_TYPE_STA)
-		always_sta_key = 0;
-	else
-		always_sta_key = 1;
+	int have_key_mapping = 0;
+	int keyidx;
+	int encrypted_frame = rx->fc & IEEE80211_FCTL_PROTECTED;
+
+	rx->key = NULL;
+
+	/*
+	 * We need to load the key even for unencryped frames because
+	 * later RX handlers will drop the packet based on that.
+	 */
 
-	if (rx->sta && rx->sta->key && always_sta_key) {
+	/*
+	 * "A key-mapping key is an unnamed key corresponding to a
+	 *  distinct transmitter address-receiver address <TA,RA>
+	 *  pair. Implementations shall use the key-mapping key
+	 *  if it is configured for a <TA,RA> pair."
+	 */
+	if (rx->sta && rx->sta->key && rx->skb->pkt_type == PACKET_HOST) {
 		rx->key = rx->sta->key;
-	} else {
-		if (rx->sta && rx->sta->key)
-			rx->key = rx->sta->key;
-		else
-			rx->key = rx->sdata->default_key;
-
-		if ((rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV) &&
-		    rx->fc & IEEE80211_FCTL_PROTECTED) {
-			int keyidx = ieee80211_wep_get_keyidx(rx->skb);
-
-			if (keyidx >= 0 && keyidx < NUM_DEFAULT_KEYS &&
-			    (!rx->sta || !rx->sta->key || keyidx > 0))
-				rx->key = rx->sdata->keys[keyidx];
-
-			if (!rx->key) {
-				if (!rx->u.rx.ra_match)
-					return TXRX_DROP;
-				if (net_ratelimit())
-					printk(KERN_DEBUG "%s: RX WEP frame "
-					       "with unknown keyidx %d "
-					       "(A1=" MAC_FMT
-					       " A2=" MAC_FMT
-					       " A3=" MAC_FMT ")\n",
-					       rx->dev->name, keyidx,
-					       MAC_ARG(hdr->addr1),
-					       MAC_ARG(hdr->addr2),
-					       MAC_ARG(hdr->addr3));
-				/*
-				 * TODO: notify userspace about this
-				 * via cfg/nl80211
-				 */
+		have_key_mapping = 1;
+	} else
+		rx->key = rx->sdata->default_key;
+
+	/*
+	 * "When key-mapping keys are used, the Key ID field value is ignored."
+	 */
+	/* should we really load a key for frames not addressed to us?? */
+	if ((rx->local->hw.flags & IEEE80211_HW_WEP_INCLUDE_IV) &&
+	    encrypted_frame && !have_key_mapping) {
+		keyidx = ieee80211_wep_get_keyidx(rx->skb);
+
+		if (keyidx >= 0 && keyidx < NUM_DEFAULT_KEYS)
+			rx->key = rx->sdata->keys[keyidx];
+
+		if (!rx->key) {
+			if (!rx->u.rx.ra_match)
 				return TXRX_DROP;
-			}
+			if (net_ratelimit())
+				printk(KERN_DEBUG "%s: RX WEP frame "
+				       "with unknown keyidx %d "
+				       "(A1=" MAC_FMT
+				       " A2=" MAC_FMT
+				       " A3=" MAC_FMT ")\n",
+				       rx->dev->name, keyidx,
+				       MAC_ARG(hdr->addr1),
+				       MAC_ARG(hdr->addr2),
+				       MAC_ARG(hdr->addr3));
+			/*
+			 * TODO: notify userspace about this
+			 * via cfg/nl80211
+			 */
+			return TXRX_DROP;
 		}
 	}
 
-	if (rx->fc & IEEE80211_FCTL_PROTECTED && rx->key && rx->u.rx.ra_match) {
+	if (encrypted_frame && rx->key && rx->u.rx.ra_match) {
 		rx->key->tx_rx_count++;
 		if (unlikely(rx->local->key_tx_rx_threshold &&
 			     rx->key->tx_rx_count >

Re: [PATCHv2] mac80211: dynamic wep

[Volker Braun]

On Wed, 2007-08-15 at 03:47 +0200, Johannes Berg wrote:
>      When a key-mapping key for an address pair is
>      present, the WEP Key ID subfield in the MPDU shall
>      be set to 0 on transmit and ignored on receive.

I'm pretty sure this is from the TKIP part. There, the unicast key index
is indeed always 0. However, for WEP (and WEP only) this is not
regulated.
 
> > I see some similar (wrong) assumptions about keyidx==0 being the
> > unicast key in ieee80211_rx_michael_mic_report. [...]
> In fact, that isn't wrong.

Yes, I was wrong here: Because Michael MIC only matters for TKIP it is
safe to assume that keyidx==0.

As for the ieee80211_rx_h_load_key, that doesn't exist in intel's
mac80211-9.0.4. I'll try wireless-dev again, but iwl4965 tends to hang
my computer a lot with other versions of mac80211.