diff mbox series

[Openvpn-devel,v2] socket: don't transfer bind family to socket in case of ANY address

Message ID 20250325090121.28813-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v2] socket: don't transfer bind family to socket in case of ANY address | expand

Commit Message

Gert Doering March 25, 2025, 9:01 a.m. UTC 
From: Antonio Quartulli <antonio@mandelbit.com>

With the introduction of multisocket, we need to transfer the
AI family of the bound address to the socket, as it may differ
from what was set globally.

However, this operation makes sense only when getaddrinfo()
for bind is performed on a non-empty hostname.
An empty hostname (ANY) may return AF_INET which will break
following connection attempts to v6 only remotes.

Change-Id: I27f305d3ae9bf650bab409e99173688d9f88ab65
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/907
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering May 19, 2025, 1:01 p.m. UTC | #1 
This was +2'ed long ago, but got left rotting due to "I need to find a 
meaningful test case"... well, I give up on this, because this is all
very obscure corner cases (setting lport != 0, but not setting a
--local address to bind to, and the OS serving us a v6 ANY, breaking
connection to a v4 host - which would otherwise work just fine on
a v6 dual-stack socket).

Arguably what the patch does is the right behaviour - if no --local
address (or host) was specified, do not override use the address family
from --proto with the one from --local.  *Iff* a --local hostname or
explicit v4/v6 address was specified, then it makes no sense to query
"for that other family" (this path is unchanged).

Since Arne and the Android client was bitten most by the "lport -> bind"
change, and gave his +2 in gerrit, this is all we need :-)

Tested on FreeBSD and Linux, t_client with lots of v4/v6 remotes...

Your patch has been applied to the master branch.

commit c319dcf048c56098a6aba142e76166684ff5ab12
Author: Antonio Quartulli
Date:   Tue Mar 25 10:01:15 2025 +0100

     socket: don't transfer bind family to socket in case of ANY address

     Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Message-Id: <20250325090121.28813-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31235.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 6b32e30..be7395d 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1724,9 +1724,19 @@ 
                 gai_strerror(status));
         }
 
-        /* the resolved 'local entry' might have a different family than what
-         * was globally configured */
-        sock->info.af = sock->info.lsa->bind_local->ai_family;
+        /* the address family returned by openvpn_getaddrinfo() should be
+         * taken into consideration only if we really passed an hostname
+         * to resolve. Otherwise its value is not useful to us and may
+         * actually break our socket, i.e. when it returns AF_INET
+         * but our remote is v6 only.
+         */
+        if (sock->local_host)
+        {
+            /* the resolved 'local entry' might have a different family than
+             * what was globally configured
+             */
+            sock->info.af = sock->info.lsa->bind_local->ai_family;
+        }
     }
 
     gc_free(&gc);