[Openvpn-devel,v4] Improve "recursive routing" warning message

From: Lev Stipakov <lev@openvpn.net>

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 <lev@openvpn.net>
---
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(-)

Hi,

On 30/10/2018 13:53, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> 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 <lev@openvpn.net>
> ---
> 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(-)
> 
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index 94b5cc4..fb9eb84 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -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
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index f8faa81..e96c546 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -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)
>          {
>              drop = true;
> +
> +            /* collect information for warning message */
> +
> +            saddr = ntohl(pip->saddr);
> +            daddr = ntohl(pip->daddr);
> +            protocol = pip->protocol;
> +
> +            extract_sport_dport(buf, protocol, ip_hdr_offset + sizeof(struct openvpn_iphdr),
> +                                ip_hdr_offset + (int)(ntohs(pip->tot_len)), &sport, &dport);

In the 3rd don-t we need to also add sizeof(struct openvpn_iphdr)?
Or is it included in pip->tot_len?

>          }
>      }
>      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",
> +                       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));

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.


Regards,

>          gc_free(&gc);
>      }
>  }
>

Hi,

On 25/03/2021 23:26, Antonio Quartulli wrote:
> Hi,
> 
> On 30/10/2018 13:53, Lev Stipakov wrote:
>> From: Lev Stipakov <lev@openvpn.net>
>>
>> +
>> +        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.

On 26/03/2021 01:09, tincanteksup wrote:
> Hi,
> 
> On 25/03/2021 23:26, Antonio Quartulli wrote:
>> Hi,
>>
>> On 30/10/2018 13:53, Lev Stipakov wrote:
>>> From: Lev Stipakov <lev@openvpn.net>
>>>
>>> +
>>> +        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

Hi,

On Fri, Mar 26, 2021 at 02:25:13AM +0000, tincanteksup wrote:
> Perhaps this would be better suited as:
> ./configure --enable-recursive-routing
> make

No.

gert

Hi,

On Tue, Oct 30, 2018 at 02:53:59PM +0200, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> 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.

I still do not like this patch.  It is quite a bit of extra code, and
I'm not convinced of the benefit.

Adding documentation is most welcome, though.

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

So, this is good.  Maybe even extend this a bit more.


On the patch itself, it assumes "no IP options" for IPv4, and "no 
intermediate headers" for IPv6, and "all payload is UDP or TCP".  

Which is usually true for any packets that OpenVPN itself originates but 
if you want to log "this is packet from someone else, and we shouldn't
have dropped it because it's not one of ours" it could be anything.


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

gert

Hi,

On 26/03/2021 08:12, Gert Doering wrote:
> 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".

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

After your explanation I agree that this could be twisted into something
more useful, where we actually drop packets we know should *not* be on
the tunnel.


I guess we can conclude that this patch can be rejected as is.

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.

@Lev, are you up for the challenge?


Regards,

Hi,

On Fri, Mar 26, 2021 at 11:30:31AM +0100, Antonio Quartulli wrote:
> 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.

Well, "not really meaningful" depends a bit on what "this feature" might
be.

The original feature (disallow recursive routing) was needed because it
prevents a CPU meltdown under certain conditions
 
 - the VPN server IP address is part of the network that is routed
   inside the tunnel (due to "redirect-gateway" or just because it is
   part of an ISP block)

 - the outgoing interface flaps or changes, and the OS removes the 
   "bypass route"

 - packets sent by OpenVPN are sent by the OS to the tunnel interface,
   re-encapsulated by OpenVPN, forever


This breaks setups where "OpenVPN traffic" is handled specially (route-policy
on Linux, VRF routing, protected socket), and users *want* to access the
VPN server IP via the tunnel - like, because there is http traffic on the
same machine that is not accessible "around".  

So, --enable-recursive-routing was added to allow "suspicious looking 
traffic" through the tunnel, under the "I know what I am doing" rule of
shooting-your-own-foot.


The existing debug info gives too little information to differenciate
"is this openvpn traffic or not?" but I find the extra code needed to
log this to be excessive for the benefit - but, yeah, thinking more through
it, since that code got written already, we *should* use it to only
block OpenVPN traffic ("same proto and dest port") and permit everything
else if the OS sends it our way.

And document more so users do not get confused more than needed :-)

(And no, adding #ifdef is not a useful approach)

gert

Am 26.03.21 um 11:30 schrieb Antonio Quartulli:
> Hi,
> 
> On 26/03/2021 08:12, Gert Doering wrote:
>> 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".
> 
> 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.

Arne

Hi,

On Fri, Mar 26, 2021 at 12:11:10PM +0100, Arne Schwabe wrote:
> 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.

Yeah, on Android the whole code could probably go into an 

#ifndef TARGET_ANDROID
#endif

block...

gert