Fix broken async push with NCP is used

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 adding a call to generate data channel keys when async push is
triggered.

Also, all the "session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized"
checks have been moved into tls_session_update_crypto_params(), which
is just reducing duplicate code, no actual code change (*all* callers
had this pre-check).

Trac: #1259

Reported-by: smaxfield@duosecurity.com
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20200313165913.12682-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19553.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 3b06b57d9f1d972ec16f0893d06697439c1bb1fe)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 37b832ab055a0447fcd0db3a663c019898f83e08..8bac74f97a6dbd21767dd2f9acbdcca68d351a22 100644 (file)
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2302,10 +2302,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 baffd744c3889a949ae2d1a2ce4abc0e4986e12c..58607730881df770038d5615b857fe436a1c59d9 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2132,8 +2132,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 ba2fbe4045551d717ae5369fa1d963ac3cb1e0e1..002be23327cba569111bb5052d14c443be394ba4 100644 (file)
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -295,10 +295,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 7dcd9622fafd8b73376c7191c26bc07990c98db9..cf6689982e7b8cd01ac7f6b0341486811ee6b707 100644 (file)
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1965,6 +1965,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 6672d43fb3907df57d06a0703e1cab5555f0dba3..3266f386aeb0c9e483d34e4ee28c72802a15f6a2 100644 (file)
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -473,7 +473,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.
@@ -481,7 +482,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,