Development discussion of WireGuard
 help / color / mirror / Atom feed
* [PATCH] wireguard: netlink: add multicast notification for peer changes
@ 2021-01-09 21:00 Linus Lotz
  2021-01-11 20:48 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Linus Lotz @ 2021-01-09 21:00 UTC (permalink / raw)
  Cc: linus, Jason A. Donenfeld, David S. Miller, Jakub Kicinski,
	wireguard, netdev, linux-kernel

This commit adds a new multicast group to the netlink api for wireguard.
The purpose of this multicast group is to notify userspace when the
peers of an interface change. Right now this is only done when the
endpoint is changed by whatever means.

An example for an consumer of this API would be a service that keeps
track of all peer endpoints and sends this information to the peers.
This would allow NAT-to-NAT connections without the need of using
STUN on each client.

Signed-off-by: Linus Lotz <linus@lotz.li>
---
 drivers/net/wireguard/netlink.c | 52 ++++++++++++++++++++++++++++++++-
 drivers/net/wireguard/netlink.h |  4 +++
 drivers/net/wireguard/socket.c  |  4 +++
 include/uapi/linux/wireguard.h  |  3 ++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index d0f3b6d7f408..88eff0a797f4 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -618,6 +618,12 @@ static const struct genl_ops genl_ops[] = {
 	}
 };
 
+static struct genl_multicast_group genl_mcgrps[] = {
+	{
+		.name = WG_MULTICAST_GROUP_PEER_CHANGE
+	}
+};
+
 static struct genl_family genl_family __ro_after_init = {
 	.ops = genl_ops,
 	.n_ops = ARRAY_SIZE(genl_ops),
@@ -626,7 +632,9 @@ static struct genl_family genl_family __ro_after_init = {
 	.maxattr = WGDEVICE_A_MAX,
 	.module = THIS_MODULE,
 	.policy = device_policy,
-	.netnsok = true
+	.netnsok = true,
+	.mcgrps = genl_mcgrps,
+	.n_mcgrps = ARRAY_SIZE(genl_mcgrps)
 };
 
 int __init wg_genetlink_init(void)
@@ -638,3 +646,45 @@ void __exit wg_genetlink_uninit(void)
 {
 	genl_unregister_family(&genl_family);
 }
+
+int wg_genl_mcast_peer_endpoint_change(struct wg_peer *peer)
+{
+	struct sk_buff *skb;
+	void *hdr, *peer_nest, *peer_array_nest;
+	int fail;
+
+	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	hdr = genlmsg_put(skb, 0, 0,
+			  &genl_family, 0, WG_CMD_CHANGED_PEER);
+
+	nla_put_u32(skb, WGDEVICE_A_IFINDEX, peer->device->dev->ifindex);
+	nla_put_string(skb, WGDEVICE_A_IFNAME, peer->device->dev->name);
+
+	peer_nest = nla_nest_start(skb, WGDEVICE_A_PEERS);
+	peer_array_nest = nla_nest_start(skb, 0);
+	down_read(&peer->handshake.lock);
+	nla_put(skb, WGPEER_A_PUBLIC_KEY, NOISE_PUBLIC_KEY_LEN,
+		peer->handshake.remote_static);
+	up_read(&peer->handshake.lock);
+
+	read_lock_bh(&peer->endpoint_lock);
+	if (peer->endpoint.addr.sa_family == AF_INET)
+		fail = nla_put(skb, WGPEER_A_ENDPOINT,
+			       sizeof(peer->endpoint.addr4),
+			       &peer->endpoint.addr4);
+	else if (peer->endpoint.addr.sa_family == AF_INET6)
+		fail = nla_put(skb, WGPEER_A_ENDPOINT,
+			       sizeof(peer->endpoint.addr6),
+			       &peer->endpoint.addr6);
+	read_unlock_bh(&peer->endpoint_lock);
+	if (fail)
+		return fail;
+
+	nla_nest_end(skb, peer_array_nest);
+	nla_nest_end(skb, peer_nest);
+	genlmsg_end(skb, hdr);
+
+	fail = genlmsg_multicast_netns(&genl_family, dev_net(peer->device->dev),
+				       skb, 0, 0, GFP_KERNEL);
+	return fail;
+}
diff --git a/drivers/net/wireguard/netlink.h b/drivers/net/wireguard/netlink.h
index 15100d92e2e3..74ecc72a79a6 100644
--- a/drivers/net/wireguard/netlink.h
+++ b/drivers/net/wireguard/netlink.h
@@ -6,6 +6,10 @@
 #ifndef _WG_NETLINK_H
 #define _WG_NETLINK_H
 
+#include "peer.h"
+
+int wg_genl_mcast_peer_endpoint_change(struct wg_peer *peer);
+
 int wg_genetlink_init(void);
 void wg_genetlink_uninit(void);
 
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c
index 410b318e57fb..d826e1f2b51c 100644
--- a/drivers/net/wireguard/socket.c
+++ b/drivers/net/wireguard/socket.c
@@ -8,6 +8,7 @@
 #include "socket.h"
 #include "queueing.h"
 #include "messages.h"
+#include "netlink.h"
 
 #include <linux/ctype.h>
 #include <linux/net.h>
@@ -293,6 +294,9 @@ void wg_socket_set_peer_endpoint(struct wg_peer *peer,
 	dst_cache_reset(&peer->endpoint_cache);
 out:
 	write_unlock_bh(&peer->endpoint_lock);
+
+	/* We need to notify the netlink listeners for about this change */
+	wg_genl_mcast_peer_endpoint_change(peer);
 }
 
 void wg_socket_set_peer_endpoint_from_skb(struct wg_peer *peer,
diff --git a/include/uapi/linux/wireguard.h b/include/uapi/linux/wireguard.h
index ae88be14c947..22a012644d71 100644
--- a/include/uapi/linux/wireguard.h
+++ b/include/uapi/linux/wireguard.h
@@ -136,9 +136,12 @@
 
 #define WG_KEY_LEN 32
 
+#define WG_MULTICAST_GROUP_PEER_CHANGE          "wg_peer_change"
+
 enum wg_cmd {
 	WG_CMD_GET_DEVICE,
 	WG_CMD_SET_DEVICE,
+	WG_CMD_CHANGED_PEER,
 	__WG_CMD_MAX
 };
 #define WG_CMD_MAX (__WG_CMD_MAX - 1)
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] wireguard: netlink: add multicast notification for peer changes
  2021-01-09 21:00 [PATCH] wireguard: netlink: add multicast notification for peer changes Linus Lotz
@ 2021-01-11 20:48 ` kernel test robot
  2021-01-15 19:53 ` [PATCH v2] " Linus Lotz
  2021-03-07 21:49 ` [PATCH] " Jason A. Donenfeld
  2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-01-11 20:48 UTC (permalink / raw)
  To: Linus Lotz
  Cc: kbuild-all, clang-built-linux, linus, Jason A. Donenfeld,
	Jakub Kicinski, wireguard, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3765 bytes --]

Hi Linus,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc3 next-20210111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Linus-Lotz/wireguard-netlink-add-multicast-notification-for-peer-changes/20210110-051531
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 996e435fd401de35df62ac943ab9402cfe85c430
config: x86_64-randconfig-a013-20210111 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 7be3285248bf54d0784a76174cf44cf7c1e780a5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/3f394635e584429ebbadfbf315c0fe4e2d36b583
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Linus-Lotz/wireguard-netlink-add-multicast-notification-for-peer-changes/20210110-051531
        git checkout 3f394635e584429ebbadfbf315c0fe4e2d36b583
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/wireguard/netlink.c:675:11: warning: variable 'fail' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           else if (peer->endpoint.addr.sa_family == AF_INET6)
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireguard/netlink.c:680:6: note: uninitialized use occurs here
           if (fail)
               ^~~~
   drivers/net/wireguard/netlink.c:675:7: note: remove the 'if' if its condition is always true
           else if (peer->endpoint.addr.sa_family == AF_INET6)
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/wireguard/netlink.c:654:10: note: initialize the variable 'fail' to silence this warning
           int fail;
                   ^
                    = 0
   1 warning generated.


vim +675 drivers/net/wireguard/netlink.c

   649	
   650	int wg_genl_mcast_peer_endpoint_change(struct wg_peer *peer)
   651	{
   652		struct sk_buff *skb;
   653		void *hdr, *peer_nest, *peer_array_nest;
   654		int fail;
   655	
   656		skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
   657		hdr = genlmsg_put(skb, 0, 0,
   658				  &genl_family, 0, WG_CMD_CHANGED_PEER);
   659	
   660		nla_put_u32(skb, WGDEVICE_A_IFINDEX, peer->device->dev->ifindex);
   661		nla_put_string(skb, WGDEVICE_A_IFNAME, peer->device->dev->name);
   662	
   663		peer_nest = nla_nest_start(skb, WGDEVICE_A_PEERS);
   664		peer_array_nest = nla_nest_start(skb, 0);
   665		down_read(&peer->handshake.lock);
   666		nla_put(skb, WGPEER_A_PUBLIC_KEY, NOISE_PUBLIC_KEY_LEN,
   667			peer->handshake.remote_static);
   668		up_read(&peer->handshake.lock);
   669	
   670		read_lock_bh(&peer->endpoint_lock);
   671		if (peer->endpoint.addr.sa_family == AF_INET)
   672			fail = nla_put(skb, WGPEER_A_ENDPOINT,
   673				       sizeof(peer->endpoint.addr4),
   674				       &peer->endpoint.addr4);
 > 675		else if (peer->endpoint.addr.sa_family == AF_INET6)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32150 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] wireguard: netlink: add multicast notification for peer changes
  2021-01-09 21:00 [PATCH] wireguard: netlink: add multicast notification for peer changes Linus Lotz
  2021-01-11 20:48 ` kernel test robot
@ 2021-01-15 19:53 ` Linus Lotz
  2021-01-15 23:40   ` Jason A. Donenfeld
  2021-03-07 21:49 ` [PATCH] " Jason A. Donenfeld
  2 siblings, 1 reply; 8+ messages in thread
From: Linus Lotz @ 2021-01-15 19:53 UTC (permalink / raw)
  Cc: Linus Lotz, kernel test robot, Jason A. Donenfeld,
	David S. Miller, Jakub Kicinski, wireguard, netdev, linux-kernel

This commit adds a new multicast group to the netlink api for wireguard.
The purpose of this multicast group is to notify userspace when the
peers of an interface change. Right now this is only done when the
endpoint is changed by whatever means.

An example for an consumer of this API would be a service that keeps
track of all peer endpoints and sends this information to the peers.
This would allow NAT-to-NAT connections without the need of using
STUN on each client.

In v2 I fixed a possible uninitialized use.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Linus Lotz <linus@lotz.li>
---
 drivers/net/wireguard/netlink.c | 52 ++++++++++++++++++++++++++++++++-
 drivers/net/wireguard/netlink.h |  4 +++
 drivers/net/wireguard/socket.c  |  4 +++
 include/uapi/linux/wireguard.h  |  3 ++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index d0f3b6d7f408..e9bb2a3a7b79 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -618,6 +618,12 @@ static const struct genl_ops genl_ops[] = {
 	}
 };
 
+static struct genl_multicast_group genl_mcgrps[] = {
+	{
+		.name = WG_MULTICAST_GROUP_PEER_CHANGE
+	}
+};
+
 static struct genl_family genl_family __ro_after_init = {
 	.ops = genl_ops,
 	.n_ops = ARRAY_SIZE(genl_ops),
@@ -626,7 +632,9 @@ static struct genl_family genl_family __ro_after_init = {
 	.maxattr = WGDEVICE_A_MAX,
 	.module = THIS_MODULE,
 	.policy = device_policy,
-	.netnsok = true
+	.netnsok = true,
+	.mcgrps = genl_mcgrps,
+	.n_mcgrps = ARRAY_SIZE(genl_mcgrps)
 };
 
 int __init wg_genetlink_init(void)
@@ -638,3 +646,45 @@ void __exit wg_genetlink_uninit(void)
 {
 	genl_unregister_family(&genl_family);
 }
+
+int wg_genl_mcast_peer_endpoint_change(struct wg_peer *peer)
+{
+	struct sk_buff *skb;
+	void *hdr, *peer_nest, *peer_array_nest;
+	int fail = 0;
+
+	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	hdr = genlmsg_put(skb, 0, 0,
+			  &genl_family, 0, WG_CMD_CHANGED_PEER);
+
+	nla_put_u32(skb, WGDEVICE_A_IFINDEX, peer->device->dev->ifindex);
+	nla_put_string(skb, WGDEVICE_A_IFNAME, peer->device->dev->name);
+
+	peer_nest = nla_nest_start(skb, WGDEVICE_A_PEERS);
+	peer_array_nest = nla_nest_start(skb, 0);
+	down_read(&peer->handshake.lock);
+	nla_put(skb, WGPEER_A_PUBLIC_KEY, NOISE_PUBLIC_KEY_LEN,
+		peer->handshake.remote_static);
+	up_read(&peer->handshake.lock);
+
+	read_lock_bh(&peer->endpoint_lock);
+	if (peer->endpoint.addr.sa_family == AF_INET)
+		fail = nla_put(skb, WGPEER_A_ENDPOINT,
+			       sizeof(peer->endpoint.addr4),
+			       &peer->endpoint.addr4);
+	else if (peer->endpoint.addr.sa_family == AF_INET6)
+		fail = nla_put(skb, WGPEER_A_ENDPOINT,
+			       sizeof(peer->endpoint.addr6),
+			       &peer->endpoint.addr6);
+	read_unlock_bh(&peer->endpoint_lock);
+	if (fail)
+		return fail;
+
+	nla_nest_end(skb, peer_array_nest);
+	nla_nest_end(skb, peer_nest);
+	genlmsg_end(skb, hdr);
+
+	fail = genlmsg_multicast_netns(&genl_family, dev_net(peer->device->dev),
+				       skb, 0, 0, GFP_KERNEL);
+	return fail;
+}
diff --git a/drivers/net/wireguard/netlink.h b/drivers/net/wireguard/netlink.h
index 15100d92e2e3..74ecc72a79a6 100644
--- a/drivers/net/wireguard/netlink.h
+++ b/drivers/net/wireguard/netlink.h
@@ -6,6 +6,10 @@
 #ifndef _WG_NETLINK_H
 #define _WG_NETLINK_H
 
+#include "peer.h"
+
+int wg_genl_mcast_peer_endpoint_change(struct wg_peer *peer);
+
 int wg_genetlink_init(void);
 void wg_genetlink_uninit(void);
 
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c
index 410b318e57fb..d826e1f2b51c 100644
--- a/drivers/net/wireguard/socket.c
+++ b/drivers/net/wireguard/socket.c
@@ -8,6 +8,7 @@
 #include "socket.h"
 #include "queueing.h"
 #include "messages.h"
+#include "netlink.h"
 
 #include <linux/ctype.h>
 #include <linux/net.h>
@@ -293,6 +294,9 @@ void wg_socket_set_peer_endpoint(struct wg_peer *peer,
 	dst_cache_reset(&peer->endpoint_cache);
 out:
 	write_unlock_bh(&peer->endpoint_lock);
+
+	/* We need to notify the netlink listeners for about this change */
+	wg_genl_mcast_peer_endpoint_change(peer);
 }
 
 void wg_socket_set_peer_endpoint_from_skb(struct wg_peer *peer,
diff --git a/include/uapi/linux/wireguard.h b/include/uapi/linux/wireguard.h
index ae88be14c947..22a012644d71 100644
--- a/include/uapi/linux/wireguard.h
+++ b/include/uapi/linux/wireguard.h
@@ -136,9 +136,12 @@
 
 #define WG_KEY_LEN 32
 
+#define WG_MULTICAST_GROUP_PEER_CHANGE          "wg_peer_change"
+
 enum wg_cmd {
 	WG_CMD_GET_DEVICE,
 	WG_CMD_SET_DEVICE,
+	WG_CMD_CHANGED_PEER,
 	__WG_CMD_MAX
 };
 #define WG_CMD_MAX (__WG_CMD_MAX - 1)

base-commit: 65f0d2414b7079556fbbcc070b3d1c9f9587606d
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] wireguard: netlink: add multicast notification for peer changes
  2021-01-15 19:53 ` [PATCH v2] " Linus Lotz
@ 2021-01-15 23:40   ` Jason A. Donenfeld
  2021-01-16  1:51     ` Linus Lotz
  2021-01-26 20:40     ` Linus Lotz
  0 siblings, 2 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2021-01-15 23:40 UTC (permalink / raw)
  To: Linus Lotz
  Cc: kernel test robot, David S. Miller, Jakub Kicinski,
	WireGuard mailing list, Netdev, LKML

Hey Linus,

My email server has been firewalled from vger.kernel.org until today,
so I didn't see the original until this v2 was sent today. My
apologies. I'll review this first thing on Monday.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] wireguard: netlink: add multicast notification for peer changes
  2021-01-15 23:40   ` Jason A. Donenfeld
@ 2021-01-16  1:51     ` Linus Lotz
  2021-01-26 20:40     ` Linus Lotz
  1 sibling, 0 replies; 8+ messages in thread
From: Linus Lotz @ 2021-01-16  1:51 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: kernel test robot, David S. Miller, Jakub Kicinski,
	WireGuard mailing list, Netdev, LKML

Hi Jason,
No worries, thanks!

Linus

> Hey Linus,
> 
> My email server has been firewalled from vger.kernel.org until today,
> so I didn't see the original until this v2 was sent today. My
> apologies. I'll review this first thing on Monday.
> 
> Jason
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] wireguard: netlink: add multicast notification for peer changes
  2021-01-15 23:40   ` Jason A. Donenfeld
  2021-01-16  1:51     ` Linus Lotz
@ 2021-01-26 20:40     ` Linus Lotz
  1 sibling, 0 replies; 8+ messages in thread
From: Linus Lotz @ 2021-01-26 20:40 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: kernel test robot, David S. Miller, Jakub Kicinski,
	WireGuard mailing list, Netdev, LKML

Hi Jason,
have you had a chance to look at it yet?

Cheers
Linus

Am 16.01.21 um 00:40 schrieb Jason A. Donenfeld:
> Hey Linus,
> 
> My email server has been firewalled from vger.kernel.org until today,
> so I didn't see the original until this v2 was sent today. My
> apologies. I'll review this first thing on Monday.
> 
> Jason
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] wireguard: netlink: add multicast notification for peer changes
  2021-01-09 21:00 [PATCH] wireguard: netlink: add multicast notification for peer changes Linus Lotz
  2021-01-11 20:48 ` kernel test robot
  2021-01-15 19:53 ` [PATCH v2] " Linus Lotz
@ 2021-03-07 21:49 ` Jason A. Donenfeld
  2021-03-13 13:14   ` Linus Lotz
  2 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2021-03-07 21:49 UTC (permalink / raw)
  To: Linus Lotz
  Cc: David S. Miller, Jakub Kicinski, WireGuard mailing list, Netdev, LKML

Hey Linus,

Thanks for the patch and sorry for the delay in reviewing it. Seeing
the basic scaffolding for getting netlink notifiers working with
WireGuard is enlightening; it looks surprisingly straightforward.

There are three classes of things that are interesting to receive
notifications for:

1) Things that the admin changes locally. This is akin to the `ip
monitor`, in which you can see various things that other programs (or
the kernel) modify. This seems straightforward and uncontroversial.

2) Authenticated events from peers. This basically amounts to new
sessions being created following a successful handshake. This seems
mostly okay because authenticated actions already have various DoS
protections in place.

3) Unauthenticated events. This would be things like (a) your patch --
a peer's endpoint changes -- or, more interestingly, (b) public keys
of unknown peers trying to handshake.

(b) is interesting because it allows doing database lookups in
userspace, adding the looked up entry, adding it to the interface, and
initiating a handshake in the reverse direction using the endpoint
information. It's partially DoS-protected due to the on-demand cookie
mac2 field.

(a) is also interesting for the use cases you outlined, but much more
perilous, as these are highspeed packets where the outer IP header is
totally unauthenticated. What prevents a man in the middle from doing
something nasty there, causing userspace to be inundated with
expensive calls and filling up netlink socket buffers and so forth?

I wonder if this would be something better belonging to (2) -- getting
an event on an authenticated peer handshake -- and combining that with
the ability to disable roaming (something discussed a few times on
wgml). Alternatively, maybe you have a different idea in mind about
this?

I also don't (yet) know much about the efficiency of multicast netlink
events and what the overhead is if nobody is listening, and questions
like that. So I'd be curious to hear your thoughts on the matter.

Regards,
Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] wireguard: netlink: add multicast notification for peer changes
  2021-03-07 21:49 ` [PATCH] " Jason A. Donenfeld
@ 2021-03-13 13:14   ` Linus Lotz
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Lotz @ 2021-03-13 13:14 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: David S. Miller, Jakub Kicinski, WireGuard mailing list, Netdev, LKML

Hi Jason,> Thanks for the patch and sorry for the delay in reviewing it.
Seeing
> the basic scaffolding for getting netlink notifiers working with
> WireGuard is enlightening; it looks surprisingly straightforward.
Glad to hear that this is a welcome feature.
> 
> There are three classes of things that are interesting to receive
> notifications for:
> 
> 1) Things that the admin changes locally. This is akin to the `ip
> monitor`, in which you can see various things that other programs (or
> the kernel) modify. This seems straightforward and uncontroversial.
The current patch will also trigger for admin changes of the endpoint.
This could obviously be extended to other events as well.
> 
> 2) Authenticated events from peers. This basically amounts to new
> sessions being created following a successful handshake. This seems
> mostly okay because authenticated actions already have various DoS
> protections in place.
> 
> 3) Unauthenticated events. This would be things like (a) your patch --
> a peer's endpoint changes -- or, more interestingly, (b) public keys
> of unknown peers trying to handshake.
I was under the impression that this is an authenticated event. The
function 'wg_socket_set_peer_endpoint' where I hook in the notification
is called from:
- set_peer (changes from netlink, authenticated)
- wg_packet_consume_data_done (which should be authenticated?)
- wg_socket_set_peer_endpoint_from_skb
And wg_socket_set_peer_endpoint_from_skb is in turn called from
wg_receive_handshake_packet where it is called after validating a
handshake initiation and after validating a handshake response. So it
should be authenticated, right?

If the endpoint could be updated without authentication I would be
concerned that an attacker could change the stored endpoint and thus do
a DOS on a tunnel, as he could change the endpoints for both peers by
sending them messages from an invalid endpoint.

> 
> (b) is interesting because it allows doing database lookups in
> userspace, adding the looked up entry, adding it to the interface, and
> initiating a handshake in the reverse direction using the endpoint
> information. It's partially DoS-protected due to the on-demand cookie
> mac2 field.
This is indeed an interesting feature. In this case we might want to
keep the handshake information so that we can complete it if the lookup
is successful. Since this would keep some state for unauthenticated
peers it should probably only be used when explicitly enabled. This
could probably also be used to debug tunnel settings.
> 
> (a) is also interesting for the use cases you outlined, but much more
> perilous, as these are highspeed packets where the outer IP header is
> totally unauthenticated. What prevents a man in the middle from doing
> something nasty there, causing userspace to be inundated with
> expensive calls and filling up netlink socket buffers and so forth?
I was assuming it was authenticated, if not, there should definitely be
some counter measures and it should only be enabled manually.

> 
> I wonder if this would be something better belonging to (2) -- getting
> an event on an authenticated peer handshake -- and combining that with
> the ability to disable roaming (something discussed a few times on
> wgml). Alternatively, maybe you have a different idea in mind about
> this?
If wg_socket_set_peer_endpoint is the only place where the endpoint is
modified it would be relatively simple to implement a flag that disables
roaming. In theory it could also be possible to send a notification, but
 not change the endpoint and only let userspace update it. So an
userspace application could decide if the roaming is allowed or not.
> 
> I also don't (yet) know much about the efficiency of multicast netlink
> events and what the overhead is if nobody is listening, and questions
> like that. So I'd be curious to hear your thoughts on the matter.
I do not know how big the overhead is. I was assuming that a change of
the endpoint address is relatively rare so that the impact should be
negligible (since I assumed that changing the endpoint is authenticated.)

Regards,
Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-13 13:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-09 21:00 [PATCH] wireguard: netlink: add multicast notification for peer changes Linus Lotz
2021-01-11 20:48 ` kernel test robot
2021-01-15 19:53 ` [PATCH v2] " Linus Lotz
2021-01-15 23:40   ` Jason A. Donenfeld
2021-01-16  1:51     ` Linus Lotz
2021-01-26 20:40     ` Linus Lotz
2021-03-07 21:49 ` [PATCH] " Jason A. Donenfeld
2021-03-13 13:14   ` Linus Lotz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).