diff mbox series

[Openvpn-devel,3/3] Fix using to_link buffer after freed

Message ID 20231108124947.76816-3-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>

When I refactored the tls_state_change method in
9a7b95fda5 I accidentally changed a break into
a return true while it should return a false.

The code here is extremely fragile in the sense
that it assumes that settings a keystate to S_ERROR
cannot have any outgoing buffer or we will have a
use after free.  The previous break and now restored
return false ensure this by skipping any further
tls_process_state loops that might set to ks->S_ERROR
and ensure that the to_link is sent out and cleared
before having more loops in tls_state_change.

CVE: 2023-46850

This affects everyone, even with tls-auth/tls-crypt enabled.

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

Comments

Gert Doering Nov. 9, 2023, 11:31 a.m. UTC | #1 
Third in the series.  This is the actual bug fix for the use-after-free
leak - looks trivial enough... as Arne says, this part of the code is
too full of magic and easy to break.

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

commit 57a5cd1e12f193927c9b7429f8778fec7e04c50a (release/2.6)
commit a0afe035cbca26f8c74b670a8c2a20b3d9c2294b (master)
Author: Arne Schwabe
Date:   Fri Oct 27 14:19:37 2023 +0200

     Fix using to_link buffer after freed

     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-3-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/search?l=mid&q=20231108124947.76816-3-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 e15f951d6..cee4afe19 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2903,7 +2903,13 @@  tls_process_state(struct tls_multi *multi,
                            CONTROL_SEND_ACK_MAX, true);
         *to_link = b;
         dmsg(D_TLS_DEBUG, "Reliable -> TCP/UDP");
-        return true;
+
+        /* This changed the state of the outgoing buffer. In order to avoid
+         * running this function again/further and invalidating the key_state
+         * buffer and accessing the buffer that is now in to_link after it being
+         * freed for a potential error, we shortcircuit exiting of the outer
+         * process here. */
+        return false;
     }
 
     /* Write incoming ciphertext to TLS object */