openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[its_Giaan (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email

to review the following change.


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

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

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 <gia...@ma...>
---
M src/openvpn/mudp.c
1 file changed, 17 insertions(+), 9 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/78/1078/1

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..c47ed16 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -216,16 +216,24 @@
 
             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)
+                /* Ensure that clients from previous sessions do not attempt to
+                 * hijack instances of newly connected clients in multi-protocol scenarios */
+                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];
+
+                    if (mi)
+                    {
+                        *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));
+                        }
+                    }
                 }
             }
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 1
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, its_Giaan.

plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email )

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


Patch Set 1: Code-Review-1

(2 comments)

Patchset:

PS1: 
The check for NULL after already accessing it, looks very fishy.


File src/openvpn/mudp.c:

http://gerrit.openvpn.net/c/openvpn/+/1078/comment/5cf6ed60_e43a0b88 :
PS1, Line 225:                     if (mi)
This looks fishy. This basically can never fail. Since we already access `m->instances[peer_id]` in the if condition, this would have already segfaulted before even getting to this if (mi) check



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 1
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: its_Giaan <gia...@ma...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Mon, 07 Jul 2025 14:05:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[its_Giaan (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, its_Giaan, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


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

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

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 <gia...@ma...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/78/1078/2

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..f62e0a3 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)
+                /* Ensure that clients from previous sessions do not attempt to
+                 * hijack instances of newly connected clients in multi-protocol scenarios */
+                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));
+                    }
                 }
             }
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 2
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: its_Giaan <gia...@ma...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[its_Giaan (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

its_Giaan has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email )

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


Patch Set 1:

(1 comment)

File src/openvpn/mudp.c:

http://gerrit.openvpn.net/c/openvpn/+/1078/comment/5867a758_0966ea4a :
PS1, Line 225:                     if (mi)
> This looks fishy. This basically can never fail. […]
Done



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 1
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Mon, 07 Jul 2025 15:05:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arn...@rf...>
Gerrit-MessageType: comment

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[its_Giaan (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email

to look at the new patch set (#3).


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

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

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 <gia...@ma...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/78/1078/3

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

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 3
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, its_Giaan, plaisthos.

cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email )

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


Patch Set 3: Code-Review+2

(1 comment)

Patchset:

PS3: 
Interesting.  I can not trigger the ASSERT() with "master" with the sequence described

- start multisocket master
- connect UDP client on peer id 0
- connect TCP client on peer id 1
- restart the server
- TCP client reconnects, now peer id 0
- UDP client "sends packets with peer id 0"

it does some other weird things here, namely killing the TCP #0 instance on each UDP packet from #0 - but this is also fixed with this patch (it will just ignore udp #0 packets).



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 3
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: its_Giaan <gia...@ma...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Fri, 18 Jul 2025 18:55:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

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

[Gert D. <ge...@gr...>]

From: Gianmarco De Gregori <gia...@ma...>

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 <gia...@ma...>
Acked-by: Gert Doering <ge...@gr...>
---

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 <ge...@gr...>

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

[Openvpn-devel] [PATCH applied] Re: Multi-socket: Fix assert triggered by stale peer-id reuse

[Gert D. <ge...@gr...>]

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 <gia...@ma...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32220.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#4) to the change originally created by its_Giaan. ( http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


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

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

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: OpenVPN/openvpn#773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gia...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32220.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/78/1078/4

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

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 4
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email )

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

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

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: OpenVPN/openvpn#773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gia...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32220.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)




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

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 4
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged