diff mbox series

[Openvpn-devel,v1] Do not leave half-initialised key wrap struct when dynamic tls-crypt fails

Message ID 20250327153606.15282-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v1] Do not leave half-initialised key wrap struct when dynamic tls-crypt fails | expand

Commit Message

Gert Doering March 27, 2025, 3:36 p.m. UTC 
From: Arne Schwabe <arne@rfc2549.org>

In case when key_state_export_keying_material fails we left a
half-initialised tls_wrap_reneg  structure in the tls_session.
Later calls to try to free this structure causes freeing of
invalid memory locations.

To test: make key_state_export_keying_material return false even though
         HAVE_EXPORT_KEYING_MATERIAL is defined and connect to a server
         supporting dynamic tls-crypt (2.6.0+)

Change-Id: I54073f8b63894a62699f6ecdc90a77f9f131205b
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: MaxF <max@max-fillinger.net>
---

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

Acked-by according to Gerrit (reflected above):
MaxF <max@max-fillinger.net>

Comments

Gert Doering March 27, 2025, 4:08 p.m. UTC | #1 
Looking at this with "git show --color-moved=zebra -U20" makes clear
that it's just moving around the call that could fail, and if it fails,
do not modify anything else that might then become inconsistent.  As
far as I can see, nothing of this has side effects where order would
be important (except when erroring-out, of course).

Your patch has been applied to the master branch.

commit 7825a8c586a8beba209370f1594cd0987b653ab7
Author: Arne Schwabe
Date:   Thu Mar 27 16:36:00 2025 +0100

     Do not leave half-initialised key wrap struct when dynamic tls-crypt fails

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: MaxF <max@max-fillinger.net>
     Message-Id: <20250327153606.15282-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31267.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index eb7b03d..9e9807d 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -97,6 +97,15 @@ 
 bool
 tls_session_generate_dynamic_tls_crypt_key(struct tls_session *session)
 {
+    struct key2 rengokeys;
+    if (!key_state_export_keying_material(session, EXPORT_DYNAMIC_TLS_CRYPT_LABEL,
+                                          strlen(EXPORT_DYNAMIC_TLS_CRYPT_LABEL),
+                                          rengokeys.keys, sizeof(rengokeys.keys)))
+    {
+        return false;
+    }
+    rengokeys.n = 2;
+
     session->tls_wrap_reneg.opt = session->tls_wrap.opt;
     session->tls_wrap_reneg.mode = TLS_WRAP_CRYPT;
     session->tls_wrap_reneg.cleanup_key_ctx = true;
@@ -108,16 +117,6 @@ 
                    session->opt->replay_time,
                    "TLS_WRAP_RENEG", session->key_id);
 
-
-    struct key2 rengokeys;
-    if (!key_state_export_keying_material(session, EXPORT_DYNAMIC_TLS_CRYPT_LABEL,
-                                          strlen(EXPORT_DYNAMIC_TLS_CRYPT_LABEL),
-                                          rengokeys.keys, sizeof(rengokeys.keys)))
-    {
-        return false;
-    }
-    rengokeys.n = 2;
-
     if (session->tls_wrap.mode == TLS_WRAP_CRYPT
         || session->tls_wrap.mode == TLS_WRAP_AUTH)
     {