Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
from clients other than the connection owner set in the connect,
authenticate or associate commands, if it was set.
The main point of this check is to prevent chaos when two processes
try to use nl80211 at the same time, it's not a security measure.
The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
START_AP/STOP_AP.
---
net/wireless/nl80211.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index a910150f81..92ddc7d88f 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -8303,6 +8303,10 @@ static int nl80211_associate(struct sk_buff *skb, struct genl_info *info)
const u8 *bssid, *ssid;
int err, ssid_len = 0;
+ if (dev->ieee80211_ptr->conn_owner_nlportid &&
+ dev->ieee80211_ptr->conn_owner_nlportid != info->snd_portid)
+ return -EPERM;
+
if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]))
return -EINVAL;
@@ -8425,6 +8429,10 @@ static int nl80211_deauthenticate(struct sk_buff *skb, struct genl_info *info)
u16 reason_code;
bool local_state_change;
+ if (dev->ieee80211_ptr->conn_owner_nlportid &&
+ dev->ieee80211_ptr->conn_owner_nlportid != info->snd_portid)
+ return -EPERM;
+
if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]))
return -EINVAL;
@@ -8472,6 +8480,10 @@ static int nl80211_disassociate(struct sk_buff *skb, struct genl_info *info)
u16 reason_code;
bool local_state_change;
+ if (dev->ieee80211_ptr->conn_owner_nlportid &&
+ dev->ieee80211_ptr->conn_owner_nlportid != info->snd_portid)
+ return -EPERM;
+
if (!is_valid_ie_attr(info->attrs[NL80211_ATTR_IE]))
return -EINVAL;
@@ -9234,6 +9246,10 @@ static int nl80211_disconnect(struct sk_buff *skb, struct genl_info *info)
u16 reason;
int ret;
+ if (dev->ieee80211_ptr->conn_owner_nlportid &&
+ dev->ieee80211_ptr->conn_owner_nlportid != info->snd_portid)
+ return -EPERM;
+
if (!info->attrs[NL80211_ATTR_REASON_CODE])
reason = WLAN_REASON_DEAUTH_LEAVING;
else
--
2.14.1
On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
> Andrew Zaborowski <[email protected]> writes:
>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>> from clients other than the connection owner set in the connect,
>> authenticate or associate commands, if it was set.
>>
>> The main point of this check is to prevent chaos when two processes
>> try to use nl80211 at the same time, it's not a security measure.
>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>> START_AP/STOP_AP.
>
> s-o-b missing.
True, thanks. Also I was going to send this as an RFC.
Best regards
Andrew Zaborowski <[email protected]> writes:
> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
> from clients other than the connection owner set in the connect,
> authenticate or associate commands, if it was set.
>
> The main point of this check is to prevent chaos when two processes
> try to use nl80211 at the same time, it's not a security measure.
> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
> START_AP/STOP_AP.
s-o-b missing.
--
Kalle Valo
On 5/7/2018 9:19 PM, Johannes Berg wrote:
> On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
>> On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
>>> Andrew Zaborowski <[email protected]> writes:
>>>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>>>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>>>> from clients other than the connection owner set in the connect,
>>>> authenticate or associate commands, if it was set.
>>>>
>>>> The main point of this check is to prevent chaos when two processes
>>>> try to use nl80211 at the same time, it's not a security measure.
>>>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>>>> START_AP/STOP_AP.
>>>
>>> s-o-b missing.
>>
>> True, thanks. Also I was going to send this as an RFC.
>>
> Looks fine to me, please resend if you want it in :)
Do we really want this? Is the referred chaos hypothetical or an actual
issue. Nothing stops me from doing an 'ifconfig down' so why should 'iw
disconnect' be any different. As far I can tell it does not affect my
testing environment, but particularly in such use-cases I can expect
issues adopting this change, which is also hypothetical of course ;-)
Regards,
Arend
On 5/8/2018 2:19 PM, Johannes Berg wrote:
> On Tue, 2018-05-08 at 14:18 +0200, Arend van Spriel wrote:
>> On 5/7/2018 9:19 PM, Johannes Berg wrote:
>>> On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
>>>> On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
>>>>> Andrew Zaborowski <[email protected]> writes:
>>>>>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>>>>>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>>>>>> from clients other than the connection owner set in the connect,
>>>>>> authenticate or associate commands, if it was set.
>>>>>>
>>>>>> The main point of this check is to prevent chaos when two processes
>>>>>> try to use nl80211 at the same time, it's not a security measure.
>>>>>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>>>>>> START_AP/STOP_AP.
>>>>>
>>>>> s-o-b missing.
>>>>
>>>> True, thanks. Also I was going to send this as an RFC.
>>>>
>>>
>>> Looks fine to me, please resend if you want it in :)
>>
>> Do we really want this? Is the referred chaos hypothetical or an actual
>> issue. Nothing stops me from doing an 'ifconfig down' so why should 'iw
>> disconnect' be any different. As far I can tell it does not affect my
>> testing environment, but particularly in such use-cases I can expect
>> issues adopting this change, which is also hypothetical of course ;-)
>
> Yeah, it's a good question. But it might help with inadvertent issues,
> like starting wpa_s which immediately disconnects if it finds something
> connected. If that fails, perhaps you have a better chance of noticing
> the error?
Sure. I guess we all have been there kicking of wpa_s and discovering
there is already one running in the background. I am just a bit
squeamish to change the behavior like this. Hmmmm. Is wpa_s already
using SOCKET_OWNER. If so, I might create a patch to opt-out for that so
people can knowingly choose chaos ;-)
Regards,
Arend
On Tue, 2018-05-08 at 14:18 +0200, Arend van Spriel wrote:
> On 5/7/2018 9:19 PM, Johannes Berg wrote:
> > On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
> > > On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
> > > > Andrew Zaborowski <[email protected]> writes:
> > > > > Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
> > > > > NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
> > > > > from clients other than the connection owner set in the connect,
> > > > > authenticate or associate commands, if it was set.
> > > > >
> > > > > The main point of this check is to prevent chaos when two processes
> > > > > try to use nl80211 at the same time, it's not a security measure.
> > > > > The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
> > > > > START_AP/STOP_AP.
> > > >
> > > > s-o-b missing.
> > >
> > > True, thanks. Also I was going to send this as an RFC.
> > >
> >
> > Looks fine to me, please resend if you want it in :)
>
> Do we really want this? Is the referred chaos hypothetical or an actual
> issue. Nothing stops me from doing an 'ifconfig down' so why should 'iw
> disconnect' be any different. As far I can tell it does not affect my
> testing environment, but particularly in such use-cases I can expect
> issues adopting this change, which is also hypothetical of course ;-)
Yeah, it's a good question. But it might help with inadvertent issues,
like starting wpa_s which immediately disconnects if it finds something
connected. If that fails, perhaps you have a better chance of noticing
the error?
johannes
Hi Arend,
On 05/22/2018 05:39 AM, Arend van Spriel wrote:
> On 5/8/2018 5:05 PM, Denis Kenzior wrote:
>> Hi Arend,
>>
>>>
>>> Sure. I guess we all have been there kicking of wpa_s and discovering
>>> there is already one running in the background. I am just a bit
>>> squeamish to change the behavior like this. Hmmmm. Is wpa_s already
>>> using SOCKET_OWNER. If so, I might create a patch to opt-out for that
>>> so people can knowingly choose chaos ;-)
>>>
>>
>> wpa_s is using SOCKET_OWNER these days.? However, with the introduction
>> of Control Port over NL80211, just getting rid of SOCKET_OWNER might not
>> be that easy.
>
> I have a regression test script employing py80211 which listens for
> connect event. Right now I am using an older wpa_s, but the above will
> screw it up.
So it is still early in the morning for me and you might have to talk
slower. But let me take a stab anyway :)
What will screw up? The connect event is always multicast, so you
should still receive it regardless of SOCKET_OWNER / Control Port over
NL80211.
>
> If I recall correctly the mlme notification needed to be unicast,
> because multicast was not 100% reliable, right? Would it be acceptable
> to send unicast to the socket owner and still do the multicast or are we
> already doing that? If not, that would fix my imminent issue.
>
Control Port events are only unicast to the application that is the
SOCKET_OWNER. E.g. either wpa_s or iwd. They are never multicast as
nobody else could make sense of them anyway due to lack of passphrase /
nonces.
The other 'regular' events like Disconnect, Connect, etc are still
multicast. What this patch does is prevents some app from rudely coming
in and sending a Disconnect on an interface which is being managed by
another process, e.g. iwd or wpa_s, that has set the SOCKET_OWNER flag.
Regards,
-Denis
On 5/22/2018 12:33 PM, Andrew Zaborowski wrote:
> On 22 May 2018 at 09:53, Arend van Spriel <[email protected]> wrote:
>> On 5/22/2018 2:46 AM, Andrew Zaborowski wrote:
>>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>>> from clients other than the connection owner set in the connect,
>>> authenticate or associate commands, if it was set.
>>>
>>> The main point of this check is to prevent chaos when two processes
>>> try to use nl80211 at the same time, it's not a security measure.
>>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>>> START_AP/STOP_AP.
>>
>> Isn't this V2? What has changed since last time?
>
> It's only a resend to include the Signed-off-by that I forgot the first time.
That could also be it ;-)
Thanks,
Arend
On 5/22/2018 2:46 AM, Andrew Zaborowski wrote:
> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
> from clients other than the connection owner set in the connect,
> authenticate or associate commands, if it was set.
>
> The main point of this check is to prevent chaos when two processes
> try to use nl80211 at the same time, it's not a security measure.
> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
> START_AP/STOP_AP.
Isn't this V2? What has changed since last time?
Regards,
Arend
> Signed-off-by: Andrew Zaborowski <[email protected]>
> ---
> net/wireless/nl80211.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
> On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
> > Andrew Zaborowski <[email protected]> writes:
> > > Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
> > > NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
> > > from clients other than the connection owner set in the connect,
> > > authenticate or associate commands, if it was set.
> > >
> > > The main point of this check is to prevent chaos when two processes
> > > try to use nl80211 at the same time, it's not a security measure.
> > > The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
> > > START_AP/STOP_AP.
> >
> > s-o-b missing.
>
> True, thanks. Also I was going to send this as an RFC.
>
Looks fine to me, please resend if you want it in :)
johannes
On Tue, 2018-05-22 at 09:53 +0200, Arend van Spriel wrote:
> On 5/22/2018 2:46 AM, Andrew Zaborowski wrote:
> > Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
> > NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
> > from clients other than the connection owner set in the connect,
> > authenticate or associate commands, if it was set.
> >
> > The main point of this check is to prevent chaos when two processes
> > try to use nl80211 at the same time, it's not a security measure.
> > The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
> > START_AP/STOP_AP.
>
> Isn't this V2? What has changed since last time?
It was RFC before I guess.
Doesn't look different to me.
johannes
On 5/22/2018 10:21 AM, Johannes Berg wrote:
> On Tue, 2018-05-22 at 09:53 +0200, Arend van Spriel wrote:
>> On 5/22/2018 2:46 AM, Andrew Zaborowski wrote:
>>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>>> from clients other than the connection owner set in the connect,
>>> authenticate or associate commands, if it was set.
>>>
>>> The main point of this check is to prevent chaos when two processes
>>> try to use nl80211 at the same time, it's not a security measure.
>>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>>> START_AP/STOP_AP.
>>
>> Isn't this V2? What has changed since last time?
>
> It was RFC before I guess.
Ah, yes. That's it.
Thanks,
Arend
Hi,
On 8 May 2018 at 14:19, Johannes Berg <[email protected]> wrote:
> On Tue, 2018-05-08 at 14:18 +0200, Arend van Spriel wrote:
>> On 5/7/2018 9:19 PM, Johannes Berg wrote:
>> > On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
>> > > On 28 April 2018 at 15:07, Kalle Valo <[email protected]> wrote:
>> > > > Andrew Zaborowski <[email protected]> writes:
>> > > > > Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>> > > > > NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>> > > > > from clients other than the connection owner set in the connect,
>> > > > > authenticate or associate commands, if it was set.
>> > > > >
>> > > > > The main point of this check is to prevent chaos when two processes
>> > > > > try to use nl80211 at the same time, it's not a security measure.
>> > > > > The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>> > > > > START_AP/STOP_AP.
>> > > >
>> > > > s-o-b missing.
>> > >
>> > > True, thanks. Also I was going to send this as an RFC.
>> > >
>> >
>> > Looks fine to me, please resend if you want it in :)
>>
>> Do we really want this? Is the referred chaos hypothetical or an actual
>> issue. Nothing stops me from doing an 'ifconfig down' so why should 'iw
>> disconnect' be any different. As far I can tell it does not affect my
>> testing environment, but particularly in such use-cases I can expect
>> issues adopting this change, which is also hypothetical of course ;-)
>
> Yeah, it's a good question. But it might help with inadvertent issues,
> like starting wpa_s which immediately disconnects if it finds something
> connected. If that fails, perhaps you have a better chance of noticing
> the error?
This patch was only motivated by a case of running both iwd and
wpa_supplicant simultaneously by mistake. They can both handle things
like 'ifconfig down', but when one daemon tries to connect the other
immediately commands a disconnect (makes sense) and it made us think
whether it was more correct if that disconnect command failed. I'm
not really sure what's right here so I wanted to float this idea.
Best regards
On 22 May 2018 at 09:53, Arend van Spriel <[email protected]> wrote:
> On 5/22/2018 2:46 AM, Andrew Zaborowski wrote:
>> Reject NL80211_CMD_DISCONNECT, NL80211_CMD_DISASSOCIATE,
>> NL80211_CMD_DEAUTHENTICATE and NL80211_CMD_ASSOCIATE commands
>> from clients other than the connection owner set in the connect,
>> authenticate or associate commands, if it was set.
>>
>> The main point of this check is to prevent chaos when two processes
>> try to use nl80211 at the same time, it's not a security measure.
>> The same thing should possibly be done for JOIN_IBSS/LEAVE_IBSS and
>> START_AP/STOP_AP.
>
> Isn't this V2? What has changed since last time?
It's only a resend to include the Signed-off-by that I forgot the first time.
Best regards