[Openvpn-devel] tls-crypt-v2: fix client reconnect bug

Message ID 1540991236-4016-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel] tls-crypt-v2: fix client reconnect bug
Related show

Commit Message

Steffan Karger Oct. 31, 2018, 1:07 p.m.
As reported by tincantech on the openvpn-devel IRC channel, a tls-crypt-v2
client could be caused to trigger an assert in tls_crypt_wrap() because the
client key might not be correctly initialized after a reconnect attempt.

This was caused by code that was written before the connection-block
tls-auth/tls-crypt logic was integrated (57d6f103), rebased on that change,
but not sufficiently changed to be compatible with the new logic.

This commit fixes that bug.

Note that I also moved the violating hunk of code to the same function
where the tls-auth and tls-crypt (v1) keys are initialized. Once moved
there, it is immediately clear that v2 didn't follow the same (new) logic.

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
 src/openvpn/init.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

Comments

tincanteksup Oct. 31, 2018, 1:50 p.m. | #1
Tested this with Ubuntu client, patched applied &
Arch server, current master and it worked correctly.

Client did not crash when trying to "reconnect" to a server
when the server had been changed from TLS Crypt V2 to V1.

When the server was changed back to V2 the client connected
successfully.  The client ran the same instance throughout.

(Would test on Windows but can't since my old rig died)

Tested-by: Richard Bonhomme <tincanteksup@gmail.com>
Antonio Quartulli Nov. 14, 2018, 10:51 a.m. | #2
Hi,

On 31/10/2018 23:07, Steffan Karger wrote:
> As reported by tincantech on the openvpn-devel IRC channel, a tls-crypt-v2
> client could be caused to trigger an assert in tls_crypt_wrap() because the
> client key might not be correctly initialized after a reconnect attempt.
> 
> This was caused by code that was written before the connection-block
> tls-auth/tls-crypt logic was integrated (57d6f103), rebased on that change,
> but not sufficiently changed to be compatible with the new logic.
> 
> This commit fixes that bug.
> 
> Note that I also moved the violating hunk of code to the same function
> where the tls-auth and tls-crypt (v1) keys are initialized. Once moved
> there, it is immediately clear that v2 didn't follow the same (new) logic.
> 
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

Yeah, it probably was a "conflict" that went unnoticed.

We now need to rely on the data stored in the Connection Entry (ce
member of the options structure) as the tls-crypt* logic is "per
connection block" and not global anymore.

I performed some basic testing and all seems good.
Thanks for fixing this!


Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering Nov. 18, 2018, 6:58 p.m. | #3
Your patch has been applied to the master branch.

(Gave it a test run, but didn't really look hard at the code)

commit 19d6d9c3533b83f934ea93359bca086a5d06011a
Author: Steffan Karger
Date:   Wed Oct 31 14:07:16 2018 +0100

     tls-crypt-v2: fix client reconnect bug

     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <1540991236-4016-1-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17866.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 39e8ca5..2a1b38e 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2594,6 +2594,26 @@  do_init_tls_wrap_key(struct context *c)
                            options->ce.tls_crypt_file,
                            options->ce.tls_crypt_inline, options->tls_server);
     }
+
+    /* tls-crypt with client-specific keys (--tls-crypt-v2) */
+    if (options->ce.tls_crypt_v2_file)
+    {
+        if (options->tls_server)
+        {
+            tls_crypt_v2_init_server_key(&c->c1.ks.tls_crypt_v2_server_key,
+                                         true, options->ce.tls_crypt_v2_file,
+                                         options->ce.tls_crypt_v2_inline);
+        }
+        else
+        {
+            tls_crypt_v2_init_client_key(&c->c1.ks.tls_wrap_key,
+                                         &c->c1.ks.tls_crypt_v2_wkc,
+                                         options->ce.tls_crypt_v2_file,
+                                         options->ce.tls_crypt_v2_inline);
+        }
+    }
+
+
 }
 
 /*
@@ -2645,27 +2665,9 @@  do_init_crypto_tls_c1(struct context *c)
         /* Initialize PRNG with config-specified digest */
         prng_init(options->prng_hash, options->prng_nonce_secret_len);
 
-        /* initialize tls-auth/crypt key */
+        /* initialize tls-auth/crypt/crypt-v2 key */
         do_init_tls_wrap_key(c);
 
-        /* tls-crypt with client-specific keys (--tls-crypt-v2) */
-        if (options->tls_crypt_v2_file)
-        {
-            if (options->tls_server)
-            {
-                tls_crypt_v2_init_server_key(&c->c1.ks.tls_crypt_v2_server_key,
-                                             true, options->tls_crypt_v2_file,
-                                             options->tls_crypt_v2_inline);
-            }
-            else
-            {
-                tls_crypt_v2_init_client_key(&c->c1.ks.tls_wrap_key,
-                                             &c->c1.ks.tls_crypt_v2_wkc,
-                                             options->tls_crypt_v2_file,
-                                             options->tls_crypt_v2_inline);
-            }
-        }
-
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
         if (options->priv_key_file_inline)
         {
@@ -2891,13 +2893,13 @@  do_init_crypto_tls(struct context *c, const unsigned int flags)
         to.tls_wrap.opt.flags |= CO_PACKET_ID_LONG_FORM;
         tls_crypt_adjust_frame_parameters(&to.frame);
 
-        if (options->tls_crypt_v2_file)
+        if (options->ce.tls_crypt_v2_file)
         {
             to.tls_wrap.tls_crypt_v2_wkc = &c->c1.ks.tls_crypt_v2_wkc;
         }
     }
 
-    if (options->tls_crypt_v2_file)
+    if (options->ce.tls_crypt_v2_file)
     {
         to.tls_crypt_v2 = true;
         if (options->tls_server)