I forgot to add trac ticket to the commit message, so maybe @cron2 could do it, assuming v2 won't be needed.
Trac ticket with (lots of) context: https://community.openvpn.net/openvpn/ticket/1259 pe 13. maalisk. 2020 klo 18.59 Lev Stipakov (lstipa...@gmail.com) kirjoitti: > > From: Lev Stipakov <l...@openvpn.net> > > With NCP and deferred auth, we perform cipher negotiation and > generate data channel keys on incoming push request, assuming that auth > succeeded. With async push, when auth succeeds in between push requests, > we send push reply immediately. > > The code which generates data channel keys is only called on handling incoming > push requests (incoming_push_message). It might not be called with NCP, > deferred auth and async push because on incoming push request auth might not > be complete yet. When auth is complete in between push requests, push reply > is sent and it is assumed that connection is established. However, since data > channel keys > are not generated on the server side, connection doesn't work. > > Fix by generating data channel keys when async push is triggered. > > Reported-by: smaxfi...@duosecurity.com > Signed-off-by: Lev Stipakov <l...@openvpn.net> > --- > src/openvpn/init.c | 6 ++---- > src/openvpn/multi.c | 24 +++++++++++++++++++++++- > src/openvpn/push.c | 6 ++---- > src/openvpn/ssl.c | 6 ++++++ > src/openvpn/ssl.h | 5 +++-- > 5 files changed, 36 insertions(+), 11 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 824b6667..b3096dc8 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2343,10 +2343,8 @@ do_deferred_options(struct context *c, const unsigned > int found) > } > #endif > > - /* Do not regenerate keys if server sends an extra push reply */ > - if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized > - && !tls_session_update_crypto_params(session, &c->options, > &c->c2.frame, > - frame_fragment)) > + if (!tls_session_update_crypto_params(session, &c->options, > &c->c2.frame, > + frame_fragment)) > { > msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto > options"); > return false; > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index da0aeeba..b42bcec9 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2137,8 +2137,30 @@ multi_process_file_closed(struct multi_context *m, > const unsigned int mpp_flags) > { > if (mi) > { > - /* continue authentication and send push_reply */ > + /* continue authentication, perform NCP negotiation and send > push_reply */ > multi_process_post(m, mi, mpp_flags); > + > + /* With NCP and deferred authentication, we perform cipher > negotiation and > + * data channel keys generation on incoming push request, > assuming that auth > + * succeeded. When auth succeeds in between push requests > and async push is used, > + * we send push reply immediately. Above > multi_process_post() call performs > + * NCP negotiation and here we do keys generation. */ > + > + struct context *c = &mi->context; > + struct frame *frame_fragment = NULL; > +#ifdef ENABLE_FRAGMENT > + if (c->options.ce.fragment) > + { > + frame_fragment = &c->c2.frame_fragment; > + } > +#endif > + struct tls_session *session = > &c->c2.tls_multi->session[TM_ACTIVE]; > + if (!tls_session_update_crypto_params(session, &c->options, > + &c->c2.frame, > frame_fragment)) > + { > + msg(D_TLS_ERRORS, "TLS Error: initializing data channel > failed"); > + register_signal(c, SIGUSR1, "init-data-channel-failed"); > + } > } > else > { > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 71f22e94..aef00d34 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -298,10 +298,8 @@ incoming_push_message(struct context *c, const struct > buffer *buffer) > } > #endif > struct tls_session *session = > &c->c2.tls_multi->session[TM_ACTIVE]; > - /* Do not regenerate keys if client send a second push request */ > - if > (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized > - && !tls_session_update_crypto_params(session, &c->options, > - &c->c2.frame, > frame_fragment)) > + if (!tls_session_update_crypto_params(session, &c->options, > + &c->c2.frame, > frame_fragment)) > { > msg(D_TLS_ERRORS, "TLS Error: initializing data channel > failed"); > goto error; > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index e21279f1..56d0576a 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -1956,6 +1956,12 @@ tls_session_update_crypto_params(struct tls_session > *session, > struct options *options, struct frame > *frame, > struct frame *frame_fragment) > { > + if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized) > + { > + /* keys already generated, nothing to do */ > + return true; > + } > + > if (!session->opt->server > && 0 != strcmp(options->ciphername, session->opt->config_ciphername) > && !tls_item_in_cipher_list(options->ciphername, > options->ncp_ciphers)) > diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h > index 3449d91e..f0a8ef54 100644 > --- a/src/openvpn/ssl.h > +++ b/src/openvpn/ssl.h > @@ -474,7 +474,8 @@ void tls_update_remote_addr(struct tls_multi *multi, > > /** > * Update TLS session crypto parameters (cipher and auth) and derive data > - * channel keys based on the supplied options. > + * channel keys based on the supplied options. Does nothing if keys are > already > + * generated. > * > * @param session The TLS session to update. > * @param options The options to use when updating session. > @@ -482,7 +483,7 @@ void tls_update_remote_addr(struct tls_multi *multi, > * adjusted based on the selected cipher/auth). > * @param frame_fragment The fragment frame options. > * > - * @return true if updating succeeded, false otherwise. > + * @return true if updating succeeded or keys are already generated, false > otherwise. > */ > bool tls_session_update_crypto_params(struct tls_session *session, > struct options *options, > -- > 2.17.1 > -- -Lev _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
