Hi,
On 20/05/2021 17:11, Arne Schwabe wrote:
> Since generating data channel keys does not happen when we have reach the
> S_ACTIVE/S_GOT_KEY state anymore like it used to be before NCP, the
> state that data channel keys deserves its own state in the TLS session
> state machine.
>
> The changes done by this commit are rather intrusive since they
> move the key generation to a completely different place and also
> rely on the state machine to decide if keys should be
> generated rather than on the complicated conditions that were
> implemented in the key_method_2_write/read methods.
>
> A (intended) side effect of this change is that sessions that
> are still in deferred state (ks->authenticated == KS_DEFERRED)
> will not have data channel keys generated. This avoids corner
> cases where a not fully authenticated sessions might leak data.
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
>
> Patch v2: rebased
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
> ---
> src/openvpn/forward.h | 2 +-
> src/openvpn/init.c | 1 +
> src/openvpn/ssl.c | 89 +++++++++++++++++-----------------------
> src/openvpn/ssl.h | 10 +++++
> src/openvpn/ssl_common.h | 9 +++-
> 5 files changed, 57 insertions(+), 54 deletions(-)
>
> diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
> index 3461e6422..b8760099e 100644
> --- a/src/openvpn/forward.h
> +++ b/src/openvpn/forward.h
> @@ -450,7 +450,7 @@ connection_established(struct context *c)
> {
> if (c->c2.tls_multi)
> {
> - return c->c2.tls_multi->multi_state >= CAS_CONNECT_DONE;
> + return c->c2.tls_multi->multi_state >= CAS_WAITING_OPTIONS_IMPORT;
> }
> else
> {
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 49c742928..4335d4870 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2202,6 +2202,7 @@ do_up(struct context *c, bool pulled_options, unsigned
> int option_types_found)
> }
>
> c->c2.do_up_ran = true;
> + c->c2.tls_multi->multi_state = CAS_CONNECT_DONE;
> }
> return true;
> }
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index fd64b8d4e..b28d8edf8 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -788,6 +788,9 @@ state_name(int state)
> case S_ERROR:
> return "S_ERROR";
>
> + case S_GENERATED_KEYS:
> + return "S_GENERATED_KEYS";
> +
> default:
> return "S_???";
> }
> @@ -1840,13 +1843,13 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx,
> uint8_t *key, size_t key_len)
> * This erases the source material used to generate the data channel keys,
> and
> * can thus be called only once per session.
> */
> -static bool
> +bool
> tls_session_generate_data_channel_keys(struct tls_session *session)
> {
> bool ret = false;
> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
>
> - if (ks->authenticated == KS_AUTH_FALSE)
> + if (ks->authenticated <= KS_AUTH_FALSE)
> {
> msg(D_TLS_ERRORS, "TLS Error: key_state not authenticated");
> goto cleanup;
> @@ -1862,6 +1865,9 @@ tls_session_generate_data_channel_keys(struct
> tls_session *session)
> tls_limit_reneg_bytes(session->opt->key_type.cipher,
> &session->opt->renegotiate_bytes);
>
> + /* 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));
> @@ -2375,31 +2381,6 @@ key_method_2_write(struct buffer *buf, struct
> tls_multi *multi, struct tls_sessi
> goto error;
> }
>
> - /*
> - * Generate tunnel keys if we're a TLS server.
> - *
> - * If we're a p2mp server to allow NCP, the first key
> - * generation is postponed until after the connect script finished and
> the
> - * NCP options can be processed. Since that always happens at after
> connect
> - * script options are available the CAS_CONNECT_DONE status is identical
> to
> - * NCP options are processed and do not wait for NCP being finished.
> - */
> - if (ks->authenticated > KS_AUTH_FALSE && session->opt->server
> - && ((session->opt->mode == MODE_SERVER && multi->multi_state >=
> CAS_CONNECT_DONE)
> - || (session->opt->mode == MODE_POINT_TO_POINT &&
> !session->opt->pull)))
> - {
> - /* if key_id >= 1, is a renegotiation, so we use the already
> established
> - * parameters and do not need to delay anything. */
> -
> - /* key-id == 0 and multi_state >= CAS_CONNECT_DONE is a special case
> of
> - * the server reusing the session of a reconnecting client. */
> - if (!tls_session_generate_data_channel_keys(session))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: server generate_key_expansion
> failed");
> - goto error;
> - }
> - }
> -
> return true;
>
> error:
> @@ -2599,21 +2580,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi
> *multi, struct tls_sessio
>
> setenv_del(session->opt->es, "exported_keying_material");
> }
> -
> - /*
> - * Generate tunnel keys if we're a client.
> - * If --pull is enabled, the first key generation is postponed until
> after the
> - * pull/push, so we can process pushed cipher directives.
> - */
> - if (!session->opt->server && (!session->opt->pull || ks->key_id > 0))
> - {
> - if (!tls_session_generate_data_channel_keys(session))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: client generate_key_expansion
> failed");
> - goto error;
> - }
> - }
> -
> +
^^ trailing spaces
> gc_free(&gc);
> return true;
>
> @@ -2815,7 +2782,7 @@ tls_process(struct tls_multi *multi,
> else
> {
> /* Skip the connect script related states */
> - multi->multi_state = CAS_CONNECT_DONE;
> + multi->multi_state = CAS_WAITING_OPTIONS_IMPORT;
> }
> }
>
> @@ -3138,6 +3105,27 @@ tls_multi_process(struct tls_multi *multi,
>
> /* If we have successfully authenticated and are still waiting for the
> authentication to finish
> * move the state machine for the multi context forward */
> +
> + if (multi->multi_state >= CAS_CONNECT_DONE)
> + {
> + for (int i = 0; i < TM_SIZE; ++i)
> + {
> + struct tls_session *session = &multi->session[i];
> + struct key_state *ks = &session->key[KS_PRIMARY];
> +
> + if (ks->state == S_ACTIVE && ks->authenticated == KS_AUTH_TRUE)
> + {
> + /* This will ks->state from S_ACTIVE to S_GENERATED_KEYS */
word missing: "switch" ?
> + if (!tls_session_generate_data_channel_keys(session))
> + {
> + msg(D_TLS_ERRORS, "TLS Error: generate_key_expansion
> failed");
> + ks->authenticated = KS_AUTH_FALSE;
> + ks->state = S_ERROR;
> + }
> + }
> + }
> + }
> +
> if (multi->multi_state == CAS_WAITING_AUTH && tas ==
> TLS_AUTHENTICATION_SUCCEEDED)
> {
> multi->multi_state = CAS_PENDING;
> @@ -3246,11 +3234,10 @@ handle_data_channel_packet(struct tls_multi *multi,
> * passive side is the server which only listens for the
> connections, the
> * active side is the client which initiates connections).
> */
> - if (TLS_AUTHENTICATED(multi, ks)
> - && key_id == ks->key_id
> - && (ks->authenticated == KS_AUTH_TRUE)
> + if (ks->state >= S_GENERATED_KEYS && key_id == ks->key_id
extra space after S_GENERATED_KEYS
> && (floated || link_socket_actual_match(from, &ks->remote_addr)))
> {
> + ASSERT(ks->authenticated == KS_AUTH_TRUE);
> if (!ks->crypto_options.key_ctx_bi.initialized)
> {
> msg(D_MULTI_DROPPED,
> @@ -3572,8 +3559,7 @@ tls_pre_decrypt(struct tls_multi *multi,
> /*
> * Remote is requesting a key renegotiation
> */
> - if (op == P_CONTROL_SOFT_RESET_V1
> - && TLS_AUTHENTICATED(multi, ks))
> + if (op == P_CONTROL_SOFT_RESET_V1 && TLS_AUTHENTICATED(multi, ks))
> {
> if (!read_control_auth(buf, &session->tls_wrap, from,
> session->opt))
> @@ -3834,10 +3820,11 @@ struct key_state *tls_select_encryption_key(struct
> tls_multi *multi)
> for (int i = 0; i < KEY_SCAN_SIZE; ++i)
> {
> struct key_state *ks = get_key_scan(multi, i);
> - if (ks->state >= S_ACTIVE
> - && ks->authenticated == KS_AUTH_TRUE
> - && ks->crypto_options.key_ctx_bi.initialized)
> + if (ks->state >= S_GENERATED_KEYS)
> {
> + ASSERT(ks->authenticated == KS_AUTH_TRUE);
> + ASSERT(ks->crypto_options.key_ctx_bi.initialized);
> +
> if (!ks_select)
> {
> ks_select = ks;
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 81cc22131..baf3560d2 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -612,4 +612,14 @@ 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_session *session);
> +
> #endif /* ifndef OPENVPN_SSL_H */
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 66700bf68..928e80929 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -64,7 +64,8 @@
> * material.
> * -# \c S_GOT_KEY, have received remote part of \c key_source2 random
> * material.
> - * -# \c S_ACTIVE, normal operation
> + * -# \c S_ACTIVE, control channel successfully established
> + * -# \c S_GENERATED_KEYS, the
> *
> * Servers follow the same order, except for \c S_SENT_KEY and \c
> * S_GOT_KEY being reversed, because the server first receives the
> @@ -92,7 +93,10 @@
> #define S_ACTIVE 6 /**< Operational \c key_state state
> * immediately after negotiation has
> * completed while still within the
> - * handshake window. */
> + * handshake window, deferred auth, client
> + * connect and can still
typ0: "and can" -> "can"
> + * be pending. */
> +#define S_GENERATED_KEYS 7 /**< The data channel keys have been
> generated */
> /* Note that earlier versions also had a S_OP_NORMAL state that was
> * virtually identical with S_ACTIVE and the code still assumes everything
> * >= S_ACTIVE to be fully operational */
> @@ -516,6 +520,7 @@ enum multi_status {
> CAS_PENDING_DEFERRED,
> CAS_PENDING_DEFERRED_PARTIAL, /**< at least handler succeeded, no
> result yet*/
> CAS_FAILED,
> + CAS_WAITING_OPTIONS_IMPORT, /**< client with pull or p2p waiting for
> first time options import */
do we wait for "options" in p2p mode? I thought in client/server we
could have "pull" enabled, no?
> CAS_CONNECT_DONE,
> };
>
>
Due to your previous simplifications, this change is easier to digest.
However, I wonder if it was easier to split the introduction of
CAS_WAITING_OPTIONS_IMPORT in a different patch?
This said, keeping everything in one patch is still reasonable - mostly
matter of taste I think.
To me this looks like yet another nice simplification which helps making
the whole flow easier to understand.
I tried to break the key generation in various ways, but I was unable to
do so.
So far it worked fine with deferred auth, without it, with client/server
and p2p mode as well.
Even though this patch is intrusive I find it very reasonable.
Acked-by: Antonio Quartulli <anto...@openvpn.net>
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
