[Openvpn-devel,v3,09/25] dco: configure keys in DCO right after generating them

Message ID 20220728152012.18643-1-a@unstable.cc
State Accepted
Headers show
Series None | expand

Commit Message

Antonio Quartulli July 28, 2022, 5:20 a.m. UTC
The ovpn-dco kernel module needs to be informed about the keys to be
used to encrypt/decrypt data traffic to/from a peer.

Configure keys in DCO right afte they are generated by the SSL code, to
avoid keeping them in memory longer than needed.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

Changes from v2:
* re-enable explicit-exit-notification in every case
* add check to drop packet when attempting to send data packet and DCO
  is enabled (print warning as well)

Changes from v1:
* adapt to new member name dco_enabled
* invert if blocks and condition in init_key_contexts() [and use 'else']
* fix comment for init_key_contexts()
* disable explicit-exit-notification in mutate_ce() when DCO is enabled
---
 src/openvpn/dco.c     | 55 ++++++++++++++++++++++++++++++++
 src/openvpn/dco.h     | 29 +++++++++++++++++
 src/openvpn/forward.c |  7 +++++
 src/openvpn/init.c    | 11 ++++---
 src/openvpn/multi.c   |  2 +-
 src/openvpn/options.c |  1 -
 src/openvpn/ssl.c     | 73 ++++++++++++++++++++++++++++++++-----------
 src/openvpn/ssl.h     |  7 +++--
 8 files changed, 158 insertions(+), 27 deletions(-)

Comments

Arne Schwabe July 28, 2022, 5:38 a.m. UTC | #1
Am 28.07.22 um 17:20 schrieb Antonio Quartulli:
> The ovpn-dco kernel module needs to be informed about the keys to be
> used to encrypt/decrypt data traffic to/from a peer.
> 
> Configure keys in DCO right afte they are generated by the SSL code, to
> avoid keeping them in memory longer than needed.
> 
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
> 
> Changes from v2:
> * re-enable explicit-exit-notification in every case
> * add check to drop packet when attempting to send data packet and DCO
>    is enabled (print warning as well)
> 
> Changes from v1:
> * adapt to new member name dco_enabled
> * invert if blocks and condition in init_key_contexts() [and use 'else']
> * fix comment for init_key_contexts()
> * disable explicit-exit-notification in mutate_ce() when DCO is enabled

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Aug. 1, 2022, 4:06 a.m. UTC | #2
Tests without --enable-dco (full server side test) - passes everything.

Test with --enable-dco but no Kernel support (client side only) - also
passes everything (spurious failure on one of the p2p tests, but that
was likely related to "too many tests running in parallel").

Did not test on a system with DCO kernel support, as we do not have all
bits and pices integrated yet.

I have not tested the "drop packet" case in forward.c (as that needs
a DCO enabled kernel).  Putting that on my "test with full DCO!" list
- the code certainly looks good.


Stared at code for a bit (even though it has the ACK).

Not sure I like the call chain ssl.c->dco.c->crypto.c for 
init_key_dco_bi() -> key_direction_state_init()... but changing
that would require a bit more ssl.c/crypto.c refactoring.

For the non-DCO cases, the _bi stuff has "key_ctx_update_implicit_iv()"
calls - are these done by the DCO kernel side?  Can't find anything
about IVs in the init_key_dco_bi()->... call chain...

Do the calls to "tls_session_update_crypto_params()" really need to
get a "session" parameter passed in now?  Since they get c->c2.tls_multi
now, "sesion" is just one pointer deref away...  so this might warrant
a cleanup patch later on.


Your patch has been applied to the master branch.

commit 6a5612fe82453915755aca945ff4e876a25f582a
Author: Antonio Quartulli
Date:   Thu Jul 28 17:20:12 2022 +0200

     dco: configure keys in DCO right after generating them

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20220728152012.18643-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24758.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Aug. 1, 2022, 5:14 a.m. UTC | #3
Tests without --enable-dco (full server side test) - passes everything.

Test with --enable-dco but no Kernel support (client side only) - also
passes everything (spurious failure on one of the p2p tests, but that
was likely related to "too many tests running in parallel").

Did not test on a system with DCO kernel support, as we do not have all
bits and pices integrated yet.

I have not tested the "drop packet" case in forward.c (as that needs
a DCO enabled kernel).  Putting that on my "test with full DCO!" list
- the code certainly looks good.


Stared at code for a bit (even though it has the ACK).

Not sure I like the call chain ssl.c->dco.c->crypto.c for 
init_key_dco_bi() -> key_direction_state_init()... but changing
that would require a bit more ssl.c/crypto.c refactoring.

For the non-DCO cases, the _bi stuff has "key_ctx_update_implicit_iv()"
calls - are these done by the DCO kernel side?  Can't find anything
about IVs in the init_key_dco_bi()->... call chain...

Do the calls to "tls_session_update_crypto_params()" really need to
get a "session" parameter passed in now?  Since they get c->c2.tls_multi
now, "sesion" is just one pointer deref away...  so this might warrant
a cleanup patch later on.


Your patch has been applied to the master branch.

commit 6a5612fe82453915755aca945ff4e876a25f582a
Author: Antonio Quartulli
Date:   Thu Jul 28 17:20:12 2022 +0200

     dco: configure keys in DCO right after generating them

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20220728152012.18643-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24758.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index b3fd135f..0471e4d0 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -33,12 +33,67 @@ 
 #if defined(ENABLE_DCO)
 
 #include "syshead.h"
+#include "crypto.h"
 #include "dco.h"
+#include "errlevel.h"
 #include "networking.h"
+#include "openvpn.h"
 #include "options.h"
+#include "ssl_common.h"
 #include "ssl_ncp.h"
 #include "tun.h"
 
+static int
+dco_install_key(struct tls_multi *multi, struct key_state *ks,
+                const uint8_t *encrypt_key, const uint8_t *encrypt_iv,
+                const uint8_t *decrypt_key, const uint8_t *decrypt_iv,
+                const char *ciphername)
+
+{
+    msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->peer_id,
+        ks->key_id);
+
+    /* Install a key in the PRIMARY slot only when no other key exist.
+     * From that moment on, any new key will be installed in the SECONDARY
+     * slot and will be promoted to PRIMARY when userspace says so (a swap
+     * will be performed in that case)
+     */
+    dco_key_slot_t slot = OVPN_KEY_SLOT_PRIMARY;
+    if (multi->dco_keys_installed > 0)
+    {
+        slot = OVPN_KEY_SLOT_SECONDARY;
+    }
+
+    int ret = dco_new_key(multi->dco, multi->peer_id, ks->key_id, slot,
+                          encrypt_key, encrypt_iv,
+                          decrypt_key, decrypt_iv,
+                          ciphername);
+    if ((ret == 0) && (multi->dco_keys_installed < 2))
+    {
+        multi->dco_keys_installed++;
+        ks->dco_status = (slot == OVPN_KEY_SLOT_PRIMARY) ? DCO_INSTALLED_PRIMARY :
+                         DCO_INSTALLED_SECONDARY;
+    }
+
+    return ret;
+}
+
+int
+init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
+                const struct key2 *key2, int key_direction,
+                const char *ciphername, bool server)
+{
+    struct key_direction_state kds;
+    key_direction_state_init(&kds, key_direction);
+
+    return dco_install_key(multi, ks,
+                           key2->keys[kds.out_key].cipher,
+                           key2->keys[(int)server].hmac,
+                           key2->keys[kds.in_key].cipher,
+                           key2->keys[1 - (int)server].hmac,
+                           ciphername);
+}
+
 static bool
 dco_check_option_conflict_platform(int msglevel, const struct options *o)
 {
diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
index 063e5028..1692f5c3 100644
--- a/src/openvpn/dco.h
+++ b/src/openvpn/dco.h
@@ -35,7 +35,10 @@ 
  * order problems)
  */
 struct event_set;
+struct key2;
+struct key_state;
 struct options;
+struct tls_multi;
 struct tuntap;
 
 #define DCO_DEFAULT_METRIC  200
@@ -111,6 +114,24 @@  int dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf);
  */
 void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg);
 
+/**
+ * Install the key material in DCO for the specified peer.
+ * The key is installed in the primary slot when no other key was yet installed.
+ * Any subsequent invocation will install the key in the secondary slot.
+ *
+ * @param multi     the TLS context of the current instance
+ * @param ks        the state of the key being installed
+ * @param key2      the container for the raw key material
+ * @param key_direction the key direction to be used to extract the material
+ * @param ciphername    the name of the cipher to use the key with
+ * @param server    whether we are running on a server instance or not
+ *
+ * @return          0 on success or a negative error code otherwise
+ */
+int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
+                    const struct key2 *key2, int key_direction,
+                    const char *ciphername, bool server);
+
 #else /* if defined(ENABLE_DCO) */
 
 typedef void *dco_context_t;
@@ -163,5 +184,13 @@  dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
 {
 }
 
+static inline int
+init_key_dco_bi(struct tls_multi *multi, struct key_state *ks,
+                const struct key2 *key2, int key_direction,
+                const char *ciphername, bool server)
+{
+    return 0;
+}
+
 #endif /* defined(ENABLE_DCO) */
 #endif /* ifndef DCO_H */
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 6afe152b..28f3c088 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -530,6 +530,13 @@  encrypt_sign(struct context *c, bool comp_frag)
     const uint8_t *orig_buf = c->c2.buf.data;
     struct crypto_options *co = NULL;
 
+    if (dco_enabled(&c->options))
+    {
+        msg(M_WARN, "Attempting to send data packet while data channel offload is in use. "
+            "Dropping packet");
+        c->c2.buf.len = 0;
+    }
+
     /*
      * Drop non-TLS outgoing packet if client-connect script/plugin
      * has not yet succeeded. In non-TLS tls_multi mode is not defined
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b6e1707f..338d797b 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2226,8 +2226,9 @@  do_deferred_p2p_ncp(struct context *c)
     }
 #endif
 
-    if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
-                                          frame_fragment, get_link_socket_info(c)))
+    if (!tls_session_update_crypto_params(c->c2.tls_multi, session, &c->options,
+                                          &c->c2.frame, frame_fragment,
+                                          get_link_socket_info(c)))
     {
         msg(D_TLS_ERRORS, "ERROR: failed to set crypto cipher");
         return false;
@@ -2340,8 +2341,10 @@  do_deferred_options(struct context *c, const unsigned int found)
 #endif
 
         struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-        if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
-                                              frame_fragment, get_link_socket_info(c)))
+        if (!tls_session_update_crypto_params(c->c2.tls_multi, session,
+                                              &c->options, &c->c2.frame,
+                                              frame_fragment,
+                                              get_link_socket_info(c)))
         {
             msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
             return false;
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index ba2f6d58..c72575ae 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2286,7 +2286,7 @@  multi_client_generate_tls_keys(struct context *c)
     }
 #endif
     struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-    if (!tls_session_update_crypto_params(session, &c->options,
+    if (!tls_session_update_crypto_params(c->c2.tls_multi, session, &c->options,
                                           &c->c2.frame, frame_fragment,
                                           get_link_socket_info(c)))
     {
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 11919f6d..3eae14a7 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3193,7 +3193,6 @@  options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         msg(M_WARN, "NOTICE: --explicit-exit-notify ignored for --proto tcp");
         ce->explicit_exit_notification = 0;
     }
-
 }
 
 #ifdef _WIN32
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 24d7f3f4..fc5a8587 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -63,6 +63,7 @@ 
 #include "ssl_util.h"
 #include "auth_token.h"
 #include "mss.h"
+#include "dco.h"
 
 #include "memdbg.h"
 
@@ -1429,21 +1430,48 @@  openvpn_PRF(const uint8_t *secret,
 }
 
 static void
-init_key_contexts(struct key_ctx_bi *key,
+init_key_contexts(struct key_state *ks,
+                  struct tls_multi *multi,
                   const struct key_type *key_type,
                   bool server,
-                  struct key2 *key2)
+                  struct key2 *key2,
+                  bool dco_enabled)
 {
+    struct key_ctx_bi *key = &ks->crypto_options.key_ctx_bi;
+
     /* 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);
+    if (dco_enabled)
+    {
+        if (key->encrypt.hmac)
+        {
+            msg(M_FATAL, "FATAL: DCO does not support --auth");
+        }
+
+        int ret = init_key_dco_bi(multi, ks, key2, key_direction,
+                                  key_type->cipher, server);
+        if (ret < 0)
+        {
+            msg(M_FATAL, "Impossible to install key material in DCO: %s",
+                strerror(-ret));
+        }
 
+        /* encrypt/decrypt context are unused with DCO */
+        CLEAR(key->encrypt);
+        CLEAR(key->decrypt);
+        key->initialized = true;
+    }
+    else
+    {
+        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 bool
@@ -1519,9 +1547,10 @@  generate_key_expansion_openvpn_prf(const struct tls_session *session, struct key
  * master key.
  */
 static bool
-generate_key_expansion(struct key_ctx_bi *key,
+generate_key_expansion(struct tls_multi *multi, struct key_state *ks,
                        struct tls_session *session)
 {
+    struct key_ctx_bi *key = &ks->crypto_options.key_ctx_bi;
     bool ret = false;
     struct key2 key2;
 
@@ -1562,7 +1591,9 @@  generate_key_expansion(struct key_ctx_bi *key,
             goto exit;
         }
     }
-    init_key_contexts(key, &session->opt->key_type, server, &key2);
+
+    init_key_contexts(ks, multi, &session->opt->key_type, server, &key2,
+                      session->opt->dco_enabled);
     ret = true;
 
 exit:
@@ -1594,7 +1625,8 @@  key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len)
  * can thus be called only once per session.
  */
 bool
-tls_session_generate_data_channel_keys(struct tls_session *session)
+tls_session_generate_data_channel_keys(struct tls_multi *multi,
+                                       struct tls_session *session)
 {
     bool ret = false;
     struct key_state *ks = &session->key[KS_PRIMARY];   /* primary key */
@@ -1607,7 +1639,7 @@  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))
+    if (!generate_key_expansion(multi, ks, session))
     {
         msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
         goto cleanup;
@@ -1625,8 +1657,10 @@  cleanup:
 }
 
 bool
-tls_session_update_crypto_params_do_work(struct tls_session *session,
-                                         struct options *options, struct frame *frame,
+tls_session_update_crypto_params_do_work(struct tls_multi *multi,
+                                         struct tls_session *session,
+                                         struct options *options,
+                                         struct frame *frame,
                                          struct frame *frame_fragment,
                                          struct link_socket_info *lsi)
 {
@@ -1669,11 +1703,12 @@  tls_session_update_crypto_params_do_work(struct tls_session *session,
         frame_print(frame_fragment, D_MTU_INFO, "Fragmentation MTU parms");
     }
 
-    return tls_session_generate_data_channel_keys(session);
+    return tls_session_generate_data_channel_keys(multi, session);
 }
 
 bool
-tls_session_update_crypto_params(struct tls_session *session,
+tls_session_update_crypto_params(struct tls_multi *multi,
+                                 struct tls_session *session,
                                  struct options *options, struct frame *frame,
                                  struct frame *frame_fragment,
                                  struct link_socket_info *lsi)
@@ -1695,8 +1730,8 @@  tls_session_update_crypto_params(struct tls_session *session,
     /* Import crypto settings that might be set by pull/push */
     session->opt->crypto_flags |= options->data_channel_crypto_flags;
 
-    return tls_session_update_crypto_params_do_work(session, options, frame,
-                                                    frame_fragment, lsi);
+    return tls_session_update_crypto_params_do_work(multi, session, options,
+                                                    frame, frame_fragment, lsi);
 }
 
 
@@ -3092,7 +3127,7 @@  tls_multi_process(struct tls_multi *multi,
                 /* Session is now fully authenticated.
                 * tls_session_generate_data_channel_keys will move ks->state
                 * from S_ACTIVE to S_GENERATED_KEYS */
-                if (!tls_session_generate_data_channel_keys(session))
+                if (!tls_session_generate_data_channel_keys(multi, session))
                 {
                     msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
                     ks->authenticated = KS_AUTH_FALSE;
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index c8802707..76b1b674 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -426,6 +426,7 @@  void tls_update_remote_addr(struct tls_multi *multi,
  * channel keys based on the supplied options. Does nothing if keys are already
  * generated.
  *
+ * @param multi           The TLS object for this instance.
  * @param session         The TLS session to update.
  * @param options         The options to use when updating session.
  * @param frame           The frame options for this session (frame overhead is
@@ -436,7 +437,8 @@  void tls_update_remote_addr(struct tls_multi *multi,
  *
  * @return true if updating succeeded or keys are already generated, false otherwise.
  */
-bool tls_session_update_crypto_params(struct tls_session *session,
+bool tls_session_update_crypto_params(struct tls_multi *multi,
+                                      struct tls_session *session,
                                       struct options *options,
                                       struct frame *frame,
                                       struct frame *frame_fragment,
@@ -551,7 +553,8 @@  show_available_tls_ciphers(const char *cipher_list,
  * can thus be called only once per session.
  */
 bool
-tls_session_generate_data_channel_keys(struct tls_session *session);
+tls_session_generate_data_channel_keys(struct tls_multi *multi,
+                                       struct tls_session *session);
 
 /**
  * Load ovpn.xkey provider used for external key signing