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
