diff mbox series

[Openvpn-devel,v3] Clean up weak key check

Message ID 20261102134915.26599-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] Clean up weak key check |

Commit Message

Gert Doering Nov. 2, 2026, 1:49 p.m. UTC 
  From: Arne Schwabe <arne@rfc2549.org>

These methods only check for zero keys after weak DES checks were removed.
Simplify all this to always the first 32 bytes of key and hmac and ensure
that they are not zero.

This now always raises a FATAL alert instead of handling this error
gracefully but a all zeros keys should never ever happen.

Change-Id: Ic5cf5ef723a33ad304b080ef19db2e4a0f4bb2eb
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Steffan Karger <steffan@karger.me>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/2080
---

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

Acked-by according to Gerrit (reflected above):
Steffan Karger <steffan@karger.me>
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 3d3ab0c..ff17b22 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1104,40 +1104,6 @@ 
     ctx->initialized = false;
 }
 
-static bool
-key_is_zero(const struct key *key, const struct key_type *kt)
-{
-    size_t cipher_length = cipher_kt_key_size(kt->cipher);
-    for (size_t i = 0; i < cipher_length; ++i)
-    {
-        if (key->cipher[i])
-        {
-            return false;
-        }
-    }
-    msg(D_CRYPT_ERRORS, "CRYPTO INFO: WARNING: zero key detected");
-    return true;
-}
-
-/*
- * Make sure that cipher key is a valid key for current key_type.
- */
-bool
-check_key(const struct key *key, const struct key_type *kt)
-{
-    if (cipher_defined(kt->cipher))
-    {
-        /*
-         * Check for zero key
-         */
-        if (key_is_zero(key, kt))
-        {
-            return false;
-        }
-    }
-    return true;
-}
-
 /*
  * Generate a random key.
  */
@@ -1307,8 +1273,8 @@ 
             print_key_filename(key_file, key_inline));
     }
 
-    /* check for and fix highly unlikely key problems */
-    verify_fix_key2(&key2, key_type, key_file);
+    /* check for highly unlikely key problems */
+    check_key2(&key2);
 
     /* handle key direction */
     key_direction_state_init(&kds, key_direction);
@@ -1708,18 +1674,19 @@ 
 }
 
 void
-verify_fix_key2(struct key2 *key2, const struct key_type *kt, const char *shared_secret_file)
+check_key2(struct key2 *key2)
 {
-    int i;
+    /* Check the first 128 bit of every key slot to be not 0. This is
+     * not a comprehensive check, but no valid key should fail this check */
 
-    for (i = 0; i < key2->n; ++i)
+    uint8_t zero_key[16] = { 0 };
+
+    if (memcmp_constant_time(key2->keys[0].cipher, zero_key, sizeof(zero_key)) == 0
+        || memcmp_constant_time(key2->keys[1].cipher, zero_key, sizeof(zero_key)) == 0
+        || memcmp_constant_time(key2->keys[0].hmac, zero_key, sizeof(zero_key)) == 0
+        || memcmp_constant_time(key2->keys[1].hmac, zero_key, sizeof(zero_key)) == 0)
     {
-        /* This should be a very improbable failure */
-        if (!check_key(&key2->keys[i], kt))
-        {
-            msg(M_FATAL, "Key #%d in '%s' is bad.  Try making a new key with --genkey.", i + 1,
-                shared_secret_file);
-        }
+        msg(M_FATAL, "One key in static key/generated key  is all zero.");
     }
 }
 
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index e3c1c6a..dbe0802 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -414,8 +414,6 @@ 
  */
 int write_key_file(const int nkeys, const char *filename);
 
-bool check_key(const struct key *key, const struct key_type *kt);
-
 /**
  * Initialize a key_type structure with.
  *
@@ -621,7 +619,7 @@ 
 
 void key_direction_state_init(struct key_direction_state *kds, int key_direction);
 
-void verify_fix_key2(struct key2 *key2, const struct key_type *kt, const char *shared_secret_file);
+void check_key2(struct key2 *key2);
 
 void must_have_n_keys(const char *filename, const char *option, const struct key2 *key2, int n);
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 086a6fc..ddee3eb 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1583,15 +1583,7 @@ 
     key2_print(&key2, &session->opt->key_type, "Master Encrypt", "Master Decrypt");
 
     /* check for weak keys */
-    for (int i = 0; i < 2; ++i)
-    {
-        if (!check_key(&key2.keys[i], &session->opt->key_type))
-        {
-            msg(D_TLS_ERRORS, "TLS Error: Bad dynamic key generated");
-            goto exit;
-        }
-    }
-
+    check_key2(&key2);
     ret = init_key_contexts(ks, multi, &session->opt->key_type, server, &key2,
                             session->opt->dco_enabled);