Attention is currently required from: flichtenheld, plaisthos, ralf_lici.
Hello plaisthos, ralf_lici,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1835?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Code-Review+2 by ralf_lici
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.
Propagate the failure instead. Since a DCO desync needs a different
recovery than any other key generation error - the kernel peer has to be
re-created, which only a reconnect can do - report it as a distinct
key_gen_status through generate_key_expansion() and
tls_session_generate_data_channel_keys(), and let tls_multi_process()
turn it into a new TLSMP_RESTART result that check_tls() dispatches as a
SIGUSR1. On a server this restarts only the affected client instance.
The restart request is tracked separately from 'active' because it must
not be overwritten by a later TLSMP_ACTIVE or TLSMP_RECONNECT
assignment, and the return value now applies an explicit precedence:
killing the session supersedes restarting it, which supersedes 'active'.
tls_session_update_crypto_params_do_work() collapses the desync back to
a plain failure, as all of its callers already turn a failure into a
SIGUSR1.
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/forward.c
M src/openvpn/ssl.c
M src/openvpn/ssl.h
3 files changed, 73 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/35/1835/3
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 46e1a53..a0ddb0f 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -206,6 +206,12 @@
register_signal(c->sig, SIGTERM, "auth-control-exit");
}
}
+ else if (tmp_status == TLSMP_RESTART)
+ {
+ /* The session cannot recover on its own. Kill the connection so
+ * that it is set up again from scratch */
+ register_signal(c->sig, SIGUSR1, "dco key state desync");
+ }
interval_future_trigger(&c->c2.tmp_int, wakeup);
}
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index ccd8264..4a4feaf 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1373,7 +1373,22 @@
secure_memzero(&e1_recv, sizeof(e1_recv));
}
-static void
+/**
+ * Outcome of generating the data channel keys of a session.
+ *
+ * \c KEY_GEN_DCO_DESYNC is kept distinct from \c KEY_GEN_FAILED because it
+ * means userspace and kernel disagree about the DCO peer: the session cannot
+ * recover on its own and the connection has to be restarted, while any other
+ * failure only invalidates the affected key state.
+ */
+enum key_gen_status
+{
+ KEY_GEN_OK, /**< keys were generated and installed */
+ KEY_GEN_FAILED, /**< generation failed, invalidate the key state */
+ KEY_GEN_DCO_DESYNC, /**< the DCO peer is gone from the kernel */
+};
+
+static enum key_gen_status
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 +1407,16 @@
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: report the desync so that the
+ * connection is restarted and the peer re-created 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));
+ return KEY_GEN_DCO_DESYNC;
}
/* encrypt/decrypt context are unused with DCO */
@@ -1415,6 +1439,8 @@
{
init_key_ctx_bi(key, key2, key_direction, key_type, "Data Channel");
}
+
+ return KEY_GEN_OK;
}
static bool
@@ -1475,11 +1501,11 @@
* Using source entropy from local and remote hosts, mix into
* master key.
*/
-static bool
+static enum key_gen_status
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;
+ enum key_gen_status ret = KEY_GEN_FAILED;
struct key2 key2;
if (key->initialized)
@@ -1524,8 +1550,8 @@
}
}
- init_key_contexts(ks, multi, &session->opt->key_type, server, &key2,
session->opt->dco_enabled);
- ret = true;
+ ret = init_key_contexts(ks, multi, &session->opt->key_type, server, &key2,
+ session->opt->dco_enabled);
exit:
secure_memzero(&key2, sizeof(key2));
@@ -1539,10 +1565,10 @@
* This erases the source material used to generate the data channel keys, and
* can thus be called only once per session.
*/
-bool
+static enum key_gen_status
tls_session_generate_data_channel_keys(struct tls_multi *multi, struct
tls_session *session)
{
- bool ret = false;
+ enum key_gen_status ret = KEY_GEN_FAILED;
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
if (ks->authenticated <= KS_AUTH_FALSE)
@@ -1553,7 +1579,8 @@
ks->crypto_options.flags = session->opt->crypto_flags;
- if (!generate_key_expansion(multi, ks, session))
+ ret = generate_key_expansion(multi, ks, session);
+ if (ret != KEY_GEN_OK)
{
msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
goto cleanup;
@@ -1565,7 +1592,6 @@
/* set the state of the keys for the session to generated */
ks->state = S_GENERATED_KEYS;
- ret = true;
cleanup:
secure_memzero(ks->key_src, sizeof(*ks->key_src));
return ret;
@@ -1637,7 +1663,10 @@
}
}
}
- return tls_session_generate_data_channel_keys(multi, session);
+ /* A DCO desync is reported as a plain failure here: every caller of this
+ * function already turns a failure into a SIGUSR1, which is exactly the
+ * recovery a desync needs */
+ return tls_session_generate_data_channel_keys(multi, session) ==
KEY_GEN_OK;
}
bool
@@ -3232,6 +3261,9 @@
struct gc_arena gc = gc_new();
int active = TLSMP_INACTIVE;
bool error = false;
+ /* kept separate from 'active' on purpose: a restart request must not be
+ * overwritten by a later TLSMP_ACTIVE/TLSMP_RECONNECT assignment */
+ bool restart = false;
tls_clear_error();
@@ -3334,12 +3366,22 @@
/* 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(multi, session))
+ enum key_gen_status status =
tls_session_generate_data_channel_keys(multi, session);
+ if (status != KEY_GEN_OK)
{
msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion failed");
ks->authenticated = KS_AUTH_FALSE;
key_state_ssl_shutdown(&ks->ks_ssl);
ks->state = S_ERROR_PRE;
+
+ /* Invalidating the key state is not enough to recover from a
+ * DCO desync: the kernel peer has to be re-created, and the
+ * key state we just invalidated will never be retried, so ask
+ * for a restart right away */
+ if (status == KEY_GEN_DCO_DESYNC)
+ {
+ restart = true;
+ }
}
/* Update auth token on the client if needed on renegotiation
@@ -3428,7 +3470,17 @@
gc_free(&gc);
- return (tas == TLS_AUTHENTICATION_FAILED) ? TLSMP_KILL : active;
+ /* strongest outcome wins: killing the session supersedes restarting it,
+ * and restarting supersedes whatever 'active' ended up being */
+ if (tas == TLS_AUTHENTICATION_FAILED)
+ {
+ return TLSMP_KILL;
+ }
+ if (restart)
+ {
+ return TLSMP_RESTART;
+ }
+ return active;
}
/**
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 7ddf965..5483fbb 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -231,6 +231,8 @@
#define TLSMP_ACTIVE 1
#define TLSMP_KILL 2
#define TLSMP_RECONNECT 3
+/** the session cannot recover on its own and has to be restarted */
+#define TLSMP_RESTART 4
/*
* Called by the top-level event loop.
@@ -554,15 +556,6 @@
void show_available_tls_ciphers(const char *cipher_list, const char
*cipher_list_tls13,
const char *tls_cert_profile);
-
-/**
- * Generate data channel keys for the supplied TLS session.
- *
- * This erases the source material used to generate the data channel keys, and
- * can thus be called only once per session.
- */
-bool tls_session_generate_data_channel_keys(struct tls_multi *multi, struct
tls_session *session);
-
void tls_session_soft_reset(struct tls_multi *multi);
/**
--
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: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I564edc6e0cc179c2b8b5ddef5e80838949fe9fcd
Gerrit-Change-Number: 1835
Gerrit-PatchSet: 3
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: ralf_lici <[email protected]>
Gerrit-CC: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Attention: ralf_lici <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
