Improve "recursive routing" warning

V2: style fixes

- print protocol, source/dest addresses and ports
- mention "--allow-recursive-routing"
- add possible usecase to manpage

Trac #843

I'm not sure I'm convinced. This is quite a lot of extra code for an "add a few extra fields to a warning" enhancement - especially given that those that are already confused by the warning today will be more confused by "more numbers"...

         /* make sure we got whole IP header */

Quoted 1 line

- if (BLEN(buf) < ((int) sizeof(struct openvpn_iphdr) +

ip_hdr_offset))

Quoted 1 line

+ if (BLEN(buf) < ((int)sizeof(struct openvpn_iphdr) +

ip_hdr_offset))

In addition, these changes look funny. The existing code style was produced
by the scripted big code cleanup for 2.4, so there should never be a reason to change the style in a patch for "master".

From: Lev Stipakov <[email protected]...>

This patch provides additional information, such as source address/port and destination address/port, to a "recursive routing" warning message. It also mentiones possible workaround.

Trac #843

Signed-off-by Lev Stipakov <[email protected]...>

---
v4:
- remove unneeded format specifier for const string

v3:
- factor out ports extraction code into own function to avoid code duplication
- better commit message

v2:
- print protocol, source/dest addresses and ports
- mention "--allow-recursive-routing"
- add possible usecase to manpage

 doc/openvpn.8         |  4 ++-
 src/openvpn/forward.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 93 insertions(+), 9 deletions(-)

doc/openvpn.8 +3 -1

index 94b5cc4..fb9eb84 100644
@@ -4090,7 +4090,9 @@ notifications unless this option is enabled.
 .TP
 .B \-\-allow\-recursive\-routing
 When this option is set, OpenVPN will not drop incoming tun packets
-with same destination as host.
+with same destination as host. Could be useful when packets sent by openvpn
+itself are not subject to the routing tables that would move packets
+into the tunnel.
 .\"*********************************************************
 .SS Data Channel Encryption Options:
 These options are meaningful for both Static & TLS\-negotiated key modes

src/openvpn/forward.c +41 -4

index f8faa81..e96c546 100644
@@ -1291,10 +1291,42 @@ read_incoming_tun(struct context *c)
     perf_pop();
 }
 
+/*
+ * Extracts sport and dport from packet, used
+ * in recursive routing warning.
+ */
+static void
+extract_sport_dport(const struct buffer *buf, uint8_t protocol, int offset_to_payload, int packet_len, uint16_t *sport, uint16_t *dport)
+{
+    /* whole packet? */
+    if (BLEN(buf) == packet_len)
+    {
+        const uint8_t *payload = BPTR(buf) + offset_to_payload;
+
+        if (protocol == OPENVPN_IPPROTO_UDP)
+        {
+            const struct openvpn_udphdr *udp = (const struct openvpn_udphdr *)payload;
+            *sport = ntohs(udp->source);
+            *dport = ntohs(udp->dest);
+        }
+        else if (protocol == OPENVPN_IPPROTO_TCP)
+        {
+            const struct openvpn_tcphdr *tcp = (const struct openvpn_tcphdr *)payload;
+            *sport = ntohs(tcp->source);
+            *dport = ntohs(tcp->dest);
+        }
+    }
+    else
+    {
+        *sport = 0;
+        *dport = 0;
+    }
+}
+
 /**
  * Drops UDP packets which OS decided to route via tun.
  *
- * On Windows and OS X when netwotk adapter is disabled or
+ * On Windows and OS X when network adapter is disabled or
  * disconnected, platform starts to use tun as external interface.
  * When packet is sent to tun, it comes to openvpn, encapsulated
  * and sent to routing table, which sends it again to tun.

@@ -1306,6 +1338,11 @@ drop_if_recursive_routing(struct context *c, struct buffer *buf)
     struct openvpn_sockaddr tun_sa;
     int ip_hdr_offset = 0;
 
+    uint32_t saddr, daddr;
+    struct in6_addr saddr6, daddr6;
+    uint16_t sport, dport;
+    uint8_t protocol;
+
     if (c->c2.to_link_addr == NULL) /* no remote addr known */
     {
         return;

@@ -1320,7 +1357,7 @@ drop_if_recursive_routing(struct context *c, struct buffer *buf)
         const struct openvpn_iphdr *pip;
 
         /* make sure we got whole IP header */
-        if (BLEN(buf) < ((int) sizeof(struct openvpn_iphdr) + ip_hdr_offset))
+        if (BLEN(buf) < ((int)sizeof(struct openvpn_iphdr) + ip_hdr_offset))
         {
             return;
         }

@@ -1331,12 +1368,21 @@ drop_if_recursive_routing(struct context *c, struct buffer *buf)
             return;
         }
 
-        pip = (struct openvpn_iphdr *) (BPTR(buf) + ip_hdr_offset);
+        pip = (const struct openvpn_iphdr *)(BPTR(buf) + ip_hdr_offset);
 
-        /* drop packets with same dest addr as gateway */
+        /* drop packets with the same dest addr as gateway */
         if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
         {

src/openvpn/forward.c +30 -4

         }
     }
     else if (proto_ver == 6)

@@ -1344,7 +1390,7 @@ drop_if_recursive_routing(struct context *c, struct buffer *buf)
         const struct openvpn_ipv6hdr *pip6;
 
         /* make sure we got whole IPv6 header */
-        if (BLEN(buf) < ((int) sizeof(struct openvpn_ipv6hdr) + ip_hdr_offset))
+        if (BLEN(buf) < ((int)sizeof(struct openvpn_ipv6hdr) + ip_hdr_offset))
         {
             return;
         }

@@ -1355,22 +1401,58 @@ drop_if_recursive_routing(struct context *c, struct buffer *buf)
             return;
         }
 
-        /* drop packets with same dest addr as gateway */
+        /* drop packets with the same dest addr as gateway */
         pip6 = (struct openvpn_ipv6hdr *) (BPTR(buf) + ip_hdr_offset);
         if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
         {
             drop = true;
+
+            /* collect information for warning message */
+
+            saddr6 = pip6->saddr;
+            daddr6 = pip6->daddr;
+            protocol = pip6->nexthdr;
+
+            extract_sport_dport(buf, protocol, ip_hdr_offset + sizeof(struct openvpn_ipv6hdr),
+                                (int)sizeof(struct openvpn_ipv6hdr) + ip_hdr_offset + (int)ntohs(pip6->payload_len),
+                                &sport, &dport);
         }
     }
 
     if (drop)
     {
         struct gc_arena gc = gc_new();
+        struct buffer addrs_buf = alloc_buf_gc(128, &gc);
 
+        /* drop packet */
         c->c2.buf.len = 0;
 
-        msg(D_LOW, "Recursive routing detected, drop tun packet to %s",
-            print_link_socket_actual(c->c2.to_link_addr, &gc));
+        if (protocol == OPENVPN_IPPROTO_UDP)
+        {
+            buf_printf(&addrs_buf, "UDP");
+        }
+        else if (protocol == OPENVPN_IPPROTO_TCP)
+        {
+            buf_printf(&addrs_buf, "TCP");
+        }
+        else
+        {
+            buf_printf(&addrs_buf, "%d", protocol);
+        }
+
+        if (proto_ver == 4)
+        {
+            buf_printf(&addrs_buf, " %s:%d -> %s:%d",

src/openvpn/forward.c +0 -0

         gc_free(&gc);
     }
 }

Hi,

Review

Quoted 4 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

(quoted diff) +2 -0

+
+        msg(D_LOW, "Recursive routing detected, drop packet %s. Fix your routing or consider using --allow-recursive-routing option.", BSTR(&addrs_buf));

I would add "if you know what you are doing", otherwisethe average Joe will think that he can just add "--allow-recursive-routing" to his config and the problem is fixed.

Hi,

Quoted 5 lines · Antonio Quartulli

Hi,

Quoted 2 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

Quoted 8 lines

Quoted 3 lines

+
+        msg(D_LOW, "Recursive routing detected, drop packet %s. Fix your routing or consider using --allow-recursive-routing option.", BSTR(&addrs_buf));

I would add "if you know what you are doing", otherwisethe average Joe will think that he can just add "--allow-recursive-routing" to his config and the problem is fixed.

"If you know what you are doing" is no help
because they all "_know what they are doing_" ...

My understanding is as follows:

Recursive-routing means that I send encrypted packets, which are destined for the public IP of the peer address, over the tunnel. Instead of over the internet (or other common network).

Considering the loops which the receiver will have to jump though to accept these packets, I question the validity of documenting this "feature" --allow-recursive-routing, at all.

Perhaps this would be better suited as:
./configure --enable-recursive-routing
make

From: Lev Stipakov <[email protected]...>

This patch provides additional information, such as source address/port and destination address/port, to a "recursive routing" warning message. It also mentiones possible workaround.

Now... if we consider a scenario where OpenVPN packets are not subject to be routed into the tunnel (Linux VRF, policy routing, ...) - which is actually something I want to see happen :-) - twisting this feature into some other direction might make the coding effort useful: what about "we only block packets that match destination IP *and port and protocol* with what OpenVPN is using"?

So, if we talk to 1.2.3.4/udp/1194, only packets inside the tunnel destined to 1.2.3.4/udp/1994 would be dropped, and everything else can be sent freely - because those are never "recursive openvpn packets".

We have two options now:
1) extend documentation (basically what part of this patch is doing);
2) rework this feature entirely.

If we go with 2 I guess we don't even need 1.

I'd go with 2, because this feature as it is now is not really meaningful to me.

Hi,

Quoted 7 lines · Gert Doering

Now... if we consider a scenario where OpenVPN packets are not subject to be routed into the tunnel (Linux VRF, policy routing, ...) - which is actually something I want to see happen :-) - twisting this feature into some other direction might make the coding effort useful: what about "we only block packets that match destination IP *and port and protocol* with what OpenVPN is using"?

Quoted 3 lines

So, if we talk to 1.2.3.4/udp/1194, only packets inside the tunnel destined to 1.2.3.4/udp/1994 would be dropped, and everything else can be sent freely - because those are never "recursive openvpn packets".

I was just questioning this feature per se: why do we want to *allow* real loops?

On Android where VPN setup is a bit different from normal setup. Different enoguh that the recursive routing message is easily triggered and the client always sets the allow-recursive-roouting option. I cannot remember the exact details anymore.