[Openvpn-devel,v2] Improve "recursive routing" warning

Message ID 1527594447-10710-1-git-send-email-lev@openvpn.net
State Superseded
Headers show
Series [Openvpn-devel,v2] Improve "recursive routing" warning | expand

Commit Message

Lev Stipakov May 29, 2018, 1:47 a.m. UTC
V2: style fixes

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

Trac #843

Signed-off-by: Lev Stipakov <lev@openvpn.net>

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

Comments

Gert Doering May 29, 2018, 9:08 a.m. UTC | #1
Hi,

On Tue, May 29, 2018 at 02:47:27PM +0300, Lev Stipakov wrote:
> 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"...



> @@ -1094,7 +1099,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))

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".

gert
Lev Stipakov May 29, 2018, 9:50 a.m. UTC | #2
Hi,


> 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"...
>

I assume that people are confused by the fact of seeing warning,
less by provided numbers.

The patch adds a few more lines of code, but those are needed to provide
more information to fix/workaroud the issue. With

proto saddr:sport -> daddr:dport

it should be easy to figure out what app generates problematic traffic. Also
warning now tells user what to do - I think it is much more useful
comparison
to "someting is wrong" - style message what we have now. There are quite
many posts on internet asking "hey I have this warning, what is that, how
to fix"
and I think this patch helps address some of those.

>          /* 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))
>
> 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".


The "whitespace after cast" thing is inconsistent at the moment, have a
look at proto.c:

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

60         if (BLEN(buf) < (int)(sizeof(struct openvpn_ethhdr)

both of those lines were touched in "The Greate Reformatting - first phase"
commit.

Let's discuss this at the next meeting.

Patch

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 4114f40..262d4ea 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4072,7 +4072,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
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 7d9a338..b121849 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1068,7 +1068,7 @@  read_incoming_tun(struct context *c)
 /**
  * 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.
@@ -1080,6 +1080,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;
@@ -1094,7 +1099,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;
         }
@@ -1105,12 +1110,38 @@  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)
         {
             drop = true;
+
+            /* collect information for warning message */
+            sport = 0;
+            dport = 0;
+
+            saddr = ntohl(pip->saddr);
+            daddr = ntohl(pip->daddr);
+            protocol = pip->protocol;
+
+            /* whole packet? */
+            if (BLEN(buf) == (ip_hdr_offset + (int)(ntohs(pip->tot_len))))
+            {
+                const uint8_t *payload = BPTR(buf) + ip_hdr_offset + sizeof(struct openvpn_iphdr);
+
+                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 if (proto_ver == 6)
@@ -1118,7 +1149,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;
         }
@@ -1129,22 +1160,68 @@  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 */
+            sport = 0;
+            dport = 0;
+
+            saddr6 = pip6->saddr;
+            daddr6 = pip6->daddr;
+            protocol = pip6->nexthdr;
+
+            /* whole packet? */
+            if (BLEN(buf) == ((int)sizeof(struct openvpn_ipv6hdr) + ip_hdr_offset + (int)ntohs(pip6->payload_len)))
+            {
+                const uint8_t *payload = BPTR(buf) + ip_hdr_offset + sizeof(struct openvpn_ipv6hdr);
+
+                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);
+                }
+            }
         }
     }
 
     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, "%s", "UDP");
+        else if (protocol == OPENVPN_IPPROTO_TCP)
+            buf_printf(&addrs_buf, "%s", "TCP");
+        else
+            buf_printf(&addrs_buf, "%d", protocol);
+
+        if (proto_ver == 4)
+        {
+            buf_printf(&addrs_buf, " %s:%d -> %s:%d",
+                       print_in_addr_t(saddr, 0, &gc), sport,
+                       print_in_addr_t(daddr, 0, &gc), dport);
+        } else if (proto_ver == 6)
+        {
+            buf_printf(&addrs_buf, " %s:%d -> %s:%d",
+                       print_in6_addr(saddr6, 0, &gc), sport,
+                       print_in6_addr(daddr6, 0, &gc), dport);
+        }
+
+        msg(D_LOW, "Recursive routing detected, drop packet %s. Fix your routing or consider using --allow-recursive-routing option.", BSTR(&addrs_buf));
         gc_free(&gc);
     }
 }