Move openvpn specific key expansion into its own function

This moves the OpenVPN specific PRF into its own function also
simplifies the code a bit by passing tls_session directly instead of
5 of its fields.

Patch v2: Rebase

Patch v4: rewrite/fix comments,
          fix potential not initialised before goto issue

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Steffan Karger <steffan@karger.me>
Message-Id: <20200825073643.15920-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20815.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

1 files changed, 76 insertions, 40 deletions

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 3fcaa25..3535fc7 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1765,27 +1765,36 @@ openvpn_PRF(const uint8_t *secret,
     VALGRIND_MAKE_READABLE((void *)output, output_len);
 }
 
-/*
- * Using source entropy from local and remote hosts, mix into
- * master key.
- */
-static bool
-generate_key_expansion(struct key_ctx_bi *key,
-                       const struct key_type *key_type,
-                       const struct key_source2 *key_src,
-                       const struct session_id *client_sid,
-                       const struct session_id *server_sid,
-                       bool server)
+static void
+init_key_contexts(struct key_ctx_bi *key,
+                  const struct key_type *key_type,
+                  bool server,
+                  struct key2 *key2)
+{
+    /* Initialize key contexts */
+    int key_direction = server ? KEY_DIRECTION_INVERSE : KEY_DIRECTION_NORMAL;
+    init_key_ctx_bi(key, key2, key_direction, key_type, "Data Channel");
+
+    /* Initialize implicit IVs */
+    key_ctx_update_implicit_iv(&key->encrypt, (*key2).keys[(int)server].hmac,
+                               MAX_HMAC_KEY_LENGTH);
+    key_ctx_update_implicit_iv(&key->decrypt, (*key2).keys[1-(int)server].hmac,
+                               MAX_HMAC_KEY_LENGTH);
+
+}
+
+static struct key2
+generate_key_expansion_openvpn_prf(const struct tls_session *session)
 {
     uint8_t master[48] = { 0 };
-    struct key2 key2 = { 0 };
-    bool ret = false;
 
-    if (key->initialized)
-    {
-        msg(D_TLS_ERRORS, "TLS Error: key already initialized");
-        goto exit;
-    }
+    const struct key_state *ks = &session->key[KS_PRIMARY];
+    const struct key_source2 *key_src = ks->key_src;
+
+    const struct session_id *client_sid = session->opt->server ?
+                                          &ks->session_id_remote : &session->session_id;
+    const struct session_id *server_sid = !session->opt->server ?
+                                          &ks->session_id_remote : &session->session_id;
 
     /* debugging print of source key material */
     key_source2_print(key_src);
@@ -1803,6 +1812,7 @@ generate_key_expansion(struct key_ctx_bi *key,
                 master,
                 sizeof(master));
 
+    struct key2 key2;
     /* compute key expansion */
     openvpn_PRF(master,
                 sizeof(master),
@@ -1815,36 +1825,67 @@ generate_key_expansion(struct key_ctx_bi *key,
                 server_sid,
                 (uint8_t *)key2.keys,
                 sizeof(key2.keys));
+    secure_memzero(&master, sizeof(master));
 
+    /*
+     * fixup_key only correctly sets DES parity bits if the cipher is a
+     * DES variant.
+     *
+     * The newer OpenSSL and mbed TLS libraries (those that support EKM)
+     * ignore these bits.
+     *
+     * We keep the DES fixup here as compatibility.
+     * OpenVPN3 never did this fixup anyway. So this code is *probably* not
+     * required but we keep it for compatibility until we remove DES support
+     * since it does not hurt either.
+     */
+    for (int i = 0; i < 2; ++i)
+    {
+        fixup_key(&key2.keys[i], &session->opt->key_type);
+    }
     key2.n = 2;
 
-    key2_print(&key2, key_type, "Master Encrypt", "Master Decrypt");
+    return key2;
+}
+
+/*
+ * Using source entropy from local and remote hosts, mix into
+ * master key.
+ */
+static bool
+generate_key_expansion(struct key_ctx_bi *key,
+                       const struct tls_session *session)
+{
+    bool ret = false;
+    struct key2 key2;
+
+    if (key->initialized)
+    {
+        msg(D_TLS_ERRORS, "TLS Error: key already initialized");
+        goto exit;
+    }
+
+
+    bool server = session->opt->server;
+
+    key2 = generate_key_expansion_openvpn_prf(session);
+
+    key2_print(&key2, &session->opt->key_type,
+               "Master Encrypt", "Master Decrypt");
 
     /* check for weak keys */
     for (int i = 0; i < 2; ++i)
     {
-        fixup_key(&key2.keys[i], key_type);
-        if (!check_key(&key2.keys[i], key_type))
+        if (!check_key(&key2.keys[i], &session->opt->key_type))
         {
             msg(D_TLS_ERRORS, "TLS Error: Bad dynamic key generated");
             goto exit;
         }
     }
-
-    /* Initialize OpenSSL key contexts */
-    int key_direction = server ? KEY_DIRECTION_INVERSE : KEY_DIRECTION_NORMAL;
-    init_key_ctx_bi(key, &key2, key_direction, key_type, "Data Channel");
-
-    /* Initialize implicit IVs */
-    key_ctx_update_implicit_iv(&key->encrypt, key2.keys[(int)server].hmac,
-                               MAX_HMAC_KEY_LENGTH);
-    key_ctx_update_implicit_iv(&key->decrypt, key2.keys[1-(int)server].hmac,
-                               MAX_HMAC_KEY_LENGTH);
-
+    init_key_contexts(key, &session->opt->key_type, server, &key2);
     ret = true;
 
 exit:
-    secure_memzero(&master, sizeof(master));
     secure_memzero(&key2, sizeof(key2));
 
     return ret;
@@ -1879,10 +1920,6 @@ tls_session_generate_data_channel_keys(struct tls_session *session)
 {
     bool ret = false;
     struct key_state *ks = &session->key[KS_PRIMARY];   /* primary key */
-    const struct session_id *client_sid = session->opt->server ?
-                                          &ks->session_id_remote : &session->session_id;
-    const struct session_id *server_sid = !session->opt->server ?
-                                          &ks->session_id_remote : &session->session_id;
 
     if (ks->authenticated == KS_AUTH_FALSE)
     {
@@ -1891,9 +1928,8 @@ tls_session_generate_data_channel_keys(struct tls_session *session)
     }
 
     ks->crypto_options.flags = session->opt->crypto_flags;
-    if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi,
-                                &session->opt->key_type, ks->key_src, client_sid, server_sid,
-                                session->opt->server))
+
+    if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi, session))
     {
         msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
         goto cleanup;