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]>
---
drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 3 ++-
.../net/wireless/marvell/mwifiex/11n_rxreorder.c | 18 +++++++-------
drivers/staging/rtl8723au/core/rtw_recv.c | 2 +-
include/net/cfg80211.h | 9 +++----
net/mac80211/rx.c | 11 +++++++--
net/wireless/util.c | 28 +++++++++-------------
6 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
index 4fdc3da..416d060 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
@@ -1436,7 +1436,8 @@ static void iwl_mvm_report_wakeup_reasons(struct iwl_mvm *mvm,
memcpy(skb_put(pkt, pktsize), pktdata, pktsize);
- if (ieee80211_data_to_8023(pkt, vif->addr, vif->type))
+ if (ieee80211_data_to_8023(pkt, NULL, vif->addr,
+ vif->type))
goto report;
wakeup.packet = pkt->data;
wakeup.packet_present_len = pkt->len;
diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
index a74cc43..f4469d7 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
@@ -30,7 +30,8 @@
* layer.
*/
static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ const u8 *ta)
{
struct rxpd *local_rx_pd = (struct rxpd *)(skb->data);
int ret;
@@ -45,7 +46,7 @@ static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
skb_trim(skb, le16_to_cpu(local_rx_pd->rx_pkt_length));
ieee80211_amsdu_to_8023s(skb, &list, priv->curr_addr,
- priv->wdev.iftype, 0, false);
+ priv->wdev.iftype, 0, ta);
while (!skb_queue_empty(&list)) {
struct rx_packet_hdr *rx_hdr;
@@ -76,9 +77,10 @@ static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
/* This function will process the rx packet and forward it to kernel/upper
* layer.
*/
-static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload)
+static int mwifiex_11n_dispatch_pkt(struct mwifiex_private *priv, void *payload,
+ const u8 *ta)
{
- int ret = mwifiex_11n_dispatch_amsdu_pkt(priv, payload);
+ int ret = mwifiex_11n_dispatch_amsdu_pkt(priv, payload, ta);
if (!ret)
return 0;
@@ -119,7 +121,7 @@ mwifiex_11n_dispatch_pkt_until_start_win(struct mwifiex_private *priv,
}
spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
if (rx_tmp_ptr)
- mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
+ mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr, tbl->ta);
}
spin_lock_irqsave(&priv->rx_pkt_lock, flags);
@@ -161,7 +163,7 @@ mwifiex_11n_scan_and_dispatch(struct mwifiex_private *priv,
rx_tmp_ptr = tbl->rx_reorder_ptr[i];
tbl->rx_reorder_ptr[i] = NULL;
spin_unlock_irqrestore(&priv->rx_pkt_lock, flags);
- mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
+ mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr, tbl->ta);
}
spin_lock_irqsave(&priv->rx_pkt_lock, flags);
@@ -568,12 +570,12 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private *priv,
tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
if (!tbl) {
if (pkt_type != PKT_TYPE_BAR)
- mwifiex_11n_dispatch_pkt(priv, payload);
+ mwifiex_11n_dispatch_pkt(priv, payload, ta);
return ret;
}
if ((pkt_type == PKT_TYPE_AMSDU) && !tbl->amsdu) {
- mwifiex_11n_dispatch_pkt(priv, payload);
+ mwifiex_11n_dispatch_pkt(priv, payload, ta);
return ret;
}
diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
index 150dabc..38ba7dd 100644
--- a/drivers/staging/rtl8723au/core/rtw_recv.c
+++ b/drivers/staging/rtl8723au/core/rtw_recv.c
@@ -1687,7 +1687,7 @@ int amsdu_to_msdu(struct rtw_adapter *padapter, struct recv_frame *prframe)
skb_pull(skb, prframe->attrib.hdrlen);
__skb_queue_head_init(&skb_list);
- ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, false);
+ ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, pattrib->ta);
while (!skb_queue_empty(&skb_list)) {
sub_skb = __skb_dequeue(&skb_list);
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index beb7610..d768fcd 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3905,12 +3905,13 @@ unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr);
/**
* ieee80211_data_to_8023 - convert an 802.11 data frame to 802.3
* @skb: the 802.11 data frame
+ * @ehdr: (out) buffer for source/destination address (optional)
* @addr: the device MAC address
* @iftype: the virtual interface type
* Return: 0 on success. Non-zero on error.
*/
-int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
- enum nl80211_iftype iftype);
+int ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
+ const u8 *addr, enum nl80211_iftype iftype);
/**
* ieee80211_data_from_8023 - convert an 802.3 frame to 802.11
@@ -3938,12 +3939,12 @@ int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr,
* @addr: The device MAC address.
* @iftype: The device interface type.
* @extra_headroom: The hardware extra headroom for SKBs in the @list.
- * @has_80211_header: Set it true if SKB is with IEEE 802.11 header.
+ * @ta: transmitter address (or NULL)
*/
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
const unsigned int extra_headroom,
- bool has_80211_header);
+ const u8 *ta);
/**
* cfg80211_classify8021d - determine the 802.1p/1d tag for a data frame
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 9dce3b1..fbf99b8 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2090,7 +2090,8 @@ __ieee80211_data_to_8023(struct ieee80211_rx_data *rx, bool *port_control)
sdata->vif.type == NL80211_IFTYPE_AP_VLAN && sdata->u.vlan.sta)
return -1;
- ret = ieee80211_data_to_8023(rx->skb, sdata->vif.addr, sdata->vif.type);
+ ret = ieee80211_data_to_8023(rx->skb, NULL, sdata->vif.addr,
+ sdata->vif.type);
if (ret < 0)
return ret;
@@ -2243,6 +2244,7 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
__le16 fc = hdr->frame_control;
struct sk_buff_head frame_list;
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
+ struct ethhdr eth_80211;
if (unlikely(!ieee80211_is_data(fc)))
return RX_CONTINUE;
@@ -2268,9 +2270,14 @@ ieee80211_rx_h_amsdu(struct ieee80211_rx_data *rx)
skb->dev = dev;
__skb_queue_head_init(&frame_list);
+ if (ieee80211_data_to_8023(skb, ð_80211, dev->dev_addr,
+ rx->sdata->vif.type) < 0)
+ return RX_DROP_UNUSABLE;
+
ieee80211_amsdu_to_8023s(skb, &frame_list, dev->dev_addr,
rx->sdata->vif.type,
- rx->local->hw.extra_tx_headroom, true);
+ rx->local->hw.extra_tx_headroom,
+ eth_80211.h_source);
while (!skb_queue_empty(&frame_list)) {
rx->skb = __skb_dequeue(&frame_list);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index b7d1592..5622740 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -414,8 +414,8 @@ unsigned int ieee80211_get_mesh_hdrlen(struct ieee80211s_hdr *meshhdr)
}
EXPORT_SYMBOL(ieee80211_get_mesh_hdrlen);
-static int __ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
- const u8 *addr, enum nl80211_iftype iftype)
+int ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
+ const u8 *addr, enum nl80211_iftype iftype)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
struct {
@@ -519,12 +519,6 @@ static int __ieee80211_data_to_8023(struct sk_buff *skb, struct ethhdr *ehdr,
return 0;
}
-
-int ieee80211_data_to_8023(struct sk_buff *skb, const u8 *addr,
- enum nl80211_iftype iftype)
-{
- return __ieee80211_data_to_8023(skb, NULL, addr, iftype);
-}
EXPORT_SYMBOL(ieee80211_data_to_8023);
int ieee80211_data_from_8023(struct sk_buff *skb, const u8 *addr,
@@ -740,24 +734,18 @@ __ieee80211_amsdu_copy(struct sk_buff *skb, unsigned int hlen,
void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
const u8 *addr, enum nl80211_iftype iftype,
const unsigned int extra_headroom,
- bool has_80211_header)
+ const u8 *ta)
{
unsigned int hlen = ALIGN(extra_headroom, 4);
struct sk_buff *frame = NULL;
u16 ethertype;
u8 *payload;
- int offset = 0, remaining, err;
+ int offset = 0, remaining;
struct ethhdr eth;
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);
- if (err)
- goto out;
- }
-
while (!last) {
unsigned int subframe_len;
int len;
@@ -768,6 +756,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(ta &&
+ (iftype == NL80211_IFTYPE_AP ||
+ iftype == NL80211_IFTYPE_AP_VLAN) &&
+ !ether_addr_equal(ta, eth.h_source)
+ ))
+ goto purge;
+
/* the last MSDU has no padding */
remaining = skb->len - offset;
if (subframe_len > remaining)
@@ -813,7 +808,6 @@ void ieee80211_amsdu_to_8023s(struct sk_buff *skb, struct sk_buff_head *list,
purge:
__skb_queue_purge(list);
- out:
dev_kfree_skb(skb);
}
EXPORT_SYMBOL(ieee80211_amsdu_to_8023s);
--
2.1.4
Michael Braun <[email protected]> writes:
> 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]>
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 3 ++-
> .../net/wireless/marvell/mwifiex/11n_rxreorder.c | 18 +++++++-------
> drivers/staging/rtl8723au/core/rtw_recv.c | 2 +-
> include/net/cfg80211.h | 9 +++----
> net/mac80211/rx.c | 11 +++++++--
> net/wireless/util.c | 28 +++++++++-------------
> 6 files changed, 38 insertions(+), 33 deletions(-)
I understand the intentions of this patch are all good, but you need to
not post patches that include both staging and mainline drivers at the
same time. In general make it a patchset and do one patch per driver.
Ideally split up changes to generic code into their own patches too.
Last drivers/staging/rtl8723au is gone - so your patch is going to fail
to apply anyway.
Jes
On Wed, 2016-09-28 at 11:19 -0400, Jes Sorensen wrote:
>
> I understand the intentions of this patch are all good, but you need
> to not post patches that include both staging and mainline drivers at
> the same time. In general make it a patchset and do one patch per
> driver.
>
> Ideally split up changes to generic code into their own patches too.
No Jes, you're wrong this time - this is changing internal API so it
does have to touch all users thereof.
> Last drivers/staging/rtl8723au is gone - so your patch is going to
> fail to apply anyway.
It's there in my tree, for now, so I guess I'll see if it's still there
when I take this in :)
johannes
On Wed, 2016-09-28 at 13:22 -0400, Jes Sorensen wrote:
> I'll still argue this could be handled better through gradual
> migration rather than one large patch that touches too many places,
I'd agree if it was actually a large patch, but if at all, I have to
coordinate only with Kalle, which won't be difficult.
Comparing that to the alternative - introducing new API, changing all
the users in separate patches, and finally removing the old API - and
the nightmare of coordinating that going in through various different
trees, I'll take this any day. :)
johannes
Johannes Berg <[email protected]> writes:
> On Wed, 2016-09-28 at 11:39 -0400, Jes Sorensen wrote:
>
>> > No Jes, you're wrong this time - this is changing internal API so
>> > it does have to touch all users thereof.
>>
>> Even in this case, change the individual components in individual
>> patches and post them as a set.
>
> No, still wrong - it has to be committed as a single patch so it
> doesn't break bisect.
>
>> Changes to staging needs to go in via staging, and rtl8723au is gone
>> from the staging tree.
>>
>
> I've previously taken API change patches that touch staging, if people
> feel so inclined, and I don't think Greg will mind. I'm going to keep
> doing that unless Dave tells me he won't pull from me when I do it :)
I'll still argue this could be handled better through gradual migration
rather than one large patch that touches too many places, but if you are
willing to take it, I am not going to fight you over it :)
Jes
On Wed, 2016-09-28 at 11:39 -0400, Jes Sorensen wrote:
> > No Jes, you're wrong this time - this is changing internal API so
> > it does have to touch all users thereof.
>
> Even in this case, change the individual components in individual
> patches and post them as a set.
No, still wrong - it has to be committed as a single patch so it
doesn't break bisect.
> Changes to staging needs to go in via staging, and rtl8723au is gone
> from the staging tree.
>
I've previously taken API change patches that touch staging, if people
feel so inclined, and I don't think Greg will mind. I'm going to keep
doing that unless Dave tells me he won't pull from me when I do it :)
johannes
Johannes Berg <[email protected]> writes:
> On Wed, 2016-09-28 at 11:19 -0400, Jes Sorensen wrote:
>>
>> I understand the intentions of this patch are all good, but you need
>> to not post patches that include both staging and mainline drivers at
>> the same time. In general make it a patchset and do one patch per
>> driver.
>>
>> Ideally split up changes to generic code into their own patches too.
>
> No Jes, you're wrong this time - this is changing internal API so it
> does have to touch all users thereof.
Even in this case, change the individual components in individual
patches and post them as a set.
>> Last drivers/staging/rtl8723au is gone - so your patch is going to
>> fail to apply anyway.
>
> It's there in my tree, for now, so I guess I'll see if it's still there
> when I take this in :)
Changes to staging needs to go in via staging, and rtl8723au is gone
from the staging tree.
Cheers,
Jes
A few more things:
First of all - there's nothing specific to "AP interfaces", which you
say in the subject, as far as I can tell? That should be removed?
On Wed, 2016-09-28 at 17:14 +0200, Michael Braun wrote:
> 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).
I think this is wrong. As we do not support DMS (yet), we should adhere
to 8.3.2.2 and only accept matching TA/SA and DA/RA.
And even when we add DMS, we should also restrict it to multicast
addresses, but that's future work anyway.
> > - if (ieee80211_data_to_8023(pkt, vif->addr, vif->type))
> > + if (ieee80211_data_to_8023(pkt, NULL, vif->addr,
> > + vif->type))
indentation is bad here - consider running checkpatch
> static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
> > - struct sk_buff *skb)
> > + struct sk_buff *skb,
> > + const u8 *ta)
[... more mwifiex changes ...]
It's great that you're doing this, but I think this patch should only
carry the bare minimum change for mwifiex to keep it compiling, with a
follow-up patch that actually implements the correct checks. That way,
should any issues arise, it's easier to determine where the problem is
and to fix/revert it.
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 150dabc..38ba7dd 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -1687,7 +1687,7 @@ int amsdu_to_msdu(struct rtw_adapter *padapter, struct recv_frame *prframe)
> > skb_pull(skb, prframe->attrib.hdrlen);
> > __skb_queue_head_init(&skb_list);
>
> > - ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, false);
> > + ieee80211_amsdu_to_8023s(skb, &skb_list, NULL, 0, 0, pattrib->ta);
A bit debatable here, but I'd actually also prefer you add NULL here
first and then submit a separate patch that puts the right thing.
Not that it actually matters, since this driver has been removed
already... since you have to resubmit anyway though, I'd prefer you
just put NULL, and then not worry about it from there on.
> > - if (has_80211_header) {
> > - err = __ieee80211_data_to_8023(skb, ð, addr, iftype);
> > - if (err)
> > - goto out;
> > - }
Good approach to split that :)
> > + if (unlikely(ta &&
> > + (iftype == NL80211_IFTYPE_AP ||
> > + iftype == NL80211_IFTYPE_AP_VLAN) &&
> > + !ether_addr_equal(ta, eth.h_source)
> > + ))
> > + goto purge;
Coding style here is very odd. All those closing parens should be on
the line before.
Thanks,
johannes
On Tue, 2016-10-04 at 23:12 +0200, M. Braun wrote:
> >
> > Obviously, now that I think about it, your patch also would break
> > client mode since it would refuse to accept any A-MSDU with SA !=
> > TA, which is highly unlikely in most cases, since traffic doesn't
> > usually originate from the AP.
>
> I still don't think my patch would break anything here, as it does
> only filter on AP/AP_VLAN interfaces so stations are not affected at
> all.
Yes, that's true.
> I agree that asking for more cases to be filtered seems natural. But
> is fixing all possible A-MSDU address mismatch problems required to
> fix the one I currently care about most?
Maybe not. But we're changing the API here, and doing that just a
single time would be easier, I think.
> > We verify today that only multicast frames can be encrypted with
> > the GTK, but that applies to the outer header, so we're susceptible
> > to a variant of the hole-196 attack, afaict?
>
> That exploited that an unicast arp reply can be injected using GTK,
> thus bypassing AP filtering, right?
No, more generally, that exploits that you often can send unicast L3
(e.g. IP) frames in multicast L2 frames, so that even checking that GTK
is used only for multicast L2 frames.
This case is different, not really quite a variant thereof maybe.
But you could possibly send a unicast inner-L2 frame in a multicast
outer-L2 frame, so we should discard multicast A-MSDUs I guess? But
that actually seems like a separate problem.
> To counter, it would suffice to required multicast A-MSDU frames to
> only carry multicast (inner) MSDUs?
Or that, but I don't even think that multicast A-MSDU is allowed, see
9.11.
> I don't see how this could be inversed, that is an attack exploiting
> multicast encrypted with PTK.
Yeah, you're right, anyone who is in possession of a PTK better also be
able to send multicast frames *anyway* somehow.
> > Overall, it seems to me we should do the following:
>
> I'm wondering if all this filtering is actually required or if it
> filters out more than required?
It seems it should be required?
> > * pass DA == RA for client mode (not when 4-addr)
>
> If this implies that encapsulating multicast as unicast A-MSDU is not
> permitted, then why? This would break multicast to unicast using A-
> MSDU frames, which currently works with most clients for me.
But arguably it shouldn't work unless DMS was negotiated? However, I
also don't see an attack vector right now, since, as I wrote above,
peers in possession of a PTK should have a way to send multicast frames
anyway.
> > * pass both on TDLS links
>
> I don't know why TDLS links should carry multicast at all, so this
> seems reasonable to me.
Yeah, they can't really carry multicast traffic.
> > * pass both for IBSS mode (I think)
>
> Why is multicast to unicast not permitted within IBSS?
Well, you're thinking of this only from a multicast angle and I was
thinking only from a "fake DA/SA" angle.
Combining the two, I suppose we could accept a multicast DA even when a
DA match was requested.
Obviously, when no DA match is requested, like in the AP (or VLAN)
case, this question is irrelevant. When a DA match *is* requested, i.e.
cases where we have real multicast over the air (client/IBSS/TDLS),
arguably inner multicast should *not* be accepted by spec, since the
multicast DA cannot map to a unicast RA.
I'll send out a patch or two to fix the most glaring issues here, and
we can continue discussing the inner-L2 filtering.
johannes
Am 30.09.2016 um 12:01 schrieb Johannes Berg:
> A few more things:
>
> First of all - there's nothing specific to "AP interfaces", which you
> say in the subject, as far as I can tell? That should be removed?
>> if (unlikely(ta &&
>>+ (iftype == NL80211_IFTYPE_AP ||
>>+ iftype == NL80211_IFTYPE_AP_VLAN) &&
>>+ !ether_addr_equal(ta, eth.h_source)
>>+ ))
>>+ goto purge;
So the A-MSDU packets are only dropped if received by an interface in AP
or AP_VLAN mode, not on client side, as my original issue was about
arp/ip filters being circumvented on AP side.
>> 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).
>
> I think this is wrong. As we do not support DMS (yet), we should adhere
> to 8.3.2.2 and only accept matching TA/SA and DA/RA.
IEEE 802.11-2012 8.3.2.2 contains the note "NOTE—It is possible to have
different DA and SA parameter values in A-MSDU subframe headers of the
same A-MSDU as long as they all map to the same Address 1 and Address 2
parameter values."
I conclude that embedding multicast in unicast A-MSDU frames is
generally allowed, because "mapping" does not mean "be identical".
Regards,
M. Braun
On Tue, 2016-10-04 at 10:29 +0200, Johannes Berg wrote:
> > IEEE 802.11-2012 8.3.2.2 contains the note "NOTE—It is possible to
> > have different DA and SA parameter values in A-MSDU subframe
> > headers of the same A-MSDU as long as they all map to the same
> > Address 1 and Address 2 parameter values."
> >
> > I conclude that embedding multicast in unicast A-MSDU frames is
> > generally allowed, because "mapping" does not mean "be identical".
>
> Yeah, I saw this. It's not clear to me that they intended this
> wording to be about multicast though. I'm not really sure what they
> had in mind here, but there's an exception for multicast for DMS,
> which would seem pointless if they had intended this "mapping" to be
> about multicast.
>
> Then again, I don't know of any "address mapping" service or the like
> in the spec either.
Actually, I just came up with an explanation: The DA and RA can be
different, and the DA inside need not necessarily be the RA. Taken in
the context of the overall paragraph, that makes sense:
An A-MSDU contains only MSDUs whose DA and SA parameter values map
to the same receiver address (RA) and transmitter address (TA)
values, i.e., all the MSDUs are intended to be received by a single
receiver, and necessarily they are all transmitted by the same
transmitter. The rules for determining RA and TA are independent of
whether the frame body carries an A-MSDU.
NOTE—It is possible to have different DA and SA parameter values in
A-MSDU subframe headers of the same A-MSDU as long as they all map
to the same Address 1 and Address 2 parameter values.
Obviously, now that I think about it, your patch also would break
client mode since it would refuse to accept any A-MSDU with SA != TA,
which is highly unlikely in most cases, since traffic doesn't usually
originate from the AP.
Overall, it seems to me we should do the following:
* allow checking both DA and SA, by having optional arguments to the
function for this
* pass DA == RA for client mode (not when 4-addr)
* pass SA == TA for AP/VLAN modes (not when 4-addr)
* pass both on TDLS links
* pass both for IBSS mode (I think)
For the "to the AP" case, this of course also covers multicast, since
the DA is A3 and won't be checked.
johannes
> * pass both for IBSS mode (I think)
two more aspects for IBSS
1. the PSK is shared by all stations, so a passive attacker on any
authenticated station in range will be able to derive it, right?
2. iff at all the source mac might be used for access control so
a TA==SA A-AMSDU filter might still be reasonable.
Michael
Am 04.10.2016 um 23:57 schrieb M. Braun:
>> * pass both for IBSS mode (I think)
>
> two more aspects for IBSS
>
> 1. the PSK is shared by all stations, so a passive attacker on any
> authenticated station in range will be able to derive it, right?
> 2. iff at all the source mac might be used for access control so
> a TA==SA A-AMSDU filter might still be reasonable.
under the assumption that every station can only know about the keys it
is supposed to know, checking for da is multicast if ra is multicast is
needed as well so that the source address cannot be spoofed using GTK.
I'll send an updated version.
Michael
> > First of all - there's nothing specific to "AP interfaces", which
> > you say in the subject, as far as I can tell? That should be
> > removed?
>
> >
> > >
> > > if (unlikely(ta &&
> > > + (iftype == NL80211_IFTYPE_AP ||
> > > + iftype == NL80211_IFTYPE_AP_VLAN)
> > > &&
> > > + !ether_addr_equal(ta, eth.h_source)
> > > + ))
> > > + goto purge;
>
> So the A-MSDU packets are only dropped if received by an interface in
> AP or AP_VLAN mode, not on client side, as my original issue was
> about arp/ip filters being circumvented on AP side.
D'oh. Not paying attention, I guess.
Nevertheless, why should this be conditional on the interface type? It
seems to me that we should apply this to any type? What if, for
example, another BSS client sends an A-MSDU encrypted with the GTK and
then fakes something inside that way? We verify today that only
multicast frames can be encrypted with the GTK, but that applies to the
outer header, so we're susceptible to a variant of the hole-196 attack,
afaict?
> > > 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).
> >
> > I think this is wrong. As we do not support DMS (yet), we should
> > adhere to 8.3.2.2 and only accept matching TA/SA and DA/RA.
>
> IEEE 802.11-2012 8.3.2.2 contains the note "NOTE—It is possible to
> have different DA and SA parameter values in A-MSDU subframe headers
> of the same A-MSDU as long as they all map to the same Address 1 and
> Address 2 parameter values."
>
> I conclude that embedding multicast in unicast A-MSDU frames is
> generally allowed, because "mapping" does not mean "be identical".
Yeah, I saw this. It's not clear to me that they intended this wording
to be about multicast though. I'm not really sure what they had in mind
here, but there's an exception for multicast for DMS, which would seem
pointless if they had intended this "mapping" to be about multicast.
Then again, I don't know of any "address mapping" service or the like
in the spec either.
johannes
> Obviously, now that I think about it, your patch also would break
> client mode since it would refuse to accept any A-MSDU with SA != TA,
> which is highly unlikely in most cases, since traffic doesn't usually
> originate from the AP.
I still don't think my patch would break anything here, as it does only
filter on AP/AP_VLAN interfaces so stations are not affected at all.
I agree that asking for more cases to be filtered seems natural. But is
fixing all possible A-MSDU address mismatch problems required to fix the
one I currently care about most?
> We verify today that only multicast frames can be encrypted with the
> GTK, but that applies to the outer header, so we're susceptible to a
> variant of the hole-196 attack, afaict?
That exploited that an unicast arp reply can be injected using GTK, thus
bypassing AP filtering, right?
To counter, it would suffice to required multicast A-MSDU frames to only
carry multicast (inner) MSDUs?
I don't see how this could be inversed, that is an attack exploiting
multicast encrypted with PTK.
> Overall, it seems to me we should do the following:
I'm wondering if all this filtering is actually required or if it
filters out more than required?
> * pass DA == RA for client mode (not when 4-addr)
If this implies that encapsulating multicast as unicast A-MSDU is not
permitted, then why? This would break multicast to unicast using A-MSDU
frames, which currently works with most clients for me.
In order to fully counter hole-196, one has to remove the shared key
property of GTK. That is, each multicast packet is send once for each
station using a different encryption key. This currently is slow due to
multicast rate and makes each station wake up and receive each copy of
the multicast frame, including all those it cannot decrypt.
Using A-MSDU based multicast to unicast, these effects can be mitigated
regardless of packet payload type by using faster unicast transmissions
instead, as the wireless stack is supposed to restore the original
(inner) frame.
> * pass both on TDLS links
I don't know why TDLS links should carry multicast at all, so this seems
reasonable to me.
> * pass both for IBSS mode (I think)
Why is multicast to unicast not permitted within IBSS?
Hole-196 does not really seem to apply, as we're lacking the AP filter
rules to be bypassed anyway?
michael