Dynamic tls-crypt for secure soft_reset/session renegotiation

Currently we have only one slot for renegotiation of the session/keys.
If a replayed/faked packet is inserted by a malicous attacker, the
legimate peer cannot renegotiate anymore.

This commit introduces dynamic tls-crypt. When both peer support this
feature, both peer create a dynamic tls-crypt key using TLS EKM (export
key material) and will enforce using that key and tls-crypt for all
renegotiations. This also add an additional protection layer for
renegotiations to be taken over by an illegimate client, binding the
renegotiations tightly to the original session. Especially when 2FA,
webauth or similar authentication is used, many third party setup ignore
the need to secure renegotiation with an auth-token.

Since one of tls-crypt/tls-crypt-v2 purposes is to provide poor man's post
quantum crypto guarantees, we have to ensure that the dynamic key tls-crypt
key that replace the original tls-crypt key is as strong as the orginal key
to avoid problems if there is a weak RNG or TLS EKM produces weak keys. We
ensure this but XORing the original key with the key from TLS EKM. If
tls-crypt/tls-cryptv2 is not active, we use just the key generated by
TLS EKM. We also do not use hashing or anything else on the original key
before XOR to avoid any potential of a structure in the key or something
else that might weaken post-quantum use cases.

OpenVPN 2.x reserves the TM_ACTIVE session for renegotiations. When a
SOFT_RESET_V1 packet is received, the active TLS session is moved from
KS_PRIMARY to KS_SECONDARY. Here an attacker could theorectically send a
faked/replayed SOFT_RESET_V1 and first packet containing the TLS client
hello. If this happens, the session is blocked until the TLS
renegotiation attempt times out, blocking the legimitate client.

Using a dynamic tls-crypt key here blocks any SOFT_RESET_V1 (and following
packets) as replay and fake packets will not have a matching
authentication/encryption and will be discarded.

HARD_RESET packets that are from a reconnecting peer are instead put in the
TM_UNTRUSTED/KS_PRIMARY slot until they are sufficiently verified, so the
dynamic tls-crypt key is not used here.  Replay/fake packets also do not
block the legimitate client.

This commit delays the purging of the original tls-crypt key data from
directly after passing it to crypto library to tls_wrap_free. We do this
to allow us mixing the new exported key with the original key.
To be able to generate the dynamic tls-cryptn key, we need the original
key, so deleting the key is not an option if we need it later again to
generate another key. Even when the client does not support secure
renegotiation, deleting the key is not an option since when the
reconnecting client or (especially in p2p mode with float) another client
does the reconnect, we might need to generate a dynamic tls-crypt key
again. Delaying the deletion of the key has also little effect as the
key is still present in the OpenSSL/mbed TLS structures in the tls_wrap
structure, so only the number of times the keys is in memory would be
reduced.

Patch v2: fix spellings of reneg and renegotiations.
Patch v3: expand comment to original_tlscrypt_keydata and commit message,
          add Changes.rst
Patch v4: improve commit message, Changes.rst
Patch v5: fix spelling/grammar mistakes. Add more comments.
Patch v6: consistently calld this feature dynamic tls-crypt crypt. Note
          this changes the export label and makes it incompatible with
          previous patches.
Patch v7: also xor tls-auth key data into the dynamic tls-crypt key like
          tls-crypt key data
Patch v8: Avoid triggering ASSERT added in v7 by properly setting keys.n = 2
          when loading tls crypt v2 client keys. Add dyn-tls-crypt to
          protocol options printout.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Message-Id: <20230307150233.3551436-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26341.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

2 files changed, 100 insertions, 2 deletions

diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c
index 3bbd989..f11e52a 100644
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -55,6 +55,16 @@ parse_line(const char *line, char **p, const int n, const char *file,
     return 0;
 }
 
+/* Define this function here as dummy since including the ssl_*.c files
+ * leads to having to include even more unrelated code */
+bool
+key_state_export_keying_material(struct tls_session *session,
+                                 const char *label, size_t label_size,
+                                 void *ekm, size_t ekm_size)
+{
+    ASSERT(0);
+}
+
 const char *
 print_link_socket_actual(const struct link_socket_actual *act, struct gc_arena *gc)
 {
@@ -191,7 +201,8 @@ init_tas_auth(int key_direction)
 
     crypto_read_openvpn_key(&tls_crypt_kt, &tas.tls_wrap.opt.key_ctx_bi,
                             static_key, true, key_direction,
-                            "Control Channel Authentication", "tls-auth");
+                            "Control Channel Authentication", "tls-auth",
+                            NULL);
 
     return tas;
 }
@@ -203,7 +214,9 @@ init_tas_crypt(bool server)
     tas.tls_wrap.mode = TLS_WRAP_CRYPT;
     tas.tls_wrap.opt.flags |= (CO_IGNORE_PACKET_ID|CO_PACKET_ID_LONG_FORM);
 
-    tls_crypt_init_key(&tas.tls_wrap.opt.key_ctx_bi, static_key, true, server);
+    tls_crypt_init_key(&tas.tls_wrap.opt.key_ctx_bi,
+                       &tas.tls_wrap.original_wrap_keydata, static_key,
+                       true, server);
 
     return tas;
 }
diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c
index 82bb0a2..19ae29c 100644
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -40,6 +40,18 @@
 
 #include "mock_msg.h"
 
+/* Define this function here as dummy since including the ssl_*.c files
+ * leads to having to include even more unrelated code */
+bool
+key_state_export_keying_material(struct tls_session *session,
+                                 const char *label, size_t label_size,
+                                 void *ekm, size_t ekm_size)
+{
+    memset(ekm, 0xba, ekm_size);
+    return true;
+}
+
+
 #define TESTBUF_SIZE            128
 
 /* Defines for use in the tests and the mock parse_line() */
@@ -141,6 +153,7 @@ struct test_tls_crypt_context {
     struct buffer unwrapped;
 };
 
+
 static int
 test_tls_crypt_setup(void **state)
 {
@@ -218,6 +231,75 @@ tls_crypt_loopback(void **state)
                         BLEN(&ctx->source));
 }
 
+
+/**
+ * Test generating dynamic tls-crypt key
+ */
+static void
+test_tls_crypt_secure_reneg_key(void **state)
+{
+    struct test_tls_crypt_context *ctx =
+        (struct test_tls_crypt_context *)*state;
+
+    struct gc_arena gc = gc_new();
+
+    struct tls_multi multi = { 0 };
+    struct tls_session session = { 0 };
+
+    struct tls_options tls_opt = { 0 };
+    tls_opt.replay_window = 32;
+    tls_opt.replay_time = 60;
+    tls_opt.frame.buf.payload_size = 512;
+    session.opt = &tls_opt;
+
+    tls_session_generate_dynamic_tls_crypt_key(&multi, &session);
+
+    struct tls_wrap_ctx *rctx = &session.tls_wrap_reneg;
+
+    tls_crypt_wrap(&ctx->source, &rctx->work, &rctx->opt);
+    assert_int_equal(buf_len(&ctx->source) + 40, buf_len(&rctx->work));
+
+    uint8_t expected_ciphertext[] = {
+        0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0xe3, 0x19, 0x27, 0x7f, 0x1c, 0x8d, 0x6e, 0x6a,
+        0x77, 0x96, 0xa8, 0x55, 0x33, 0x7b, 0x9c, 0xfb, 0x56, 0xe1, 0xf1, 0x3a, 0x87, 0x0e, 0x66, 0x47,
+        0xdf, 0xa1, 0x95, 0xc9, 0x2c, 0x17, 0xa0, 0x15, 0xba, 0x49, 0x67, 0xa1, 0x1d, 0x55, 0xea, 0x1a,
+        0x06, 0xa7
+    };
+    assert_memory_equal(BPTR(&rctx->work), expected_ciphertext, buf_len(&rctx->work));
+    tls_wrap_free(&session.tls_wrap_reneg);
+
+    /* Use previous tls-crypt key as 0x00, with xor we should have the same key
+     * and expect the same result */
+    session.tls_wrap.mode = TLS_WRAP_CRYPT;
+    memset(&session.tls_wrap.original_wrap_keydata.keys, 0x00, sizeof(session.tls_wrap.original_wrap_keydata.keys));
+    session.tls_wrap.original_wrap_keydata.n = 2;
+
+    tls_session_generate_dynamic_tls_crypt_key(&multi, &session);
+    tls_crypt_wrap(&ctx->source, &rctx->work, &rctx->opt);
+    assert_int_equal(buf_len(&ctx->source) + 40, buf_len(&rctx->work));
+
+    assert_memory_equal(BPTR(&rctx->work), expected_ciphertext, buf_len(&rctx->work));
+    tls_wrap_free(&session.tls_wrap_reneg);
+
+    /* XOR should not force a different key */
+    memset(&session.tls_wrap.original_wrap_keydata.keys, 0x42, sizeof(session.tls_wrap.original_wrap_keydata.keys));
+    tls_session_generate_dynamic_tls_crypt_key(&multi, &session);
+
+    tls_crypt_wrap(&ctx->source, &rctx->work, &rctx->opt);
+    assert_int_equal(buf_len(&ctx->source) + 40, buf_len(&rctx->work));
+
+    /* packet id at the start should be equal */
+    assert_memory_equal(BPTR(&rctx->work), expected_ciphertext, 8);
+
+    /* Skip packet id */
+    buf_advance(&rctx->work, 8);
+    assert_memory_not_equal(BPTR(&rctx->work), expected_ciphertext, buf_len(&rctx->work));
+    tls_wrap_free(&session.tls_wrap_reneg);
+
+
+    gc_free(&gc);
+}
+
 /**
  * Check that zero-byte messages are successfully wrapped-and-unwrapped.
  */
@@ -632,6 +714,9 @@ main(void)
         cmocka_unit_test_setup_teardown(tls_crypt_v2_wrap_unwrap_dst_too_small,
                                         test_tls_crypt_v2_setup,
                                         test_tls_crypt_v2_teardown),
+        cmocka_unit_test_setup_teardown(test_tls_crypt_secure_reneg_key,
+                                        test_tls_crypt_setup,
+                                        test_tls_crypt_teardown),
         cmocka_unit_test(test_tls_crypt_v2_write_server_key_file),
         cmocka_unit_test(test_tls_crypt_v2_write_client_key_file),
         cmocka_unit_test(test_tls_crypt_v2_write_client_key_file_metadata),