When using WPA security, the station and thus the required key is
identified by its mac address when packets are received. So a
station usually cannot spoof its source mac address.
But when a station sends an A-MSDU frame, port control and crypto
is done using the outer mac address, while the packets delivered
and forwarded use the inner mac address.
IEEE 802.11-2012 mandates that the outer source mac address should
match the inner source address (section 8.3.2.2). For the
destination mac address, matching is not required (section 10.23.15).
Signed-off-by: Michael Braun <[email protected]>
--
v2: fix typo inversed source mac address check
---
net/wireless/util.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index b7d1592..86ca5d2 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -747,13 +747,13 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
u16 ethertype;
u8 *payload;
int offset = 0, remaining, err;
- struct ethhdr eth;
+ struct ethhdr eth, eth_80211;
bool reuse_frag = skb->head_frag && !skb_has_frag_list(skb);
bool reuse_skb = false;
bool last = false;
if (has_80211_header) {
- err = __ieee80211_data_to_8023(skb, ð, addr, iftype);
+ err = __ieee80211_data_to_8023(skb, ð_80211, addr, iftype);
if (err)
goto out;
}
@@ -768,6 +768,13 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
subframe_len = sizeof(struct ethhdr) + len;
padding = (4 - subframe_len) & 0x3;
+ if (unlikely(has_80211_header &&
+ (iftype == NL80211_IFTYPE_AP ||
+ iftype == NL80211_IFTYPE_AP_VLAN) &&
+ !ether_addr_equal(eth_80211.h_source, eth.h_source)
+ ))
+ goto purge;
+
/* the last MSDU has no padding */
remaining = skb->len - offset;
if (subframe_len > remaining)
--
2.1.4
Huh. I know this bug, I thought we fixed it a long time ago. Oops.
> - struct ethhdr eth;
> + struct ethhdr eth, eth_80211;
> bool reuse_frag = skb->head_frag && !skb_has_frag_list(skb);
> bool reuse_skb = false;
> bool last = false;
>
> if (has_80211_header) {
> - err = __ieee80211_data_to_8023(skb, ð, addr,
> iftype);
> + err = __ieee80211_data_to_8023(skb, ð_80211,
> addr, iftype);
> if (err)
> goto out;
> }
This leaves "eth_80211" uninitialized if has_80211_header is false.
> @@ -768,6 +768,13 @@ void ieee80211_amsdu_to_8023s(struct sk_buff
> *skb, struct sk_buff_head *list,
> subframe_len = sizeof(struct ethhdr) + len;
> padding = (4 - subframe_len) & 0x3;
>
> + if (unlikely(has_80211_header &&
> + (iftype == NL80211_IFTYPE_AP ||
> + iftype == NL80211_IFTYPE_AP_VLAN) &&
> + !ether_addr_equal(eth_80211.h_source,
> eth.h_source)
> + ))
> + goto purge;
And this then compares against uninitialized data, so this won't work.
I'd suggest removing the "has_80211_header" argument entirely, and
replacing it with a "const u8 *sa" argument, but that complicates
mac80211 significantly since all the checks
in __ieee80211_data_to_8023() would have to be replicated.
Maybe we can still do this, and say that it must be NULL when an 802.11
header is present, and be the SA when not. However, mwifiex doesn't
seem to be able to easily provide the SA (at least I don't see it,
perhaps it can), so that we'd have to allow some kind of ERR_PTR() or
something for that special case ... Actually it'd be better to just fix
mwifiex though :)
The staging driver using this (rtl8712) can easily provide the SA,
afaict.
johannes
On Tue, 2016-09-27 at 10:53 +0200, michael-dev wrote:
> Am 27.09.2016 10:01, schrieb Johannes Berg:
> >
> > ...
> >
> > This leaves "eth_80211" uninitialized if has_80211_header is false.
> >
> > >
> > > @@ -768,6 +768,13 @@ void ieee80211_amsdu_to_8023s(struct sk_buff
> > > *skb, struct sk_buff_head *list,
> > > subframe_len = sizeof(struct ethhdr) + len;
> > > padding = (4 - subframe_len) & 0x3;
> > >
> > > + if (unlikely(has_80211_header &&
> > > + (iftype == NL80211_IFTYPE_AP ||
> > > + iftype == NL80211_IFTYPE_AP_VLAN)
> > > &&
> > > > > > + !ether_addr_equal(eth_80211.h_source,
> > > eth.h_source)
> > > + ))
> > > + goto purge;
> >
> > And this then compares against uninitialized data, so this won't
> > work.
>
> but it only compares against eth_80211 if has_80211_header is true
> due to order of evaluation, which in turn implies eth_80211 is
> initialized, right?
>
Oh, right, I missed that, sorry.
Nevertheless, it seems it would be better to allow the other users (not
mac80211) that have has_80211_header=false to still have the check?
johannes
Am 27.09.2016 10:01, schrieb Johannes Berg:
> ...
>=20
> This leaves "eth_80211" uninitialized if has_80211_header is false.
>=20
>> @@ -768,6 +768,13 @@ void ieee80211_amsdu_to_8023s(struct sk_buff
>> *skb, struct sk_buff_head *list,
>> =C2=A0 subframe_len =3D sizeof(struct ethhdr) + len;
>> =C2=A0 padding =3D (4 - subframe_len) & 0x3;
>> =C2=A0
>> + if (unlikely(has_80211_header &&
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(iftype =3D=3D NL80211_IFTYPE_AP ||
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0iftype =3D=3D NL80211_IFTYPE_AP_=
VLAN) &&
>> + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0!ether_addr_equal(eth_80211.h_source,
>> eth.h_source)
>> + =C2=A0=C2=A0=C2=A0))
>> + goto purge;
>=20
> And this then compares against uninitialized data, so this won't work.
but it only compares against eth_80211 if has_80211_header is true due=20
to order of evaluation, which in turn implies eth_80211 is initialized,=20
right?
michael