[S] Change in openvpn[master]: dco: do not exit the process when installing a DCO key fails

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1835?usp=email

to review the following change.


Change subject: dco: do not exit the process when installing a DCO key fails
......................................................................

dco: do not exit the process when installing a DCO key fails

When the DCO peer is gone from the kernel while userspace still believes
it exists, dco_new_key() fails with ENOENT and init_key_contexts() calls
msg(M_FATAL, ...). On a server this terminates the whole daemon and
disconnects every other client, even though only a single peer is
affected.

Make init_key_contexts() return a bool and propagate the failure through
generate_key_expansion(), which already has an error path that wipes the
generated key material. Record the condition in tls_multi so that
dco_update_keys() reports the mismatch and the existing
check_dco_key_status() caller restarts only the affected connection,
which re-creates the DCO peer from scratch.

Note that commit ea3bb67e2b1e ("dco: make key state desync recoverable")
does not cover this case, as both of its hunks are conditional on the
key installation having succeeded.

Github: fixes OpenVPN/openvpn#542
Change-Id: I564edc6e0cc179c2b8b5ddef5e80838949fe9fcd
Signed-off-by: Antonio Quartulli <[email protected]>
---
M src/openvpn/dco.c
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
3 files changed, 38 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/35/1835/1

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 2584368..b66f697 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -132,6 +132,18 @@
 bool
 dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
 {
+    /* installing a key has failed earlier, therefore userspace and kernel
+     * disagree about this peer and no key operation can be trusted anymore.
+     * Report the mismatch so that the caller restarts the connection
+     */
+    if (multi->dco_desync)
+    {
+        msg(D_DCO, "DCO key state mismatch: installing a key has failed earlier "
+                   "(peer_id=%d, dco_keys_installed=%d)",
+            multi->dco_peer_id, multi->dco_keys_installed);
+        return false;
+    }
+
     /* this function checks if keys have to be swapped or erased, therefore it
      * can't do much if we don't have any key installed
      */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index ccd8264..08ca306 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1373,7 +1373,7 @@
     secure_memzero(&e1_recv, sizeof(e1_recv));
 }
 
-static void
+static bool
 init_key_contexts(struct key_state *ks, struct tls_multi *multi, const struct key_type *key_type,
                   bool server, struct key2 *key2, bool dco_enabled)
 {
@@ -1392,7 +1392,18 @@
         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));
+            /* This normally means the DCO peer is gone from the kernel while
+             * userspace still believes it exists. Do not take the whole
+             * process down over a single peer: flag the desync so that
+             * dco_update_keys() can ask for this connection to be restarted,
+             * which re-creates the peer from scratch */
+            msg(M_WARN,
+                "Impossible to install key material in DCO: %s. The underlying "
+                "DCO peer may have been deleted from the kernel without "
+                "notifying userspace. Restarting the session",
+                strerror(-ret));
+            multi->dco_desync = true;
+            return false;
         }
 
         /* encrypt/decrypt context are unused with DCO */
@@ -1415,6 +1426,8 @@
     {
         init_key_ctx_bi(key, key2, key_direction, key_type, "Data Channel");
     }
+
+    return true;
 }
 
 static bool
@@ -1524,7 +1537,11 @@
         }
     }
 
-    init_key_contexts(ks, multi, &session->opt->key_type, server, &key2, session->opt->dco_enabled);
+    if (!init_key_contexts(ks, multi, &session->opt->key_type, server, &key2,
+                           session->opt->dco_enabled))
+    {
+        goto exit;
+    }
     ret = true;
 
 exit:
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 9c90242..9c180a3 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -713,6 +713,12 @@
     /* Only used when DCO is used to remember how many keys we installed
      * for this session */
     int dco_keys_installed;
+    /* Set when installing a key in DCO failed, which means userspace and
+     * kernel disagree about the state of this peer (i.e. the kernel peer is
+     * gone while we still believe it exists). The session cannot recover on
+     * its own, so \c dco_update_keys() reports the mismatch and lets the
+     * caller restart the connection */
+    bool dco_desync;
     /**
      * This is the handle that DCO uses to identify this session with the
      * kernel.

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1835?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I564edc6e0cc179c2b8b5ddef5e80838949fe9fcd
Gerrit-Change-Number: 1835
Gerrit-PatchSet: 1
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel