[PATCH] nl80211: Reject disconnect commands except from conn_owner

[PATCH] nl80211: Reject disconnect commands except from conn_owner

[Andrew Zaborowski]

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.

Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
 net/wireless/nl80211.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index e4a52a2b5e..85f094a564 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -8506,6 +8506,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;
 
@@ -8628,6 +8632,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;
 
@@ -8675,6 +8683,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;
 
@@ -9451,6 +9463,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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

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 <andrew.zaborowski@intel.com>
> ---
>   net/wireless/nl80211.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Johannes Berg]

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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Andrew Zaborowski]

On 22 May 2018 at 09:53, Arend van Spriel <arend.vanspriel@broadcom.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

On 5/22/2018 12:33 PM, Andrew Zaborowski wrote:
> On 22 May 2018 at 09:53, Arend van Spriel <arend.vanspriel@broadcom.com> 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

[PATCH] nl80211: Reject disconnect commands except from conn_owner

[Andrew Zaborowski]

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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Kalle Valo]

Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Andrew Zaborowski]

On 28 April 2018 at 15:07, Kalle Valo <kvalo@codeaurora.org> wrote:
> Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Johannes Berg]

On Sun, 2018-04-29 at 20:30 +0200, Andrew Zaborowski wrote:
> On 28 April 2018 at 15:07, Kalle Valo <kvalo@codeaurora.org> wrote:
> > Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

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 <kvalo@codeaurora.org> wrote:
>>> Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Johannes Berg]

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 <kvalo@codeaurora.org> wrote:
> > > > Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

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 <kvalo@codeaurora.org> wrote:
>>>>> Andrew Zaborowski <andrew.zaborowski@intel.com> 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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Arend van Spriel]

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.

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.

Regards,
Arend

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Denis Kenzior]

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

Re: [PATCH] nl80211: Reject disconnect commands except from conn_owner

[Andrew Zaborowski]

Hi,

On 8 May 2018 at 14:19, Johannes Berg <johannes@sipsolutions.net> 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 <kvalo@codeaurora.org> wrote:
>> > > > Andrew Zaborowski <andrew.zaborowski@intel.com> 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