[Openvpn-devel,2/3] Double check that we do not use a freed buffer when freeing a session

Message ID 20231108124947.76816-2-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,1/3] Remove saving initial frame code | expand

Commit Message

Gert Doering Nov. 8, 2023, 12:49 p.m. UTC
From: Arne Schwabe <arne@rfc2549.org>

This is a find cases where the session already has planned to send out
a packet but encounters some other errors that invalidate the session,
setting it to S_ERROR and leaving the buffer behind.

This will detect and clear that to_link buffer in that case.

Change-Id: I5ffb41bed1c9237946b13d787eb4c4013e0bec68
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Gert Doering Nov. 9, 2023, 11:31 a.m. UTC | #1
Second in the series.  This one is "should we ever do stupid things again,
catch us doing so", so not the actual bugfix but belt&suspenders.

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

commit cd4d819c99266fa727c294225cafdb4ae331d02e (release/2.6)
commit f09d75083e8d7f6db3d90a25480e44de91bd8154 (master)
Author: Arne Schwabe
Date:   Wed Oct 25 17:46:24 2023 +0200

     Double check that we do not use a freed buffer when freeing a session

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: David Sommerseth <davids@openvpn.net>
     Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
     Message-Id: <20231108124947.76816-2-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/search?l=mid&q=20231108124947.76816-2-gert@greenie.muc.de
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 5e6205cc2..e15f951d6 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3155,6 +3155,53 @@  tls_process(struct tls_multi *multi,
     return false;
 }
 
+
+/**
+ * This is a safe guard function to double check that a buffer from a session is
+ * not used in a session to avoid a use after free.
+ *
+ * @param to_link
+ * @param session
+ */
+static void
+check_session_buf_not_used(struct buffer *to_link, struct tls_session *session)
+{
+    uint8_t *dataptr = to_link->data;
+    if (!dataptr)
+    {
+        return;
+    }
+
+    /* Checks buffers in tls_wrap */
+    if (session->tls_wrap.work.data == dataptr)
+    {
+        msg(M_INFO, "Warning buffer of freed TLS session is "
+            "still in use (tls_wrap.work.data)");
+        goto used;
+    }
+
+    for (int i = 0; i < KS_SIZE; i++)
+    {
+        struct key_state *ks = &session->key[i];
+        for (int j = 0; j < ks->send_reliable->size; j++)
+        {
+            if (ks->send_reliable->array[i].buf.data == dataptr)
+            {
+                msg(M_INFO, "Warning buffer of freed TLS session is still in"
+                    " use (session->key[%d].send_reliable->array[%d])",
+                    i, j);
+
+                goto used;
+            }
+        }
+    }
+    return;
+
+used:
+    to_link->len = 0;
+    to_link->data = 0;
+    /* for debugging, you can add an ASSERT(0); here to trigger an abort */
+}
 /*
  * Called by the top-level event loop.
  *
@@ -3253,6 +3300,7 @@  tls_multi_process(struct tls_multi *multi,
                 }
                 else
                 {
+                    check_session_buf_not_used(to_link, session);
                     reset_session(multi, session);
                 }
             }