diff mbox series

[Openvpn-devel,v1] Fix construction of invalid pointer in tls_pre_decrypt

Message ID 20251112141335.17417-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v1] Fix construction of invalid pointer in tls_pre_decrypt | expand

Commit Message

Gert Doering Nov. 12, 2025, 2:13 p.m. UTC 
From: Arne Schwabe <arne@rfc2549.org>

In tls_pre_decrypt we construct a pointer ks with an invalid i if i is TM_SIZE
doing a out-of-bounds access in multi->session.

This is a something that exists at least since 2.3.0 (I didn't go further
back but probalby exists in earlier version as well as the commits date
back to SVN beta21 branch).

So we construct the pointer but do not do anything with it if it is inval
id as we check i *after* we construct the pointer `ks`.

I suspect that the compiler optimises the bug away in any higher optimisation
level.

Assuming there is no optimisation, let's check what is possible.
Since we never use the value `ks` if it is invalid, we do not have
worry if it ends up invalid or not. The only thing that we have to
worry about is whether
`session + offsetof(struct tls_session, key[KS_PRIMARY])` is pointing
to memory that is valid to read to construct the `ks` pointer.
This is outside the tls_multi struct, so this is not guaranteed to be
allocated memory but at the same time it is also only few bytes (or few
tens/houndred) after the struct, so it will with an extremely high
probably be in a memory region that will not cause a segfault.

Every time this condition is hit and we construct the invalid pointer,
the log message "TLS Error: Unroutable control packet received" is
printed at `verb 1` or higher. And this is a quite common log message,
which serves as indication as well that a crash is not something that
typically happens but either the optimisation fixes or the memory
region of the invalid access is valid to read from.

Change-Id: Ided1ac7c804487055b175d8766535bead257b7d5
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1373
---

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

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

Comments

Gert Doering Nov. 12, 2025, 3:33 p.m. UTC | #1 
Trivially-correct avoidance of the invalid-index pointer read... and 
quite likely this is what the compiler does as well "this is only needed
after the if(), so let's do that one first", or suchabouts.  Where this
is now, we know i is TM_ACTIVE (0) or TM_UNTRUSTED (1), and so ->session[i]
is valid.

Tested the master patch on the t_server testbed, 2.6 and 2.5 just on the
clients (which is arguably not excercising this code very heavily).

Your patch has been applied to the master, release/2.6 and release/2.5 branch.

Since this is arguably a bug, but has been shown to have no adverse effects,
and everything older has been out of support for a long time, I decided to
be lazy and not backport to 2.4, 2.3, 2.2 and 2.1...

commit 5cdf3f9724c89b278c88fd408714a8d2c1f4d1a1 (master)
commit 4e31670b1e1215130ffaec0f6769e084169da0f1 (release/2.6)
commit 03385f89a1cd95f12bc8ff92b76c209d8b11ef83 (release/2.5)
Author: Arne Schwabe
Date:   Wed Nov 12 15:13:28 2025 +0100

     Fix construction of invalid pointer in tls_pre_decrypt

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1373
     Message-Id: <20251112141335.17417-1-gert@greenie.muc.de>
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 398c9ae..e21ac78 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3729,9 +3729,6 @@ 
     }
     else
     {
-        struct tls_session *session = &multi->session[i];
-        struct key_state *ks = &session->key[KS_PRIMARY];
-
         /*
          * Packet must belong to an existing session.
          */
@@ -3742,6 +3739,8 @@ 
             goto error;
         }
 
+        struct tls_session *session = &multi->session[i];
+        struct key_state *ks = &session->key[KS_PRIMARY];
         /*
          * Verify remote IP address
          */