diff mbox series

[Openvpn-devel,v1] Fix check_addr_clash argument order

Message ID 20240917091433.24092-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v1] Fix check_addr_clash argument order | expand

Commit Message

Gert Doering Sept. 17, 2024, 9:14 a.m. UTC 
From: Ralf Lici <ralf@mandelbit.com>

In init_tun() make sure to pass the --local and --remote addresses in
the host order so that they can be compared to the --ifconfig addresses.

Change-Id: I5adbe0a79f078221c4bb5f3d39391a81b4d8adce
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
---

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/+/737
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <frank@lichtenheld.com>

Comments

Gert Doering Sept. 17, 2024, 9:40 a.m. UTC | #1 
Good find.

I tested this as instructed in gerrit, also enabling the #if 0 debug code -
this is a p2p setup with --topology subnet, to actually have a subnet to
check against.

Test 1, "--local" in the "--ifconfig" subnet:

2024-09-17 11:25:45 CHECK_ADDR_CLASH type=2 public=10.204.8.7 local=10.204.8.2, remote_netmask=255.255.255.0
2024-09-17 11:25:45 WARNING: potential conflict between --local address [10.204.8.7] and --ifconfig address pair [10.204.8.2, 255.255.255.0] -- this is a warning only that is triggered when local/remote addresses exist within the same /24 subnet as --ifconfig endpoints. (silence this warning with --ifconfig-nowarn)

"works".

Test 2, "--remote" in the "--ifconfig" subnet:

2024-09-17 11:27:03 CHECK_ADDR_CLASH type=2 public=10.204.8.8 local=10.204.8.2, remote_netmask=255.255.255.0
2024-09-17 11:27:03 WARNING: potential conflict between --remote address [10.204.8.8] and --ifconfig address pair [10.204.8.2, 255.255.255.0] -- this is a warning only that is triggered when local/remote addresses exist within the same /24 subnet as --ifconfig endpoints. (silence this warning with --ifconfig-nowarn)


Test 3, no "--topology subnet":

2024-09-17 11:36:28 CHECK_ADDR_CLASH type=2 public=10.204.8.250 local=10.204.8.2, remote_netmask=10.204.8.1
2024-09-17 11:36:28 WARNING: potential conflict between --local address [10.204.8.250] and --ifconfig address pair [10.204.8.2, 10.204.8.1] -- this is a warning only that is triggered when local/remote addresses exist within the same /24 subnet as --ifconfig endpoints. (silence this warning with --ifconfig-nowarn)
2024-09-17 11:36:28 CHECK_ADDR_CLASH type=2 public=10.204.8.220 local=10.204.8.2, remote_netmask=10.204.8.1
2024-09-17 11:36:28 WARNING: potential conflict between --remote address [10.204.8.220] and --ifconfig address pair [10.204.8.2, 10.204.8.1] -- this is a warning only that is triggered when local/remote addresses exist within the same /24 subnet as --ifconfig endpoints. (silence this warning with --ifconfig-nowarn)

.. it "works as designed", but does raise questions... see below.


So, while this patch is actually fixing the first problem with this
code, I have some ideas how to improve things further ;-)

 - the line wrapping "one function argument per line" is not how we do
   things today, so a future (master-only) patch could improve that

 - the check as implemented today checks "within the same /24", so if
   I do "--ifconfig 10.204.8.2 255.255.255.128 --remote 10.204.8.220" 
   (which is perfectly sane) I get

      2024-09-17 11:30:44 CHECK_ADDR_CLASH type=2 public=10.204.8.220 local=10.204.8.2, remote_netmask=255.255.255.128
      2024-09-17 11:30:44 WARNING: potential conflict between --remote address [10.204.8.220] and --ifconfig address pair [10.204.8.2, 255.255.255.128] -- this is a warning only that is triggered when local/remote addresses exist within the same /24 subnet as --ifconfig endpoints. (silence this warning with --ifconfig-nowarn)

   ... which is not making any sense.  Computers can do math, AND the
   code actually knows the netmask there, but uses /24 always
   (the TAP check code actually uses the netmask...).

   So I think this code needs to be taught about "--topology subnet",
   and only does the /24 heuristic for p2p/net30 (if at all).


Your patch has been applied to the master and release/26 branch
(bugfix).

commit 7d345b19e20f30cb2ecbea71682b5a41e6cff454 (master)
commit 7e6723aa7096bee80eb42a473fbfde7de4362b0f (release/2.6)
Author: Ralf Lici
Date:   Tue Sep 17 11:14:33 2024 +0200

     Fix check_addr_clash argument order

     Signed-off-by: Ralf Lici <ralf@mandelbit.com>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20240917091433.24092-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg29261.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 739e008..1cd6ad2 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -877,9 +877,10 @@ 
             {
                 if (curele->ai_family == AF_INET)
                 {
+                    const in_addr_t local = ntohl(((struct sockaddr_in *)curele->ai_addr)->sin_addr.s_addr);
                     check_addr_clash("local",
                                      tt->type,
-                                     ((struct sockaddr_in *)curele->ai_addr)->sin_addr.s_addr,
+                                     local,
                                      tt->local,
                                      tt->remote_netmask);
                 }
@@ -889,9 +890,10 @@ 
             {
                 if (curele->ai_family == AF_INET)
                 {
+                    const in_addr_t remote = ntohl(((struct sockaddr_in *)curele->ai_addr)->sin_addr.s_addr);
                     check_addr_clash("remote",
                                      tt->type,
-                                     ((struct sockaddr_in *)curele->ai_addr)->sin_addr.s_addr,
+                                     remote,
                                      tt->local,
                                      tt->remote_netmask);
                 }