Message ID | 1540904039-2129-1-git-send-email-lstipakov@gmail.com |
---|---|
State | New |
Headers | show |
Series | [Openvpn-devel,v4] Improve "recursive routing" warning message | expand |
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
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); } } 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)); gc_free(&gc); } }