openvpn-devel

[Openvpn-devel] [PATCH] Drop recursively routed packets

[Lev S. <lst...@gm...>]

On certain OSes (Windows, OS X) when network adapter is
disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
operating system starts to use tun as an external interface.
Outgoing packets are routed to tun, UDP encapsulated, given to
routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power,
traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as
destination address.

Tested on Win7/10, OS X.

Trac #642

Signed-off-by: Lev Stipakov <lst...@gm...>
---
 src/openvpn/forward.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 36a99e6..05445a1 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -973,6 +973,68 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun. 
+ * 
+ * On Windows and OS X when netwotk 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.
+ */
+static void
+drop_recursive (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;
+
+  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) BPTR (buf);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (is_ipv6 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with same dest addr as gateway */
+      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
+      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
+	drop = true;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      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));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link
@@ -998,6 +1060,7 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      drop_recursive (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).
-- 
1.9.1

[Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Lev S. <lst...@gm...>]

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is
disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
operating system starts to use tun as an external interface.
Outgoing packets are routed to tun, UDP encapsulated, given to
routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power,
traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as
destination address.

Tested on Win7/10, OS X.

Trac #642

Signed-off-by: Lev Stipakov <lst...@gm...>
---
 src/openvpn/forward.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 36a99e6..af05bd0 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -973,6 +973,68 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun. 
+ * 
+ * On Windows and OS X when netwotk 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.
+ */
+static void
+drop_if_recursive_routing (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;
+
+  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) BPTR (buf);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (is_ipv6 (TUNNEL_TYPE (c->c1.tuntap), buf))
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with same dest addr as gateway */
+      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
+      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
+	drop = true;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      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));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link
@@ -998,6 +1060,7 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      drop_if_recursive_routing (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).
-- 
1.9.1

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Mon, Jan 04, 2016 at 02:43:44PM +0200, Lev Stipakov wrote:
> v2: better method naming
> 
> On certain OSes (Windows, OS X) when network adapter is
> disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
> operating system starts to use tun as an external interface.
> Outgoing packets are routed to tun, UDP encapsulated, given to
> routing table and sent to.. tun.
> 
> As a consequence, system starts talking to itself on full power,
> traffic counters skyrocket and user is not happy.
> 
> To prevent that, drop packets which have gateway IP as
> destination address.
> 
> Tested on Win7/10, OS X.
> 
> Trac #642
> 
> Signed-off-by: Lev Stipakov <lst...@gm...>

ACK.

Thanks for coding this, thanks to Valdikss for testing (David, can
you include the tested-by: in the commit?).

Code still looks good, as in "does what is advertised, and safely does so".


Per discussion with David, we're not totally happy with the performance
implications - but that's not your fault, our is_ipv4() and is_ipv6()
functions are really doing their job in a very non-optimized way (read:
all the work is done twice, once to find out whether it's v4, then 
again to find out whether it's v6).

I volunteer to refactor the two places where is_ipv*() are called today
(thinking along the lines of "ip_version = get_ip_version(...)" and then
compare ip_version to "4" or "6" - plus, making this an inline function)

For now, we stick to "fixes the issue in a easy to read and non-intrusive
way", so it can go into 2.3.12.

thanks again :)

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Mon, Aug 22, 2016 at 09:18:28PM +0200, Gert Doering wrote:
> On Mon, Jan 04, 2016 at 02:43:44PM +0200, Lev Stipakov wrote:
> > v2: better method naming
[..]
> > Trac #642
> > 
> > Signed-off-by: Lev Stipakov <lst...@gm...>
> 
> ACK.

As stupid as this feels - we need to back this out again, because it
breaks TAP mode.  Buildbot complained that all tap tests failed, and
manually bisecting master nailed it to *this* patch, and release/2.3
is similarily broken.

On the server side, for a t_client test with --dev tap, with this patch,
you see "the source mac is rotating"

Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d8:7a -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:d7:c1 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:fc:a0], packet dropped
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:3e:b1 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:3d:f8 -> cron2-gentoo-i386/193.xx.xx.xx
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:62:d7], packet dropped
Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d7:d8 -> cron2-gentoo-i386/193.xx.xx.xx

... which looks like "the IP header ends up where the ethernet header
should be" (every ping packet shows up as "new source address" on the
openvpn server).

I have no idea what this could be, but since we want 2.3.12 out *today*,
we'll need to back it out of 2.3 for the time being.

Lev, do you have time to investigate?

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

[Openvpn-devel] [PATCH applied] Re: Drop recursively routed packets

[David S. <da...@op...>]

Your patch has been applied to the following branches

commit e9d64bc03742c96a3d7fe2a473c43d40e5ba2001  (master)
commit 122469f5ad30b563cbefbc753d2a55af4227bb74  (release/2.3)
Author: Lev Stipakov
Date:   Mon Jan 4 14:43:44 2016 +0200

     Drop recursively routed packets

     Signed-off-by: Lev Stipakov <lst...@gm...>
     Trac: 642
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <145...@gm...>
     URL: https://sourceforge.net/p/openvpn/mailman/message/34737757/
     Signed-off-by: David Sommerseth <da...@op...>


--
kind regards,

David Sommerseth

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[ValdikSS <ia...@va...>]

I'd like to see this applied if possible.

On 04.01.2016 15:43, Lev Stipakov wrote:
> v2: better method naming
>
> On certain OSes (Windows, OS X) when network adapter is
> disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
> operating system starts to use tun as an external interface.
> Outgoing packets are routed to tun, UDP encapsulated, given to
> routing table and sent to.. tun.
>
> As a consequence, system starts talking to itself on full power,
> traffic counters skyrocket and user is not happy.
>
> To prevent that, drop packets which have gateway IP as
> destination address.
>
> Tested on Win7/10, OS X.
>
> Trac #642
>
> Signed-off-by: Lev Stipakov <lst...@gm...>
> ---
>  src/openvpn/forward.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 36a99e6..af05bd0 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -973,6 +973,68 @@ read_incoming_tun (struct context *c)
>    perf_pop ();
>  }
>  
> +/**
> + * Drops UDP packets which OS decided to route via tun. 
> + * 
> + * On Windows and OS X when netwotk 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.
> + */
> +static void
> +drop_if_recursive_routing (struct context *c, struct buffer *buf)
> +{
> +  bool drop = false;
> +  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;
> +
> +  if (is_ipv4 (TUNNEL_TYPE (c->c1.tuntap), buf))
> +    {
> +      const struct openvpn_iphdr *pip;
> +
> +      /* make sure we got whole IP header */
> +      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
> +	return;
> +
> +      /* skip ipv4 packets for ipv6 tun */
> +      if (tun_sa.addr.sa.sa_family != AF_INET)
> +	return;
> +
> +      pip = (struct openvpn_iphdr *) BPTR (buf);
> +
> +      /* drop packets with same dest addr as gateway */
> +      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
> +	drop = true;
> +    }
> +  else if (is_ipv6 (TUNNEL_TYPE (c->c1.tuntap), buf))
> +    {
> +      const struct openvpn_ipv6hdr *pip6;
> +
> +      /* make sure we got whole IPv6 header */
> +      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
> +	return;
> +
> +      /* skip ipv6 packets for ipv4 tun */
> +      if (tun_sa.addr.sa.sa_family != AF_INET6)
> +	return;
> +
> +      /* drop packets with same dest addr as gateway */
> +      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
> +      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
> +	drop = true;
> +    }
> +
> +  if (drop)
> +    {
> +      struct gc_arena gc = gc_new ();
> +
> +      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));
> +      gc_free (&gc);
> +    }
> +}
> +
>  /*
>   * Input:  c->c2.buf
>   * Output: c->c2.to_link
> @@ -998,6 +1060,7 @@ process_incoming_tun (struct context *c)
>  
>    if (c->c2.buf.len > 0)
>      {
> +      drop_if_recursive_routing (c, &c->c2.buf);
>        /*
>         * The --passtos and --mssfix options require
>         * us to examine the IP header (IPv4 or IPv6).

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Wed, Apr 06, 2016 at 02:37:13PM +0300, ValdikSS wrote:
> I'd like to see this applied if possible.

Is this a "I have tested it and it works" confirmation, or a "folks, go
out and do work!"?

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[ValdikSS <ia...@va...>]

More like a notification not to forget about it. I'll test it tomorrow and write the results.

On 06.04.2016 17:51, Gert Doering wrote:
> Hi,
>
> On Wed, Apr 06, 2016 at 02:37:13PM +0300, ValdikSS wrote:
> Is this a "I have tested it and it works" confirmation, or a "folks, go
> out and do work!"?
>
> gert
>

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Wed, Apr 06, 2016 at 11:44:34PM +0300, ValdikSS wrote:
> More like a notification not to forget about it. I'll test it tomorrow and write the results.
> 
> On 06.04.2016 17:51, Gert Doering wrote:
> > Hi,
> >
> > On Wed, Apr 06, 2016 at 02:37:13PM +0300, ValdikSS wrote:
> > Is this a "I have tested it and it works" confirmation, or a "folks, go
> > out and do work!"?

Coming back to this patch set - could you share your test results?

I'll then go and see that I can code review & merge during the next week.

gert


-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[ValdikSS <ia...@va...>]

Tested on Windows 10 and Linux. Works as intended.
I've connected to the VPN which redirects the gateway and physically unplugged Ethernet cable. Git version started to consume 100% CPU while patched version
either doesn't consume CPU and doesn't print anything or doesn't consume CPU and print 5-10 messages per second to the log with verb 4.

On 06/11/2016 02:05 PM, Gert Doering wrote:
> Hi,
>
> On Wed, Apr 06, 2016 at 11:44:34PM +0300, ValdikSS wrote:
> Coming back to this patch set - could you share your test results?
>
> I'll then go and see that I can code review & merge during the next week.
>
> gert
>
>
>
>
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity 
> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[David S. <op...@sf...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/01/16 13:43, Lev Stipakov wrote:
> v2: better method naming
> 
> On certain OSes (Windows, OS X) when network adapter is disabled
> (ethernet cable pulled off, Wi-Fi hardware switch disabled), 
> operating system starts to use tun as an external interface. 
> Outgoing packets are routed to tun, UDP encapsulated, given to 
> routing table and sent to.. tun.

Can this happen on Linux or *BSD?  Could this recursive routing
actually happen at all?

[...snip...]

> + +      /* skip ipv4 packets for ipv6 tun */ +      if
> (tun_sa.addr.sa.sa_family != AF_INET) +	   return;

[...snip...]

> + +      /* skip ipv6 packets for ipv4 tun */ +      if
> (tun_sa.addr.sa.sa_family != AF_INET6) +	  return;

How likely is it that these two checks will hit?  Do we truly need
them?  I'm basically concerned of the potential cost an extra check
adds.  As the link speed increases, such checks will have an impact
later on if now.  If truly needed, is it worth looking into providing
some branch prediction hints to the compiler?  (unlikely/likely hints)

> if (c->c2.buf.len > 0) { +      drop_if_recursive_routing (c,
> &c->c2.buf);

If this is truly only an issue on Windows and OSX, can we consider to
#ifdef the drop_if_recursive_routing() call?


- -- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAleuFGMACgkQDC186MBRfrphqACcCJ7UNTcaFM79njJ1HXFpw7hX
NbcAn2MgNCVh1joOOKhPo46dJ+5GAGdq
=xaHi
-----END PGP SIGNATURE-----

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Fri, Aug 12, 2016 at 08:24:35PM +0200, David Sommerseth wrote:
> On 04/01/16 13:43, Lev Stipakov wrote:
> > v2: better method naming
> > 
> > On certain OSes (Windows, OS X) when network adapter is disabled
> > (ethernet cable pulled off, Wi-Fi hardware switch disabled), 
> > operating system starts to use tun as an external interface. 
> > Outgoing packets are routed to tun, UDP encapsulated, given to 
> > routing table and sent to.. tun.
> 
> Can this happen on Linux or *BSD?  Could this recursive routing
> actually happen at all?

Of course :-) - the moment you lose your external interface, for
example, because you unplug the network cable and network manager
deconfigures ip address plus all routes via that interface...

What is left is "routes into the tunnel", so you send the tunneled
packets into the tunnel again.  Without that patch, this leads to
"openvpn eats 100% cpu, your VPN is down, and until ping-timeout
fires, it won't recover and the user has no idea why it is broken".

Very typical problem with routing and tunnels :-)

> 
> > + +      /* skip ipv4 packets for ipv6 tun */ +      if
> > (tun_sa.addr.sa.sa_family != AF_INET) +	   return;
> 
> [...snip...]
> 
> > + +      /* skip ipv6 packets for ipv4 tun */ +      if
> > (tun_sa.addr.sa.sa_family != AF_INET6) +	  return;
> 
> How likely is it that these two checks will hit?  Do we truly need
> them?  

It's quite likely for IPv6 packets to be transported over an IPv4
tunnel, and vice versa, and will be even *more* likely for the next
10 years to come.

So comparing "inside IPv6 address" to "outside IPv4 address" needs
to be avoided (at best, the code might misfire, at worst, you might
overrun comparing an IPv6 address to something that has not allocated
enough bytes) - maybe it can be written more efficient, but in the end
you always have the "is it the same protocol inside and outside?"
check, before you go on comparing addresses.

Right now, the code is nicely readable by using the is_ipv4() macro
etc., but sacrificing this by having a check very early on that
is the unrolled form of the macro, and basically does

  if ( outside_address_family != inside_address_family ) { return; }

would indeed be more efficient for the mixed-family case.  

OTOH, you need another comparison then for "is it IPv4?  is it IPv6?", 
so the overall amount of code for the same-family case might be even
higher.

OTOH again, is_ipv4() boils down to an amazing amount of code...  so
moving *that* from proto.c to proto.h and make it an inline function
(and hope that the compiler will be smart enough to keep track of all
the then-duplicate checks for is_ipv4() vs. is_ipv6() and just optimize
it properly) might produce more efficient *and* more readable code :-)
(and since it's only called in two places ever, the extra code size
due to inlining isn't big)


> I'm basically concerned of the potential cost an extra check
> adds.  As the link speed increases, such checks will have an impact
> later on if now.  If truly needed, is it worth looking into providing
> some branch prediction hints to the compiler?  (unlikely/likely hints)

For the "protocol variants", you can't, because it depends on what
users do with their tunnels.  Today, they might be predominantly
IPv4-over-IPv4, while my personal sessions are mostly IPv6-over-IPv4
already...

> > if (c->c2.buf.len > 0) { +      drop_if_recursive_routing (c,
> > &c->c2.buf);
> 
> If this is truly only an issue on Windows and OSX, can we consider to
> #ifdef the drop_if_recursive_routing() call?

It can happen on all platforms where "inside" and "outside" routing 
(wrt openvpn) happens in the same routing context / namespace / ... and
"some external entity" can modify the "outside" routing table without
OpenVPN knowing.  In other words: on all platforms we support today.

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Lev S. <lst...@gm...>]

Hi,

I could probably add that patch has been in our production for a while
(several months), OS X / Win, works as expected.

-Lev

2016-08-13 12:58 GMT+03:00 Gert Doering <ge...@gr...>:

> Hi,
>
> On Fri, Aug 12, 2016 at 08:24:35PM +0200, David Sommerseth wrote:
> > On 04/01/16 13:43, Lev Stipakov wrote:
> > > v2: better method naming
> > >
> > > On certain OSes (Windows, OS X) when network adapter is disabled
> > > (ethernet cable pulled off, Wi-Fi hardware switch disabled),
> > > operating system starts to use tun as an external interface.
> > > Outgoing packets are routed to tun, UDP encapsulated, given to
> > > routing table and sent to.. tun.
> >
> > Can this happen on Linux or *BSD?  Could this recursive routing
> > actually happen at all?
>
> Of course :-) - the moment you lose your external interface, for
> example, because you unplug the network cable and network manager
> deconfigures ip address plus all routes via that interface...
>
> What is left is "routes into the tunnel", so you send the tunneled
> packets into the tunnel again.  Without that patch, this leads to
> "openvpn eats 100% cpu, your VPN is down, and until ping-timeout
> fires, it won't recover and the user has no idea why it is broken".
>
> Very typical problem with routing and tunnels :-)
>
> >
> > > + +      /* skip ipv4 packets for ipv6 tun */ +      if
> > > (tun_sa.addr.sa.sa_family != AF_INET) +        return;
> >
> > [...snip...]
> >
> > > + +      /* skip ipv6 packets for ipv4 tun */ +      if
> > > (tun_sa.addr.sa.sa_family != AF_INET6) +      return;
> >
> > How likely is it that these two checks will hit?  Do we truly need
> > them?
>
> It's quite likely for IPv6 packets to be transported over an IPv4
> tunnel, and vice versa, and will be even *more* likely for the next
> 10 years to come.
>
> So comparing "inside IPv6 address" to "outside IPv4 address" needs
> to be avoided (at best, the code might misfire, at worst, you might
> overrun comparing an IPv6 address to something that has not allocated
> enough bytes) - maybe it can be written more efficient, but in the end
> you always have the "is it the same protocol inside and outside?"
> check, before you go on comparing addresses.
>
> Right now, the code is nicely readable by using the is_ipv4() macro
> etc., but sacrificing this by having a check very early on that
> is the unrolled form of the macro, and basically does
>
>   if ( outside_address_family != inside_address_family ) { return; }
>
> would indeed be more efficient for the mixed-family case.
>
> OTOH, you need another comparison then for "is it IPv4?  is it IPv6?",
> so the overall amount of code for the same-family case might be even
> higher.
>
> OTOH again, is_ipv4() boils down to an amazing amount of code...  so
> moving *that* from proto.c to proto.h and make it an inline function
> (and hope that the compiler will be smart enough to keep track of all
> the then-duplicate checks for is_ipv4() vs. is_ipv6() and just optimize
> it properly) might produce more efficient *and* more readable code :-)
> (and since it's only called in two places ever, the extra code size
> due to inlining isn't big)
>
>
> > I'm basically concerned of the potential cost an extra check
> > adds.  As the link speed increases, such checks will have an impact
> > later on if now.  If truly needed, is it worth looking into providing
> > some branch prediction hints to the compiler?  (unlikely/likely hints)
>
> For the "protocol variants", you can't, because it depends on what
> users do with their tunnels.  Today, they might be predominantly
> IPv4-over-IPv4, while my personal sessions are mostly IPv6-over-IPv4
> already...
>
> > > if (c->c2.buf.len > 0) { +      drop_if_recursive_routing (c,
> > > &c->c2.buf);
> >
> > If this is truly only an issue on Windows and OSX, can we consider to
> > #ifdef the drop_if_recursive_routing() call?
>
> It can happen on all platforms where "inside" and "outside" routing
> (wrt openvpn) happens in the same routing context / namespace / ... and
> "some external entity" can modify the "outside" routing table without
> OpenVPN knowing.  In other words: on all platforms we support today.
>
> gert
> --
> USENET is *not* the non-clickable part of WWW!
>                                                            //
> www.muc.de/~gert/
> Gert Doering - Munich, Germany
> ge...@gr...
> fax: +49-89-35655025                        ge...@ne...rmatik.tu-
> muenchen.de
>



-- 
-Lev

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Tue, Aug 23, 2016 at 03:43:36PM +0200, Gert Doering wrote:
> ... which looks like "the IP header ends up where the ethernet header
> should be" (every ping packet shows up as "new source address" on the
> openvpn server).
> 
> I have no idea what this could be, but since we want 2.3.12 out *today*,
> we'll need to back it out of 2.3 for the time being.
> 
> Lev, do you have time to investigate?

is_ipv_X() in proto.c is to blame here, as it breaks the buffer 
for tap mode...

...
  /* IP version is stored in the same bits for IPv4 or IPv6 header */
  if (OPENVPN_IPH_GET_VER (ih->version_len) == ip_ver)
    return buf_advance (buf, offset);
...

... and "offset" is only set != 0 in tap mode - so in tun mode, it works,
and the code itself looks totally reasonable.

David, please revert both release/2.3 and master, and we'll need to 
rethink this, not using "is_ipv4()"...

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Tue, Aug 23, 2016 at 03:49:55PM +0200, Gert Doering wrote:
> David, please revert both release/2.3 and master, and we'll need to 
> rethink this, not using "is_ipv4()"...

Ow, ow, this patch is not good...

Turns out it also breaks p2p mode...

Tue Aug 23 21:28:07 2016 us=528530 UDP link local (bound): [AF_INET][undef]:51198
Tue Aug 23 21:28:07 2016 us=528539 UDP link remote: [AF_INET][undef]:51198
Tue Aug 23 21:28:07 2016 us=528612 TIMER: coarse timer wakeup 1 seconds
Tue Aug 23 21:28:07 2016 us=529094 RANDOM USEC=205433
Tue Aug 23 21:28:07 2016 us=529128 PO_CTL rwflags=0x0001 ev=5 arg=0x006a5ee8
Tue Aug 23 21:28:07 2016 us=529137 PO_CTL rwflags=0x0001 ev=4 arg=0x006a5824
Tue Aug 23 21:28:07 2016 us=529147 I/O WAIT TR|Tw|SR|Sw [1/205433]
Tue Aug 23 21:28:07 2016 us=529958 PO_WAIT[1,0] fd=4 rev=0x00000001 rwflags=0x0001 arg=0x006a5824 
Tue Aug 23 21:28:07 2016 us=529976  event_wait returned 1
Tue Aug 23 21:28:07 2016 us=529984 I/O WAIT status=0x0004
Tue Aug 23 21:28:07 2016 us=530017  read from TUN/TAP returned 76
Tue Aug 23 21:28:07 2016 us=530028 TUN READ [76]
Memory fault


... because it unconditionally checks

1005    drop_if_recursive_routing (struct context *c, struct buffer *buf)
1006    {
1007      bool drop = false;
1008      struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;

... which is a NULL ptr dereference here...

(gdb) print  c->c2.to_link_addr      
$7 = (struct link_socket_actual *) 0x0

(tun interface comes up, operating system wants to send something, most
likely IPv6 DAD or the likes, which won't go out due to "no remote yet",
but still enters here, and *blam*)

Needs more thorough testing...  I'll put the next version into my treadmill
of "all possible weird ways to hook stuff together" (p2p, --inetd, ...)
and that will hopefully find all issues before pushing it out.

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[David S. <op...@sf...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 23/08/16 15:49, Gert Doering wrote:
> David, please revert both release/2.3 and master, and we'll need
> to rethink this, not using "is_ipv4()"...

Done.

- -- 
kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJXvM9gAAoJEIbPlEyWcf3ycNEQALsZ2nJQfHIqYalF4a+xxoA9
63R76nSZG6qQo1uyZnHwipBf0crYEPCLd4EbRdSIV3leilXrg3Va0QkW9X3Wn0ld
E5uQ4WR4Y4pNkrSB1+l2Iw+N0b1675H5tH/3pKhLe73RvVjI4/APiYtNMEA2FoUv
hnmTBgXzNgPfhJxZgkPiJ4LHIL2XG6SfuFt4H56InIs2BraOul6WKZ4XbvTKpE/R
fyRHMg4BCzRI20g5yd4SpXYlcMkggIPckFss8A78MlpeZfwZgRvsVGtviNyuL62o
Rodv5QSq0nxEMTqIiOK4hhIxmVBiOZu2Xh4HdnG9AAa3ghkux23fD8Iy+aGKmVvf
I2PEZCSorealH19DS8AoIDHaD+dlv8XNz46vwcfxLB06M5JUgTS3mt7iQWXI2dFP
8ORCrllvNb0HZjGPNrobkRGZ4JSos8TCjAYbereRpci642Qy323BqFHljwKbquV0
EThKqdHiQo/x8xJWaqxEz58AATBKF775b4lwy1DCbGO2ca2RSfZJueGzytGtn4Ke
d/5B2oLyOVT6vW5guNTvla8eywNKyVHiwAsaQs31tVEyGOjVa7mTQjUbpowVfntg
PRy4kC+imeElwHXun9dgVg0q9AwdTOQuUUt4OJUyHJqh8Bdp3wYTvxGO4yAFVyMu
yvWuf4Gfwi6FOBpNyjCt
=/Fjb
-----END PGP SIGNATURE-----

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Jan J. K. <ja...@ni...>]

Hi,

On 23/08/16 15:43, Gert Doering wrote:
> Hi,
>
> On Mon, Aug 22, 2016 at 09:18:28PM +0200, Gert Doering wrote:
>> On Mon, Jan 04, 2016 at 02:43:44PM +0200, Lev Stipakov wrote:
>>> v2: better method naming
> [..]
>>> Trac #642
>>>
>>> Signed-off-by: Lev Stipakov <lst...@gm...>
>> ACK.
> As stupid as this feels - we need to back this out again, because it
> breaks TAP mode.  Buildbot complained that all tap tests failed, and
> manually bisecting master nailed it to *this* patch, and release/2.3
> is similarily broken.

may I suggest to make this configurable, i.e. the user can specify 
whether rec routed packets should be dropped?  I'm afraid that we might 
end up with code that drops packets that really should not be dropped - 
people do weird things with routing: in 99% of the cases in error, but 
in 1% of the cases because they want to do something funky.

It would also make it easy to include the current code in 2.3 - turn it 
on in TUN mode by default and OFF in TAP mode.

JM2CW,

JJK

> On the server side, for a t_client test with --dev tap, with this patch,
> you see "the source mac is rotating"
>
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d8:7a -> cron2-gentoo-i386/193.xx.xx.xx
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:d7:c1 -> cron2-gentoo-i386/193.xx.xx.xx
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:fc:a0], packet dropped
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:3e:b1 -> cron2-gentoo-i386/193.xx.xx.xx
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:b9:40:01:3d:f8 -> cron2-gentoo-i386/193.xx.xx.xx
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: bad source address from client [01:72:40:01:62:d7], packet dropped
> Aug 23 09:37:24 phillip tap-udp-p2mp[60213]: cron2-gentoo-i386/193.xx.xx.xx MULTI: Learn: 20:00:40:01:d7:d8 -> cron2-gentoo-i386/193.xx.xx.xx
>
> ... which looks like "the IP header ends up where the ethernet header
> should be" (every ping packet shows up as "new source address" on the
> openvpn server).
>
> I have no idea what this could be, but since we want 2.3.12 out *today*,
> we'll need to back it out of 2.3 for the time being.
>
> Lev, do you have time to investigate?
>
> gert
>
>
>
> ------------------------------------------------------------------------------
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Wed, Aug 24, 2016 at 10:12:54AM +0200, Jan Just Keijser wrote:
> may I suggest to make this configurable, 

Well...

> i.e. the user can specify 
> whether rec routed packets should be dropped?  I'm afraid that we might 
> end up with code that drops packets that really should not be dropped - 
> people do weird things with routing: in 99% of the cases in error, but 
> in 1% of the cases because they want to do something funky.

There is no way that this might ever work(*).

If your tunnel gateway is 1.2.3.4, and you send packets to 1.2.3.4 into
the tunnel, how are these going to arrive?  

Packet goes to TUN, is ecapsulated, and sent to 1.2.3.4.

1.2.3.4 route points to TUN, so packet goes to TUN, gets another layer
of openvpn, and is sent to 1.2.3.4.

1.2.3.4 route points to TUN, so packet goes to TUN, gets a *third* layer
of openvpn, and is sent to 1.2.3.4.

repeat, until the packet hits 1500 byte MTU, and gets fragmented into
*two* packets that loop forever...

gert


(*) it will work if and only if there are multiple routing tables involved,
that is, packets sent by openvpn itself are not subject to the routing 
tables that would move packets *into* the tunnel - so the Android client
wouldn't need it (VPN API takes care of this), and with the work on
Github #13 to suport network name spaces on linux and multiple routing 
tables on *BSD, there are indeed scenarios where this makes sense...

... so you're right, v3 needs to have an option...

(This started out really small and simple :-) )
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Lev S. <lst...@gm...>]

So, following changes are required for V3:

1) No drop_if_recursive() call for P2P
2) Same for TAP
3) Add an option to disable it

Sounds reasonable?


2016-08-24 16:13 GMT+03:00 Gert Doering <ge...@gr...>:

> Hi,
>
> On Wed, Aug 24, 2016 at 10:12:54AM +0200, Jan Just Keijser wrote:
> > may I suggest to make this configurable,
>
> Well...
>
> > i.e. the user can specify
> > whether rec routed packets should be dropped?  I'm afraid that we might
> > end up with code that drops packets that really should not be dropped -
> > people do weird things with routing: in 99% of the cases in error, but
> > in 1% of the cases because they want to do something funky.
>
> There is no way that this might ever work(*).
>
> If your tunnel gateway is 1.2.3.4, and you send packets to 1.2.3.4 into
> the tunnel, how are these going to arrive?
>
> Packet goes to TUN, is ecapsulated, and sent to 1.2.3.4.
>
> 1.2.3.4 route points to TUN, so packet goes to TUN, gets another layer
> of openvpn, and is sent to 1.2.3.4.
>
> 1.2.3.4 route points to TUN, so packet goes to TUN, gets a *third* layer
> of openvpn, and is sent to 1.2.3.4.
>
> repeat, until the packet hits 1500 byte MTU, and gets fragmented into
> *two* packets that loop forever...
>
> gert
>
>
> (*) it will work if and only if there are multiple routing tables involved,
> that is, packets sent by openvpn itself are not subject to the routing
> tables that would move packets *into* the tunnel - so the Android client
> wouldn't need it (VPN API takes care of this), and with the work on
> Github #13 to suport network name spaces on linux and multiple routing
> tables on *BSD, there are indeed scenarios where this makes sense...
>
> ... so you're right, v3 needs to have an option...
>
> (This started out really small and simple :-) )
> --
> USENET is *not* the non-clickable part of WWW!
>                                                            //
> www.muc.de/~gert/
> Gert Doering - Munich, Germany
> ge...@gr...
> fax: +49-89-35655025                        ge...@ne...rmatik.tu-
> muenchen.de
>



-- 
-Lev

Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Tue, Aug 30, 2016 at 09:59:52PM +0300, Lev Stipakov wrote:
> So, following changes are required for V3:
> 
> 1) No drop_if_recursive() call for P2P

Well, sort of.  It's useful in p2p mode as well, but it needs to check
if we already know the remote address before trying to read via the
pointer :-) - basically, this code:

+  struct openvpn_sockaddr tun_sa = c->c2.to_link_addr->dest;

needs to be prefixed by

   if ( c->c2.to_link_addr == NULL ) return;	/* no remote addr known */

(maybe the same effect can be achieved by moving the drop_if_recursive()
call slightly further down in the packet processing, because something
should drop it anyway if there is no remote address)


> 2) Same for TAP

Nah, not "disable it on tap", but "look up the protocol type without
messing up the buffer" (so, not using is_ipv4() and is_ipv6()).  The
function itself is useful on tap as well.

> 3) Add an option to disable it

Yes... people using multiple routing tables will not need this check,
and it might get in the way.  (I'm not a big fan of "yet another option",
but it seems necessary)

> Sounds reasonable?

Yes.  Thanks :-) - I'm also willing to work on it, just had no time yet.

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

[Openvpn-devel] [PATCH v3] Drop recursively routed packets

[Lev S. <lst...@gm...>]

v3: Use better way of figuring out IP proto version which
does not break TAP mode. Add an option to allow recursive
routing, could be useful when packets sent by openvpn itself
are not subject to the routing tables that would move packets
into the tunnel.

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is
disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
operating system starts to use tun as an external interface.
Outgoing packets are routed to tun, UDP encapsulated, given to
routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power,
traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as
destination address.

Tested on Win7/10, OS X.
---
 Changes.rst           |  2 ++
 doc/openvpn.8         |  4 +++
 src/openvpn/forward.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.c | 10 ++++++++
 src/openvpn/options.h |  4 +++
 src/openvpn/proto.h   | 32 +++++++++++++++++++++++
 6 files changed, 123 insertions(+)

diff --git a/Changes.rst b/Changes.rst
index 9fcba75..4cc4acf 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -135,6 +135,8 @@ User-visible Changes
   ciphers configured in the config file.  Use --ncp-disable if you don't want
   that.
 
+- On the client side recursively routed packets, which have same destination as host,
+  are dropped. This could be disabled with --allow-recursive-routing option.
 
 Maintainer-visible changes
 --------------------------
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 2f42636..49d2f72 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3985,6 +3985,10 @@ to the same server, with
 
 OpenVPN will not send any exit
 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.
 .\"*********************************************************
 .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 6c11439..3016677 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -993,6 +993,75 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun.
+ *
+ * On Windows and OS X when netwotk 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.
+ */
+static void
+drop_if_recursive_routing (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa;
+
+  if (c->c2.to_link_addr == NULL) /* no remote addr known */
+    return;
+
+  tun_sa = c->c2.to_link_addr->dest;
+
+  int proto_ver = get_tun_ip_ver (TUNNEL_TYPE (c->c1.tuntap), &c->c2.buf);
+
+  if (proto_ver == 4)
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) BPTR (buf);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (proto_ver == 6)
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < (int) sizeof (struct openvpn_ipv6hdr))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with same dest addr as gateway */
+      pip6 = (struct openvpn_ipv6hdr *) BPTR(buf);
+      if (IN6_ARE_ADDR_EQUAL(&tun_sa.addr.in6.sin6_addr, &pip6->daddr))
+	drop = true;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      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));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link
@@ -1018,6 +1087,8 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      if ((c->options.mode == MODE_POINT_TO_POINT) && (!c->options.allow_recursive_routing))
+	drop_if_recursive_routing (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index c100d4c..90d26c0 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -500,6 +500,8 @@ static const char usage_message[] =
   "--connect-timeout n : when polling possible remote servers to connect to\n"
   "                  in a round-robin fashion, spend no more than n seconds\n"
   "                  waiting for a response before trying the next server.\n"
+  "--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
+  "                  incoming tun packets with same destination as host.\n"
 #endif
 #ifdef ENABLE_OCC
   "--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"
@@ -875,6 +877,7 @@ init_options (struct options *o, const bool init_gc)
   }
 #endif /* WIN32 */
 #endif /* P2MP_SERVER */
+  o->allow_recursive_routing = false;
 }
 
 void
@@ -2128,6 +2131,8 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--ifconfig-ipv6-pool needs --ifconfig-ipv6");
       if (options->ifconfig_ipv6_local && !options->tun_ipv6 )
 	msg (M_INFO, "Warning: --ifconfig-ipv6 without --tun-ipv6 will not do IPv6");
+      if (options->allow_recursive_routing)
+	msg (M_USAGE, "--allow-recursive-routing cannot be used with --mode server");
 
       if (options->auth_user_pass_file)
 	msg (M_USAGE, "--auth-user-pass cannot be used with --mode server (it should be used on the client side only)");
@@ -7365,6 +7370,11 @@ add_option (struct options *options,
       options->keying_material_exporter_length = ekm_length;
     }
 #endif
+  else if (streq (p[0], "allow-recursive-routing") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->allow_recursive_routing = true;
+    }
   else
     {
       int i;
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 9b7b57c..45412b1 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -596,6 +596,10 @@ struct options
 #endif
 
   struct pull_filter_list *pull_filter_list;
+
+  /* Useful when packets sent by openvpn itself are not subject
+     to the routing tables that would move packets into the tunnel. */
+  bool allow_recursive_routing;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))
diff --git a/src/openvpn/proto.h b/src/openvpn/proto.h
index f91e787..93444dd 100644
--- a/src/openvpn/proto.h
+++ b/src/openvpn/proto.h
@@ -219,6 +219,38 @@ struct ip_tcp_udp_hdr {
                              - sizeof(struct openvpn_tcphdr))
 
 /*
+ * This returns an ip protocol version of packet inside tun.
+ */
+inline static int get_tun_ip_ver(int tunnel_type, struct buffer *buf)
+{
+  int offset;
+  int ip_ver;
+  const struct openvpn_iphdr *ih;
+
+  ip_ver = -1;
+
+  /* for tun get ip version from ip header */
+  if (tunnel_type == DEV_TYPE_TUN)
+    {
+      if (likely(BLEN (buf) >= (int) sizeof (struct openvpn_iphdr)))
+	{
+	  ip_ver = OPENVPN_IPH_GET_VER (*BPTR(buf));
+	}
+    }
+  else if (tunnel_type == DEV_TYPE_TAP)
+    {
+      /* for tap get ip version from eth header */
+      if (likely(BLEN (buf) >= (int)(sizeof (struct openvpn_ethhdr))))
+	{
+	  const struct openvpn_ethhdr *eh = (const struct openvpn_ethhdr *) BPTR (buf);
+	  return (ntohs (eh->proto) == OPENVPN_ETH_P_IPV6) ? 6 : 4;
+	}
+    }
+
+    return ip_ver;
+}
+
+/*
  * If raw tunnel packet is IPv4 or IPv6, return true and increment
  * buffer offset to start of IP header.
  */
-- 
1.9.1

[Openvpn-devel] [PATCH applied] Re: Drop recursively routed packets

[Gert D. <ge...@gr...>]

ACK, thanks.  Took us long enough.

Tested on Linux, does what it says (--verb 4 shows nice log messages,
with --allow-recursive-routing (and UDP), you get the expected CPU 
explosion instead).  

Passes all my t_client tests on client AND server, including --inetd
and p2p server.

I had to apply two of your changes manually - the option check in 
options.c and Changes.rst had changes in the preceding lines due to
other patches, so "git am" refused to apply it "just so".   Also, 
a small whitespace change was applied to proto.h

Your patch has been applied to the master and release/2.3 branch
(2.3 had conflicts in Changes.rst and options.[ch] - easily solved,
again by fixing contextual lines).

commit fa3e730e19a71419cb88a021b9a0333f088b39ac (master)
commit 7c930767c0b4ae38c4d886678f091a6077ed55a9 (release/2.3)
Author: Lev Stipakov
Date:   Sat Sep 17 12:47:32 2016 +0300

     Drop recursively routed packets

     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <147...@gm...>
     URL: https://www.mail-archive.com/ope...@li.../msg12476.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] NAK on Re: Drop recursively routed packets

[Gert D. <ge...@gr...>]

Hi,

On Wed, Nov 02, 2016 at 08:41:12PM +0100, Gert Doering wrote:
> ACK, thanks.  Took us long enough.

Bah.  I wanted to do everything right, but missed one crucial test: that
it "does what it says" in tap mode as well - well, it doesn't.  

Unlike v2, this will not break tap (or p2p server or --inetd) mode, but
the functionality is not right.

This code:

      /* make sure we got whole IP header */
      if (BLEN (buf) < (int) sizeof (struct openvpn_iphdr))
        return;

      /* skip ipv4 packets for ipv6 tun */
      if (tun_sa.addr.sa.sa_family != AF_INET)
        return;

      pip = (struct openvpn_iphdr *) BPTR (buf);

      /* drop packets with same dest addr as gateway */
      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
        drop = true;


... does not take "if it's TAP, skip the ethernet header" into account,
so is comparing tun_sa...s_addr with "something", but not with the 
IP address in the IP packet inside the ethernet frame.


So, unfortunately, another NAK.

But I noticed before pushing, so just totally ignore my previous mail -
this is not in the official tree (and won't make 2.3.13, meh).

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

[Openvpn-devel] [PATCH v4] Drop recursively routed packets

[Lev S. <lst...@gm...>]

From: Lev Stipakov <lev...@f-...>

v4:
 - Account for IP header offset in TAP mode
 - Correct handle of non-IP protocols in TAP mode

v3: Use better way of figuring out IP proto version which
does not break TAP mode. Add an option to allow recursive
routing, could be useful when packets sent by openvpn itself
are not subject to the routing tables that would move packets
into the tunnel.

v2: better method naming

On certain OSes (Windows, OS X) when network adapter is
disabled (ethernet cable pulled off, Wi-Fi hardware switch disabled),
operating system starts to use tun as an external interface.
Outgoing packets are routed to tun, UDP encapsulated, given to
routing table and sent to.. tun.

As a consequence, system starts talking to itself on full power,
traffic counters skyrocket and user is not happy.

To prevent that, drop packets which have gateway IP as
destination address.

Tested on Win7/10, OS X.
---
 Changes.rst           |  2 ++
 doc/openvpn.8         |  4 +++
 src/openvpn/forward.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.c | 10 +++++++
 src/openvpn/options.h |  4 +++
 src/openvpn/proto.h   | 39 ++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+)

diff --git a/Changes.rst b/Changes.rst
index 8fd5859..d7aae00 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -188,6 +188,8 @@ User-visible Changes
   capable. The ``--tun-ipv6`` option is ignored (behaves like it is always
   on).
 
+- On the client side recursively routed packets, which have same destination as host,
+  are dropped. This could be disabled with --allow-recursive-routing option.
 
 Maintainer-visible changes
 --------------------------
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 3a4ab21..863dcf9 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -4004,6 +4004,10 @@ to the same server, with
 
 OpenVPN will not send any exit
 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.
 .\"*********************************************************
 .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 b3077ed..3a4c26a 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -993,6 +993,76 @@ read_incoming_tun (struct context *c)
   perf_pop ();
 }
 
+/**
+ * Drops UDP packets which OS decided to route via tun.
+ *
+ * On Windows and OS X when netwotk 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.
+ */
+static void
+drop_if_recursive_routing (struct context *c, struct buffer *buf)
+{
+  bool drop = false;
+  struct openvpn_sockaddr tun_sa;
+  int ip_hdr_offset = 0;
+
+  if (c->c2.to_link_addr == NULL) /* no remote addr known */
+    return;
+
+  tun_sa = c->c2.to_link_addr->dest;
+
+  int proto_ver = get_tun_ip_ver (TUNNEL_TYPE (c->c1.tuntap), &c->c2.buf, &ip_hdr_offset);
+
+  if (proto_ver == 4)
+    {
+      const struct openvpn_iphdr *pip;
+
+      /* make sure we got whole IP header */
+      if (BLEN (buf) < ((int) sizeof (struct openvpn_iphdr) + ip_hdr_offset))
+	return;
+
+      /* skip ipv4 packets for ipv6 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET)
+	return;
+
+      pip = (struct openvpn_iphdr *) (BPTR (buf) + ip_hdr_offset);
+
+      /* drop packets with same dest addr as gateway */
+      if (tun_sa.addr.in4.sin_addr.s_addr == pip->daddr)
+	drop = true;
+    }
+  else if (proto_ver == 6)
+    {
+      const struct openvpn_ipv6hdr *pip6;
+
+      /* make sure we got whole IPv6 header */
+      if (BLEN (buf) < ((int) sizeof (struct openvpn_ipv6hdr) + ip_hdr_offset))
+	return;
+
+      /* skip ipv6 packets for ipv4 tun */
+      if (tun_sa.addr.sa.sa_family != AF_INET6)
+	return;
+
+      /* drop packets with 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;
+    }
+
+  if (drop)
+    {
+      struct gc_arena gc = gc_new ();
+
+      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));
+      gc_free (&gc);
+    }
+}
+
 /*
  * Input:  c->c2.buf
  * Output: c->c2.to_link
@@ -1018,6 +1088,8 @@ process_incoming_tun (struct context *c)
 
   if (c->c2.buf.len > 0)
     {
+      if ((c->options.mode == MODE_POINT_TO_POINT) && (!c->options.allow_recursive_routing))
+	drop_if_recursive_routing (c, &c->c2.buf);
       /*
        * The --passtos and --mssfix options require
        * us to examine the IP header (IPv4 or IPv6).
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index be31ed3..552bf5a 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -508,6 +508,8 @@ static const char usage_message[] =
   "--connect-timeout n : when polling possible remote servers to connect to\n"
   "                  in a round-robin fashion, spend no more than n seconds\n"
   "                  waiting for a response before trying the next server.\n"
+  "--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
+  "                  incoming tun packets with same destination as host.\n"
 #endif
 #ifdef ENABLE_OCC
   "--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"
@@ -886,6 +888,7 @@ init_options (struct options *o, const bool init_gc)
   }
 #endif /* WIN32 */
 #endif /* P2MP_SERVER */
+  o->allow_recursive_routing = false;
 }
 
 void
@@ -2134,6 +2137,8 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 	msg (M_USAGE, "--ifconfig-pool-persist must be used with --ifconfig-pool");
       if (options->ifconfig_ipv6_pool_defined && !options->ifconfig_ipv6_local )
 	msg (M_USAGE, "--ifconfig-ipv6-pool needs --ifconfig-ipv6");
+      if (options->allow_recursive_routing)
+	msg (M_USAGE, "--allow-recursive-routing cannot be used with --mode server");
       if (options->auth_user_pass_file)
 	msg (M_USAGE, "--auth-user-pass cannot be used with --mode server (it should be used on the client side only)");
       if (options->ccd_exclusive && !options->client_config_dir)
@@ -7385,6 +7390,11 @@ add_option (struct options *options,
       options->keying_material_exporter_length = ekm_length;
     }
 #endif
+  else if (streq (p[0], "allow-recursive-routing") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->allow_recursive_routing = true;
+    }
   else
     {
       int i;
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 2f91a52..0ebea30 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -597,6 +597,10 @@ struct options
 #endif
 
   struct pull_filter_list *pull_filter_list;
+
+  /* Useful when packets sent by openvpn itself are not subject
+     to the routing tables that would move packets into the tunnel. */
+  bool allow_recursive_routing;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))
diff --git a/src/openvpn/proto.h b/src/openvpn/proto.h
index f91e787..922f936 100644
--- a/src/openvpn/proto.h
+++ b/src/openvpn/proto.h
@@ -219,6 +219,45 @@ struct ip_tcp_udp_hdr {
                              - sizeof(struct openvpn_tcphdr))
 
 /*
+ * This returns an ip protocol version of packet inside tun
+ * and offset of IP header (via parameter).
+ */
+inline static int get_tun_ip_ver(int tunnel_type, struct buffer *buf, int *ip_hdr_offset)
+{
+  int ip_ver = -1;
+
+  /* for tun get ip version from ip header */
+  if (tunnel_type == DEV_TYPE_TUN)
+    {
+      *ip_hdr_offset = 0;
+      if (likely(BLEN (buf) >= (int) sizeof (struct openvpn_iphdr)))
+	{
+	  ip_ver = OPENVPN_IPH_GET_VER (*BPTR(buf));
+	}
+    }
+  else if (tunnel_type == DEV_TYPE_TAP)
+    {
+      *ip_hdr_offset = (int)(sizeof (struct openvpn_ethhdr));
+      /* for tap get ip version from eth header */
+      if (likely(BLEN (buf) >= *ip_hdr_offset))
+	{
+	  const struct openvpn_ethhdr *eh = (const struct openvpn_ethhdr *) BPTR (buf);
+	  uint16_t proto = ntohs (eh->proto);
+	  if (proto == OPENVPN_ETH_P_IPV6)
+	    {
+	      ip_ver = 6;
+	    }
+	  else if (proto == OPENVPN_ETH_P_IPV4)
+	    {
+	      ip_ver = 4;
+	    }
+	}
+    }
+
+    return ip_ver;
+}
+
+/*
  * If raw tunnel packet is IPv4 or IPv6, return true and increment
  * buffer offset to start of IP header.
  */
-- 
1.9.1

[Openvpn-devel] [PATCH applied] Re: Drop recursively routed packets

[Gert D. <ge...@gr...>]

ACK (finally).

Tested with tun+tap for proper recursion drops on v4+v6 (works, though
on tap it's sometimes non-trivial to reproduce as you need an valid ARP
entry first...), and with all I have for client or server side breakage 
(tun, tap, p2mp, p2p, --inetd, ...) - no breakage.

Your patch has been applied to the master and release/2.3 branch.

I've taken the liberty to add trac#642 to the commit message, fix
an indentation oversight in proto.h, shorten the Changes.rst lines 
somewhat, and adjust 2.3 Changes.rst to "2.3 style" :-)

commit e8c42658ff8df10ad56659788a73900648b9d92d (master)
commit 748974b960a099ef4fdf083f25226659775603b9 (release/2.3)

Author: Lev Stipakov
Date:   Thu Nov 3 23:28:23 2016 +0200

     Drop recursively routed packets

     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <147...@gm...>
     URL: https://www.mail-archive.com/ope...@li.../msg12894.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering