From: Chaitanya T K <[email protected]>
Enforce the check for protected field for all unicast
robust management frames.
Signed-off-by: Chaitanya T K <[email protected]>
---
This removed the dependency on the driver to check for protected
bit, especially for those drivers who believed the API :-).
---
net/mac80211/rx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a8d862f..63e8f3d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -569,6 +569,9 @@ static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
if (is_multicast_ether_addr(hdr->addr1))
return 0;
+ if (!ieee80211_has_protected(hdr->frame_control))
+ return 0;
+
return ieee80211_is_robust_mgmt_frame(skb);
}
On Fri, Aug 22, 2014 at 2:44 PM, Jouni Malinen <[email protected]> wrote:
>
> On Tue, Aug 12, 2014 at 12:21:54AM +0530, [email protected] wrote:
> > Enforce the check for protected field for all unicast
> > robust management frames.
>
> Why? This function is supposed to indicate whether the frame is a robust
> action frame and as such, has to have Protected bit set to one. If the
> sender (attacker) tries to send the frame unprotected, it will still
> need to be caught here.
>
> Rather than enforcing anything, this would add a significant security
> vulnerability by breaking PMF more or less completely.
I agree jouni, we were using this API to figure out the length of the
crypto header (IV) to pass it to the HW crypto, so even for
unencrypted frames during the initial connection we were treating as
robust mgmt frames causing us trouble.
>
>
> > This removed the dependency on the driver to check for protected
> > bit, especially for those drivers who believed the API :-).
>
> Huh.. What is this driver referring to or what do you think the API is
> supposed to be doing? ieee80211_is_unicast_robust_mgmt_frame() is a
> static function within net/mac80211/rx.c and has only a single caller,
> so it cannot really be used by any driver..
Sorry, i overlooked the static in git, we have a custom kernel without
the static.
>
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > @@ -569,6 +569,9 @@ static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
> > if (is_multicast_ether_addr(hdr->addr1))
> > return 0;
> >
> > + if (!ieee80211_has_protected(hdr->frame_control))
> > + return 0;
> > +
> > return ieee80211_is_robust_mgmt_frame(skb);
>
> This looks very incorrect. This would completely break
> ieee80211_drop_unencrypted_mgmt() and allow unprotected robust
> management frames to be processed.
Ok i see it. robust mgmt and protected robust mgmt checks are
independently handled, but as the name suggested unicast robust mgmt
isn't it better to club those 2 checks together?
--
Thanks,
Regards,
Chaitanya T K.
On Tue, Aug 12, 2014 at 12:21 AM, <[email protected]> wrote:
> From: Chaitanya T K <[email protected]>
>
> Enforce the check for protected field for all unicast
> robust management frames.
>
> Signed-off-by: Chaitanya T K <[email protected]>
> ---
>
> This removed the dependency on the driver to check for protected
> bit, especially for those drivers who believed the API :-).
>
> ---
>
> net/mac80211/rx.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index a8d862f..63e8f3d 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -569,6 +569,9 @@ static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
> if (is_multicast_ether_addr(hdr->addr1))
> return 0;
>
> + if (!ieee80211_has_protected(hdr->frame_control))
> + return 0;
> +
> return ieee80211_is_robust_mgmt_frame(skb);
> }
>
Johannes,
Subject is wrong it should be [PATCH] mac8021:, so should i resend it again?
On Tue, Aug 12, 2014 at 12:21:54AM +0530, [email protected] wrote:
> Enforce the check for protected field for all unicast
> robust management frames.
Why? This function is supposed to indicate whether the frame is a robust
action frame and as such, has to have Protected bit set to one. If the
sender (attacker) tries to send the frame unprotected, it will still
need to be caught here.
Rather than enforcing anything, this would add a significant security
vulnerability by breaking PMF more or less completely.
> This removed the dependency on the driver to check for protected
> bit, especially for those drivers who believed the API :-).
Huh.. What is this driver referring to or what do you think the API is
supposed to be doing? ieee80211_is_unicast_robust_mgmt_frame() is a
static function within net/mac80211/rx.c and has only a single caller,
so it cannot really be used by any driver..
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> @@ -569,6 +569,9 @@ static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
> if (is_multicast_ether_addr(hdr->addr1))
> return 0;
>
> + if (!ieee80211_has_protected(hdr->frame_control))
> + return 0;
> +
> return ieee80211_is_robust_mgmt_frame(skb);
This looks very incorrect. This would completely break
ieee80211_drop_unencrypted_mgmt() and allow unprotected robust
management frames to be processed.
--
Jouni Malinen PGP id EFC895FA
On Sat, Aug 23, 2014 at 9:02 PM, Krishna Chaitanya
<[email protected]> wrote:
> On Fri, Aug 22, 2014 at 2:44 PM, Jouni Malinen <[email protected]> wrote:
>>
>> On Tue, Aug 12, 2014 at 12:21:54AM +0530, [email protected] wrote:
>> > Enforce the check for protected field for all unicast
>> > robust management frames.
>>
>> Why? This function is supposed to indicate whether the frame is a robust
>> action frame and as such, has to have Protected bit set to one. If the
>> sender (attacker) tries to send the frame unprotected, it will still
>> need to be caught here.
>>
>> Rather than enforcing anything, this would add a significant security
>> vulnerability by breaking PMF more or less completely.
>
> I agree jouni, we were using this API to figure out the length of the
> crypto header (IV) to pass it to the HW crypto, so even for
> unencrypted frames during the initial connection we were treating as
> robust mgmt frames causing us trouble.
>>
>>
>> > This removed the dependency on the driver to check for protected
>> > bit, especially for those drivers who believed the API :-).
>>
>> Huh.. What is this driver referring to or what do you think the API is
>> supposed to be doing? ieee80211_is_unicast_robust_mgmt_frame() is a
>> static function within net/mac80211/rx.c and has only a single caller,
>> so it cannot really be used by any driver..
>
> Sorry, i overlooked the static in git, we have a custom kernel without
> the static.
>
>>
>> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
>> > @@ -569,6 +569,9 @@ static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
>> > if (is_multicast_ether_addr(hdr->addr1))
>> > return 0;
>> >
>> > + if (!ieee80211_has_protected(hdr->frame_control))
>> > + return 0;
>> > +
>> > return ieee80211_is_robust_mgmt_frame(skb);
>>
>> This looks very incorrect. This would completely break
>> ieee80211_drop_unencrypted_mgmt() and allow unprotected robust
>> management frames to be processed.
>
> Ok i see it. robust mgmt and protected robust mgmt checks are
> independently handled, but as the name suggested unicast robust mgmt
> isn't it better to club those 2 checks together?
>
Johannes,
Please drop this patch, i did not see that the function is static.
Thanks.