| Message ID | 20210520151148.2565578-6-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state | expand |
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 <arne@rfc2549.org> > > Patch v2: rebased > > Signed-off-by: Arne Schwabe <arne@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 <antonio@openvpn.net>
Hi, On Thu, May 20, 2021 at 05:11:45PM +0200, 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. Master as of today (ccee09d1478a) + this patch crashes on p2p --secret 2021-07-02 15:06:31 us=336025 Peer Connection Initiated with [AF_INET6]::ffff:194.97.140.21:41858 2021-07-02 15:06:31 us=336099 WARNING: this configuration may cache passwords in memory -- use the auth-nocache option to prevent this 2021-07-02 15:06:31 us=336118 Initialization Sequence Completed Program received signal SIGSEGV, Segmentation fault. 0x00005555555794c9 in do_up (c=c@entry=0x7fffffffd660, pulled_options=pulled_options@entry=false, option_types_found=option_types_found@entry=0) at init.c:2205 2205 c->c2.tls_multi->multi_state = CAS_CONNECT_DONE; (gdb) print c->c2.tls_multi $1 = (struct tls_multi *) 0x0 I'm fairly sure it should not do that :-) Is this fixed in 7/9...9/9? Or do we need a v3 of this one? gert
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; - } - } - + 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 */ + 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 && (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 + * 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 */ CAS_CONNECT_DONE, };