[Openvpn-devel,v4,1/2] Move openvpn specific key expansion into its own function

Message ID 20200825073643.15920-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v4,1/2] Move openvpn specific key expansion into its own function
Related show

Commit Message

Arne Schwabe Aug. 25, 2020, 7:36 a.m.
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>
---
 src/openvpn/ssl.c | 119 ++++++++++++++++++++++++++++++----------------
 1 file changed, 79 insertions(+), 40 deletions(-)

Comments

Steffan Karger Oct. 4, 2020, 6:01 p.m. | #1
Hi,

On 25-08-2020 09:36, Arne Schwabe wrote:
> 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

Thanks. All my previous concerns were tackled accordingly.

> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ssl.c | 119 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 79 insertions(+), 40 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 3fcaa25f..b4d94b8a 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1765,27 +1765,37 @@ 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);
> +
> +}
> +
> +

One newline too much.

> +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 +1813,7 @@ generate_key_expansion(struct key_ctx_bi *key,
>                  master,
>                  sizeof(master));
>  
> +    struct key2 key2;
>      /* compute key expansion */
>      openvpn_PRF(master,
>                  sizeof(master),
> @@ -1815,41 +1826,73 @@ 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;
>  }
>  
> +

Stray newline.

>  static void
>  key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
>  {
> @@ -1879,10 +1922,7 @@ 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 +1931,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))

Since you're passing session now, shouldn't you remove the key_ctx_bi
argument too? That's also part of session: (struct key_state) *ks =
&session->key[KS_PRIMARY].

>      {
>          msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
>          goto cleanup;
> 

Otherwise this looks good. I can also live with leaving key_ctx_bi in
for now. The current version of the patch is a clear improvement (and
paving the way for 2/2, ofc.)

So, as long as at least the whitespace is fixed:

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan
Gert Doering Oct. 5, 2020, 6:52 a.m. | #2
Your patch has been applied to the master branch.

(Since this is only needed for the new PRF functionality and it's
very unlikely that we'll see bugfixes for "master *and* 2.5" for this
code in future, the refactoring is also not needed in 2.5)

Whitespace has been fixed :-) (we really need T-Shirts with
"the whitespace dragon made me do this" on them...)

Tested on the client side against an umodified server, works fine.

commit 15d0524327a10c2999f37375688196f8452f1029
Author: Arne Schwabe
Date:   Tue Aug 25 09:36:42 2020 +0200

     Move openvpn specific key expansion into its own function

     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>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 3fcaa25f..b4d94b8a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1765,27 +1765,37 @@  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 +1813,7 @@  generate_key_expansion(struct key_ctx_bi *key,
                 master,
                 sizeof(master));
 
+    struct key2 key2;
     /* compute key expansion */
     openvpn_PRF(master,
                 sizeof(master),
@@ -1815,41 +1826,73 @@  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;
 }
 
+
 static void
 key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
 {
@@ -1879,10 +1922,7 @@  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 +1931,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;