diff mbox series

[Openvpn-devel,v3] Multi-socket: Fix assert triggered by stale peer-id reuse

Message ID 20250718185559.4515-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v3] Multi-socket: Fix assert triggered by stale peer-id reuse | expand

Commit Message

Gert Doering July 18, 2025, 6:55 p.m. UTC 
From: Gianmarco De Gregori <gianmarco@mandelbit.com>

Fixed a bug where clients using different transport
protocols (UDP, TCP) could interfere with each other
after a server restart.
The issue occurred when a client reused a previously
assigned peer-id that was now associated with a
different client using a different transport protocol.

For example, a UDP client could send packets with a
peer-id now assigned to a TCP client, which lacks
a valid context->c2.from which is filled by the
recvfrom(), causing an assert to be triggered.

A protocol check has been added to prevent packets
from different protocols from hijacking active
connections.

Github: #773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

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

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering July 18, 2025, 7:07 p.m. UTC | #1 
So, while I could not trigger the original ASSERT() (GH issue #773) I
was able to trigger "server misbehaviour" (TCP client with peer-id #0
being kicked out when a leftover UDP client with (old) peer-id #0 sent
data packets).  With the patch, these are gone.

The explanation makes sense - when checking for float, just do not look
at TCP instances at all.  Those can not float, might not have all
data fields filled in, and bring no relevant info for a floating UDP 
client.  So when seen with "git show -w", it's just one extra if()
to verify "only compare with UDP instances" (= 'same proto', as this
function is only called for UDP).

Your patch has been applied to the master branch.

commit fd93e4ad8245e1fd9530a6c1f89cb66c047f3abe
Author: Gianmarco De Gregori
Date:   Fri Jul 18 20:55:53 2025 +0200

     Multi-socket: Fix assert triggered by stale peer-id reuse

     Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20250718185559.4515-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg32220.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..ee8446a 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -216,16 +216,20 @@ 
 
             if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
             {
-                mi = m->instances[peer_id];
-
-                *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
-
-                if (*floated)
+                /* Floating on TCP will never be possible, so ensure we only process
+                 * UDP clients */
+                if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto == sock->info.proto)
                 {
-                    /* reset prefix, since here we are not sure peer is the one it claims to be */
-                    ungenerate_prefix(mi);
-                    msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
-                        mroute_addr_print(&real, &gc));
+                    mi = m->instances[peer_id];
+                    *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
+
+                    if (*floated)
+                    {
+                        /* reset prefix, since here we are not sure peer is the one it claims to be */
+                        ungenerate_prefix(mi);
+                        msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
+                            mroute_addr_print(&real, &gc));
+                    }
                 }
             }
         }