Re: [Openvpn-devel] [PATCH 3/3] Introduce dynamic tls-crypt for secure soft_reset/session renegotiation
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8553,6 +8553,10 @@ add_option(struct options *options,
{
options->imported_protocol_flags |=
CO_USE_TLS_KEY_MATERIAL_EXPORT; }
+else if (streq(p[j], "secure-renog"))
Should be rewritten to use --protocol-flags instead.
It is already using protocol-flags. That is in the middle of
protocol-flags in options.c
tls_crypt_v2_load_client_key(key, , false);
-secure_memzero(, sizeof(key2));
+*original_key = key2;
We should do the zeroing in tls_session_generate_secure_renegotiation_key()
shortly after we used it to XOR then. And maybe only delay it if we need to
XOR anyways, could use original_key == NULL as indication.
We cannot do that. The reason is that a client might reconnect to the
same session (using the TM_UNTRUSTED session slot) or if we have a
simple reconnect. And after the reconnect we need to calculate a new
key. And anyway the data is memory anyway just implicit in the OpenSSL
data structure (tls_wrap.opt.key_ctx_bi) that holds the HMAC/encryption
key for the session just not explicitly.
@@ -587,8 +655,8 @@ tls_crypt_v2_extract_client_key(struct buffer *buf,
ctx->cleanup_key_ctx = true;
ctx->opt.flags |= CO_PACKET_ID_LONG_FORM;
memset(>opt.key_ctx_bi, 0, sizeof(ctx->opt.key_ctx_bi));
-tls_crypt_v2_load_client_key(>opt.key_ctx_bi, _key, true);
-secure_memzero(_key, sizeof(client_key));
+tls_crypt_v2_load_client_key(>opt.key_ctx_bi,
+ >original_tlscrypt_keydata, true);
Same as above for the server side. Could zero here immediately if
original_tlscrypt_keydata == NULL
If we zero the key here, it will not be available when we want to
calculate the secure renegotiation key with xor.
Arne
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Introduce dynamic tls-crypt for secure soft_reset/session renegotiation
On Freitag, 9. September 2022 21:59:02 CEST Arne Schwabe wrote:
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1803,6 +1803,10 @@ multi_client_set_protocol_options(struct context *c)
> {
> o->imported_protocol_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
> }
> +if (proto & IV_PROTO_SECURE_RENOG)
I think any occurrence of "renog" (i.e. short for renegotiation[s]) should be
changed to "reneg" throughout this patch. See --reneg-x options. Could be just
me, so a more opinions would be welcome.
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -8553,6 +8553,10 @@ add_option(struct options *options,
> {
> options->imported_protocol_flags |=
> CO_USE_TLS_KEY_MATERIAL_EXPORT; }
> +else if (streq(p[j], "secure-renog"))
Should be rewritten to use --protocol-flags instead.
> @@ -71,9 +71,77 @@ tls_crypt_init_key(struct key_ctx_bi *key, const char
> *key_file, msg(M_FATAL, "ERROR: --tls-crypt not supported");
> }
> crypto_read_openvpn_key(, key, key_file, key_inline, key_direction,
> -"Control Channel Encryption", "tls-crypt");
> +"Control Channel Encryption", "tls-crypt",
> keydata); }
>
> +/**
> + * Will produce dest = dest XOR data
> + */
> +static void
> +xor_key2_key(struct key2 *dest, const struct key2 *data)
I wonder if this would be more readable as
xor_key(struct key2 *key, const struct key2 *mask)
> +bool
> +tls_session_generate_secure_renegotition_key(struct tls_multi *multi,
> + struct tls_session *session)
typo renegotiation_key
> +struct key2 rengokeys;
typo renogkeys (renegkeys, IMHO)
> @@ -285,7 +354,7 @@ tls_crypt_v2_init_client_key(struct key_ctx_bi *key,
> struct buffer *wkc_buf, }
>
> tls_crypt_v2_load_client_key(key, , false);
> -secure_memzero(, sizeof(key2));
> +*original_key = key2;
We should do the zeroing in tls_session_generate_secure_renegotiation_key()
shortly after we used it to XOR then. And maybe only delay it if we need to
XOR anyways, could use original_key == NULL as indication.
> @@ -587,8 +655,8 @@ tls_crypt_v2_extract_client_key(struct buffer *buf,
> ctx->cleanup_key_ctx = true;
> ctx->opt.flags |= CO_PACKET_ID_LONG_FORM;
> memset(>opt.key_ctx_bi, 0, sizeof(ctx->opt.key_ctx_bi));
> -tls_crypt_v2_load_client_key(>opt.key_ctx_bi, _key, true);
> -secure_memzero(_key, sizeof(client_key));
> +tls_crypt_v2_load_client_key(>opt.key_ctx_bi,
> + >original_tlscrypt_keydata, true);
Same as above for the server side. Could zero here immediately if
original_tlscrypt_keydata == NULL
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/3] Introduce 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.
Since one of tls-crypt/tls-crypt-v2 purpose 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.
OpenVPN 2.x reserves the TM_ACTIVE session for renegotians. When a
SOFT_RESET_V1 packet is received, the active TLS session is moved from
KS_PRIMARY to KS_SECONDARY. If the SOFT_RESET_V1 came from a replay or a
was faked (no tls-auth/tls-crypt), the session is blocked until the TLS
renegotiation attempt times out, blocking the legimitate client.
Using a dynamic tls-crypt key here block any SOFT_RESET_V1 as replay and
fake packets will not have a matching authentication/encryption are
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
dyanmic tls-crypt key is not used here. Replay/fake packets also do not
block the legimiate client.
The issue was initially reported by Fabio Streun
Signed-off-by: Arne Schwabe
---
src/openvpn/crypto.c | 7 +-
src/openvpn/crypto.h | 16 +++-
src/openvpn/init.c| 8 +-
src/openvpn/multi.c | 4 +
src/openvpn/openvpn.h | 2 +
src/openvpn/options.c | 4 +
src/openvpn/push.c| 4 +
src/openvpn/ssl.c | 30 ++--
src/openvpn/ssl.h | 5 ++
src/openvpn/ssl_backend.h | 1 +
src/openvpn/ssl_common.h | 6 ++
src/openvpn/ssl_ncp.c | 4 +
src/openvpn/ssl_pkt.c | 2 +-
src/openvpn/ssl_pkt.h | 21 +
src/openvpn/tls_crypt.c | 93 ---
src/openvpn/tls_crypt.h | 19 -
tests/unit_tests/openvpn/test_pkt.c | 17 -
tests/unit_tests/openvpn/test_tls_crypt.c | 85 +
18 files changed, 299 insertions(+), 29 deletions(-)
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 9e10f64ee..4a8f514cd 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1121,7 +1121,8 @@ void
crypto_read_openvpn_key(const struct key_type *key_type,
struct key_ctx_bi *ctx, const char *key_file,
bool key_inline, const int key_direction,
-const char *key_name, const char *opt_name)
+const char *key_name, const char *opt_name,
+struct key2 *keydata)
{
struct key2 key2;
struct key_direction_state kds;
@@ -1149,6 +1150,10 @@ crypto_read_openvpn_key(const struct key_type *key_type,
/* initialize key in both directions */
init_key_ctx_bi(ctx, , key_direction, key_type, key_name);
+if (keydata)
+{
+*keydata = key2;
+}
secure_memzero(, sizeof(key2));
}
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 5ea889081..e7177ea43 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -234,7 +234,14 @@ struct crypto_options
* both sending and receiving
* directions. */
struct packet_id packet_id; /**< Current packet ID state for both
- * sending and receiving directions. */
+ * sending and receiving directions.
+ *
+ * This contains the packet id that is
+ * used for replay protection.
+ *
+ * The packet id also used as the IV
+ * for AEAD/OFB/CFG ciphers.
+ * */
struct packet_id_persist *pid_persist;
/**< Persistent packet ID state for
* keeping state between successive
@@ -268,6 +275,10 @@ struct crypto_options
/**< Bit-flag indicating that explicit exit notifies should be
* sent via the control channel instead of using an OCC message
*/
+#define CO_USE_SECURE_RENEGOTIATION (1<<7)
+/**< Bit-flag indicating that renegotiations are using tls-crypt
+ * with a TLS-EKM derived key.
+ */
unsigned int flags;
