Attention is currently required from: ordex, plaisthos.
Hello plaisthos,
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 (#2).
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
M src/openvpn/ssl_common.h
4 files changed, 75 insertions(+), 14 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/35/1835/2
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..b5ab51e 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 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 +1392,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 +1424,8 @@
{
init_key_ctx_bi(key, key2, key_direction, key_type, "Data Channel");
}
+
+ return KEY_GEN_OK;
}
static bool
@@ -1475,11 +1486,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 +1535,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 +1550,10 @@
* This erases the source material used to generate the data channel keys, and
* can thus be called only once per session.
*/
-bool
+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 +1564,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 +1577,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 +1648,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 +3246,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 +3351,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 +3455,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..9d614aa 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.
@@ -561,7 +563,8 @@
* 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);
+enum key_gen_status tls_session_generate_data_channel_keys(struct tls_multi
*multi,
+ struct tls_session
*session);
void tls_session_soft_reset(struct tls_multi *multi);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 9c90242..6db304d 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -187,6 +187,21 @@
};
/**
+ * 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 */
+};
+
+/**
* Security parameter state of one TLS and data channel %key session.
* @ingroup control_processor
*
--
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: 2
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-CC: ralf_lici <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: ordex <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
