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

Message ID 20220720123249.909-1-a@unstable.cc
State Changes Requested
Headers show
Series None | expand

Commit Message

Antonio Quartulli July 20, 2022, 2:32 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 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/init.c    | 11 ++++---
 src/openvpn/multi.c   |  2 +-
 src/openvpn/options.c | 10 ++++++
 src/openvpn/ssl.c     | 73 ++++++++++++++++++++++++++++++++-----------
 src/openvpn/ssl.h     |  7 +++--
 7 files changed, 161 insertions(+), 26 deletions(-)

Comments

Arne Schwabe July 28, 2022, 2:56 a.m. UTC | #1
> index 87d6fc31..dba9d02c 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3194,6 +3194,16 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
>           ce->explicit_exit_notification = 0;
>       }
>   
> +    /* when DCO is in use we can't send data channel packets.
> +     * EEN needs to be re-implemented over the control channel in order
> +     * to work.
> +     */
> +    if (dco_enabled(o) && ce->explicit_exit_notification)
> +    {
> +        msg(M_WARN, "NOTICE: --explicit-exit-notify ignored when "
> +            "data channel offload is in use");
> +        ce->explicit_exit_notification = 0;
> +    }

I don't like this. There is already the patch from me that allows this 
on the control channel. So we need a better solution than disabling it 
when dco is enabled. Did you check what happens if data packets are 
tried being sent when DCO is enabled? Maybe that just has a warning now 
that no key is active.



Arne
Antonio Quartulli July 28, 2022, 3:01 a.m. UTC | #2
On 28/07/2022 14:56, Arne Schwabe wrote:
> 
>> index 87d6fc31..dba9d02c 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -3194,6 +3194,16 @@ options_postprocess_mutate_ce(struct options 
>> *o, struct connection_entry *ce)
>>           ce->explicit_exit_notification = 0;
>>       }
>> +    /* when DCO is in use we can't send data channel packets.
>> +     * EEN needs to be re-implemented over the control channel in order
>> +     * to work.
>> +     */
>> +    if (dco_enabled(o) && ce->explicit_exit_notification)
>> +    {
>> +        msg(M_WARN, "NOTICE: --explicit-exit-notify ignored when "
>> +            "data channel offload is in use");
>> +        ce->explicit_exit_notification = 0;
>> +    }
> 
> I don't like this. There is already the patch from me that allows this 
> on the control channel. So we need a better solution than disabling it 
> when dco is enabled. Did you check what happens if data packets are 
> tried being sent when DCO is enabled? Maybe that just has a warning now 
> that no key is active.

Haven't tried, but by looking at the code it seems OpenVPN will assert 
out because it checks for the key context being not-null.

So, since it cannot work the way it is right now, I wanted to at least 
disable it.

Once implemented on the control channel it could then be re-enabled again.

Cheers,

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/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 87d6fc31..dba9d02c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3194,6 +3194,16 @@  options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         ce->explicit_exit_notification = 0;
     }
 
+    /* when DCO is in use we can't send data channel packets.
+     * EEN needs to be re-implemented over the control channel in order
+     * to work.
+     */
+    if (dco_enabled(o) && ce->explicit_exit_notification)
+    {
+        msg(M_WARN, "NOTICE: --explicit-exit-notify ignored when "
+            "data channel offload is in use");
+        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