Re: [Openvpn-devel] [PATCH] Fix broken async push with NCP is used
Am 13.03.20 um 17:59 schrieb Lev Stipakov:
> From: Lev Stipakov
>
> 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
> ---
> 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, >options,
> >c2.frame,
> - frame_fragment))
> +if (!tls_session_update_crypto_params(session, >options,
> >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 = >context;
> +struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +if (c->options.ce.fragment)
> +{
> +frame_fragment = >c2.frame_fragment;
> +}
> +#endif
> +struct tls_session *session =
> >c2.tls_multi->session[TM_ACTIVE];
> +if (!tls_session_update_crypto_params(session, >options,
> + >c2.frame,
> frame_fragment))
> +{
> +msg(D_TLS_ERRORS, "TLS Error: initializing data channel
> failed");
> +register_signal(c, SIGUSR1, "init-data-channel-failed");
> +}
I am not happy to see this code fragment being copied here and having
the same code now three times almost identical.
This is the point where we should put it into its own fuction.
Otherwise the code looks good.
So
Semi-Acked-By: Arne Schwabe
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix broken async push with NCP is used
Is it possible to get an estimate on a timeline for this fix being released?
Duo has a few customers that have been impacted by this and, while we have
provided them with workarounds in the meantime, they are asking for updates on
when this will be fixed. I also expect more customers will be impacted as time
goes on as many are installing OpenVPN from a package repository where the
OpenVPN package available there was built with --enable-async-push (e.g. RHEL 8
from EPEL). Since NCP is on by default, this is essentially resulting in broken
behavior out-of-the-box for anyone installing OpenVPN 2.4 from a package repo
and using a plugin that does a deferred authentication.
Thanks!
Spencer
On 3/13/20, 5:22 PM, "Lev Stipakov" wrote:
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
>
> 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
> ---
> 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, >options,
>c2.frame,
> - frame_fragment))
> +if (!tls_session_update_crypto_params(session, >options,
>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 = >context;
> +struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +if (c->options.ce.fragment)
> +{
> +frame_fragment = >c2.frame_fragment;
> +}
> +#endif
> +struct tls_session *session =
>c2.tls_multi->session[TM_ACTIVE];
> +if (!tls_session_update_crypto_params(session,
>options,
> + >c2.frame,
frame_fragment))
> +{
> +msg(D_TLS_ERRORS, "TLS Error: initializing data
channel failed");
> +
Re: [Openvpn-devel] [PATCH] Fix broken async push with NCP is used
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
>
> 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
> ---
> 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, >options,
> >c2.frame,
> - frame_fragment))
> +if (!tls_session_update_crypto_params(session, >options,
> >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 = >context;
> +struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +if (c->options.ce.fragment)
> +{
> +frame_fragment = >c2.frame_fragment;
> +}
> +#endif
> +struct tls_session *session =
> >c2.tls_multi->session[TM_ACTIVE];
> +if (!tls_session_update_crypto_params(session, >options,
> + >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 =
> >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, >options,
> - >c2.frame,
> frame_fragment))
> +if (!tls_session_update_crypto_params(session, >options,
> + >c2.frame,
> frame_fragment))
> {
> msg(D_TLS_ERRORS, "TLS Error:
