allowed-ips: separation of concerns, routing and firewalling

[Kim Nilsson <kim@wayoftao.net>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3173 bytes --]

Hello wireguard project,


I am currently working on several projects that make use of wireguard as 
part of a larger networking scheme. Since there are many details about 
tunneling, network routing, and firewalling that are considered 
must-know for many of my coworkers I recently had to make a presentation 
on how packets move through the network stack and, for example, how they 
end up on the receiving end of a wireguard tunnel. During the 
presentation a question arose on what happens when an IP packet is 
routed through a gateway and what wireguard does.


When an IP packet is to be sent over e.g. ethernet, the link layer 
destination address is usually discovered using ARP. In the case of 
wireguard, a lookup is performed into a table which maps the entries of 
allowed-ips to their corresponding wireguard peer. This behavior is 
relatively straightforward and does what one would expect from a link layer.


However, when an IP packet is to be routed through a gateway the 
interaction with link layer processes such ARP is usually performed 
using the gateway address as opposed to the actual destination address 
of the packet (in Linux this is attached to a given skb as dst info). 
 From what I understand, wireguard completely ignores the presence of 
such routing information and instead requires the user to manually 
populate a particular peer with all possible destination addresses. In 
effect, the concern of packet routing is placed inside the wireguard 
implementation instead of being left to the routing subsystem.


As for possible motivations for this design choice, I can think of 
security as one that could be considered motivating enough - A packet 
cannot travel to a wireguard peer unless its destination address is in 
the set of allowed-ips. Looking at the implementation it is also evident 
that wireguard also will not receive packets with source addresses not 
present in allowed-ips (and complain that there is a "dishonest peer").

However, is this not also a case of the concerns of another subsystem 
viz. the firewall being placed inside the wireguard implementation? As 
it stands, what is traditionally considered routing and firewall 
information has to be shared with wireguard in order to maintain a 
working tunnel.


Would it not be more reasonable if wireguard acted as a common link 
layer and respected the boundaries of internet layer routing and 
firewalling? To this effect I have created and tested a small patch 
which does two things, namely;

1. Checks for the presence of routing information on outgoing payloads 
and, if present, uses the specified gateway address as input to the peer 
lookup.

2. Removes the restriction w.r.t. the source address of incoming payloads.


I'm sure it is not possible at this stage to just fundamentally alter 
the semantics of allowed-ips, but, if you agree with my observations, 
perhaps the patch can serve as the foundation of something new which can 
begin to deprecatee allowed-ips as we know it today?


Regards,

Kim Nilsson


P.S.

Apologies if this has already been discussed before.



[-- Attachment #1.1.2: wireguard_routing.patch --]
[-- Type: text/x-patch, Size: 3627 bytes --]

Index: linux/drivers/net/wireguard/allowedips.c
===================================================================
--- linux.orig/drivers/net/wireguard/allowedips.c
+++ linux/drivers/net/wireguard/allowedips.c
@@ -5,6 +5,8 @@
 
 #include "allowedips.h"
 #include "peer.h"
+#include "net/dst.h"
+#include "net/route.h"
 
 enum { MAX_ALLOWEDIPS_DEPTH = 129 };
 
@@ -356,6 +358,15 @@ int wg_allowedips_read_node(struct allow
 struct wg_peer *wg_allowedips_lookup_dst(struct allowedips *table,
 					 struct sk_buff *skb)
 {
+	const struct dst_entry *dst = skb_dst(skb);
+	if (dst) {
+		const struct rtable *rt = container_of(dst, struct rtable, dst);
+		if (rt->rt_gw_family == AF_INET)
+			return lookup(table->root4, 32, &rt->rt_gw4);
+		else if (rt->rt_gw_family == AF_INET6)
+			return lookup(table->root6, 128, &rt->rt_gw6);
+	}
+
 	if (skb->protocol == htons(ETH_P_IP))
 		return lookup(table->root4, 32, &ip_hdr(skb)->daddr);
 	else if (skb->protocol == htons(ETH_P_IPV6))
@@ -363,17 +374,6 @@ struct wg_peer *wg_allowedips_lookup_dst
 	return NULL;
 }
 
-/* Returns a strong reference to a peer */
-struct wg_peer *wg_allowedips_lookup_src(struct allowedips *table,
-					 struct sk_buff *skb)
-{
-	if (skb->protocol == htons(ETH_P_IP))
-		return lookup(table->root4, 32, &ip_hdr(skb)->saddr);
-	else if (skb->protocol == htons(ETH_P_IPV6))
-		return lookup(table->root6, 128, &ipv6_hdr(skb)->saddr);
-	return NULL;
-}
-
 int __init wg_allowedips_slab_init(void)
 {
 	node_cache = KMEM_CACHE(allowedips_node, 0);
Index: linux/drivers/net/wireguard/receive.c
===================================================================
--- linux.orig/drivers/net/wireguard/receive.c
+++ linux/drivers/net/wireguard/receive.c
@@ -338,7 +338,6 @@ static void wg_packet_consume_data_done(
 {
 	struct net_device *dev = peer->device->dev;
 	unsigned int len, len_before_trim;
-	struct wg_peer *routed_peer;
 
 	wg_socket_set_peer_endpoint(peer, endpoint);
 
@@ -401,24 +400,10 @@ static void wg_packet_consume_data_done(
 	if (unlikely(pskb_trim(skb, len)))
 		goto packet_processed;
 
-	routed_peer = wg_allowedips_lookup_src(&peer->device->peer_allowedips,
-					       skb);
-	wg_peer_put(routed_peer); /* We don't need the extra reference. */
-
-	if (unlikely(routed_peer != peer))
-		goto dishonest_packet_peer;
-
 	napi_gro_receive(&peer->napi, skb);
 	update_rx_stats(peer, message_data_len(len_before_trim));
 	return;
 
-dishonest_packet_peer:
-	net_dbg_skb_ratelimited("%s: Packet has unallowed src IP (%pISc) from peer %llu (%pISpfsc)\n",
-				dev->name, skb, peer->internal_id,
-				&peer->endpoint.addr);
-	++dev->stats.rx_errors;
-	++dev->stats.rx_frame_errors;
-	goto packet_processed;
 dishonest_packet_type:
 	net_dbg_ratelimited("%s: Packet is neither ipv4 nor ipv6 from peer %llu (%pISpfsc)\n",
 			    dev->name, peer->internal_id, &peer->endpoint.addr);
Index: linux/drivers/net/wireguard/allowedips.h
===================================================================
--- linux.orig/drivers/net/wireguard/allowedips.h
+++ linux/drivers/net/wireguard/allowedips.h
@@ -43,11 +43,9 @@ void wg_allowedips_remove_by_peer(struct
 /* The ip input pointer should be __aligned(__alignof(u64))) */
 int wg_allowedips_read_node(struct allowedips_node *node, u8 ip[16], u8 *cidr);
 
-/* These return a strong reference to a peer: */
+/* Returns a strong reference to a peer: */
 struct wg_peer *wg_allowedips_lookup_dst(struct allowedips *table,
 					 struct sk_buff *skb);
-struct wg_peer *wg_allowedips_lookup_src(struct allowedips *table,
-					 struct sk_buff *skb);
 
 #ifdef DEBUG
 bool wg_allowedips_selftest(void);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]