New upstream version 2.5.1upstream/2.5.1

11 files changed, 143 insertions, 98 deletions

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 31ecadc..ed7e732 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1606,7 +1606,7 @@ initialization_sequence_completed(struct context *c, const unsigned int flags)
      */
     if (c->options.mode == MODE_POINT_TO_POINT)
     {
-        delayed_auth_pass_purge();
+        ssl_clean_user_pass();
     }
 
     /* Test if errors */
@@ -2552,7 +2552,6 @@ key_schedule_free(struct key_schedule *ks, bool free_ssl_ctx)
     if (tls_ctx_initialised(&ks->ssl_ctx) && free_ssl_ctx)
     {
         tls_ctx_free(&ks->ssl_ctx);
-        free_key_ctx(&ks->tls_crypt_v2_server_key);
     }
     CLEAR(*ks);
 }
@@ -3615,6 +3614,7 @@ do_close_free_key_schedule(struct context *c, bool free_ssl_ctx)
      * always free the tls_auth/crypt key. If persist_key is true, the key will
      * be reloaded from memory (pre-cached)
      */
+    free_key_ctx(&c->c1.ks.tls_crypt_v2_server_key);
     free_key_ctx_bi(&c->c1.ks.tls_wrap_key);
     CLEAR(c->c1.ks.tls_wrap_key);
     buf_clear(&c->c1.ks.tls_crypt_v2_wkc);
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index ac14217..d86b6a7 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -2102,7 +2102,7 @@ man_io_error(struct management *man, const char *prefix)
 static ssize_t
 man_send_with_fd(int fd, void *ptr, size_t nbytes, int flags, int sendfd)
 {
-    struct msghdr msg;
+    struct msghdr msg = { 0 };
     struct iovec iov[1];
 
     union {
@@ -2134,7 +2134,7 @@ man_send_with_fd(int fd, void *ptr, size_t nbytes, int flags, int sendfd)
 static ssize_t
 man_recv_with_fd(int fd, void *ptr, size_t nbytes, int flags, int *recvfd)
 {
-    struct msghdr msghdr;
+    struct msghdr msghdr = { 0 };
     struct iovec iov[1];
     ssize_t n;
 
@@ -3613,7 +3613,6 @@ management_query_user_pass(struct management *man,
         {
             /* preserve caller's settings */
             man->connection.up_query.nocache = up->nocache;
-            man->connection.up_query.wait_for_push = up->wait_for_push;
             *up = man->connection.up_query;
         }
         secure_memzero(&man->connection.up_query, sizeof(man->connection.up_query));
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1038b38..c0c72dd 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -510,10 +510,15 @@ void
 set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token)
 {
 
-    if (token && strlen(token) && up && up->defined)
+    if (strlen(token) && (up->defined || tk->defined))
     {
+        /* auth-token has no password, so it needs the username
+         * either already set or copied from up */
         strncpynt(tk->password, token, USER_PASS_LEN);
-        strncpynt(tk->username, up->username, USER_PASS_LEN);
+        if (up->defined)
+        {
+            strncpynt(tk->username, up->username, USER_PASS_LEN);
+        }
         tk->defined = true;
     }
 
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index a03d94e..e4342b0 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -64,7 +64,6 @@ struct user_pass
 {
     bool defined;
     bool nocache;
-    bool wait_for_push; /* true if this object is waiting for a push-reply */
 
 /* max length of username/password */
 #ifdef ENABLE_PKCS11
@@ -145,6 +144,17 @@ void fail_user_pass(const char *prefix,
 
 void purge_user_pass(struct user_pass *up, const bool force);
 
+/**
+ * Sets the auth-token to token if a username is available from either
+ * up or already present in tk. The method will also purge up if
+ * the auth-nocache option is active.
+ *
+ * @param up        (non Auth-token) Username/password
+ * @param tk        auth-token userpass to set
+ * @param token     token to use as password for the
+ *
+ * @note    all parameters to this function must not be null.
+ */
 void set_auth_token(struct user_pass *up, struct user_pass *tk,
                     const char *token);
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 658ca53..0d99e99 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1983,6 +1983,23 @@ connection_entry_load_re(struct connection_entry *ce, const struct remote_entry
 }
 
 static void
+connection_entry_preload_key(const char **key_file, bool *key_inline,
+                             struct gc_arena *gc)
+{
+    if (key_file && *key_file && !(*key_inline))
+    {
+        struct buffer in = buffer_read_from_file(*key_file, gc);
+        if (!buf_valid(&in))
+        {
+            msg(M_FATAL, "Cannot pre-load keyfile (%s)", *key_file);
+        }
+
+        *key_file = (const char *) in.data;
+        *key_inline = true;
+    }
+}
+
+static void
 options_postprocess_verify_ce(const struct options *options,
                               const struct connection_entry *ce)
 {
@@ -2933,36 +2950,17 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         ce->tls_crypt_v2_file_inline = o->tls_crypt_v2_file_inline;
     }
 
-    /* pre-cache tls-auth/crypt key file if persist-key was specified and keys
-     * were not already embedded in the config file
+    /* Pre-cache tls-auth/crypt(-v2) key file if persist-key was specified and
+     * keys were not already embedded in the config file.
      */
     if (o->persist_key)
     {
-        if (ce->tls_auth_file && !ce->tls_auth_file_inline)
-        {
-            struct buffer in = buffer_read_from_file(ce->tls_auth_file, &o->gc);
-            if (!buf_valid(&in))
-            {
-                msg(M_FATAL, "Cannot pre-load tls-auth keyfile (%s)",
-                    ce->tls_auth_file);
-            }
-
-            ce->tls_auth_file = (char *)in.data;
-            ce->tls_auth_file_inline = true;
-        }
-
-        if (ce->tls_crypt_file && !ce->tls_crypt_file_inline)
-        {
-            struct buffer in = buffer_read_from_file(ce->tls_crypt_file, &o->gc);
-            if (!buf_valid(&in))
-            {
-                msg(M_FATAL, "Cannot pre-load tls-crypt keyfile (%s)",
-                    ce->tls_crypt_file);
-            }
-
-            ce->tls_crypt_file = (char *)in.data;
-            ce->tls_crypt_file_inline = true;
-        }
+        connection_entry_preload_key(&ce->tls_auth_file,
+                                     &ce->tls_auth_file_inline, &o->gc);
+        connection_entry_preload_key(&ce->tls_crypt_file,
+                                     &ce->tls_crypt_file_inline, &o->gc);
+        connection_entry_preload_key(&ce->tls_crypt_v2_file,
+                                     &ce->tls_crypt_v2_file_inline, &o->gc);
     }
 }
 
@@ -3966,7 +3964,7 @@ options_warning_safe_scan2(const int msglevel,
     if (strprefix(p1, "key-method ")
         || strprefix(p1, "keydir ")
         || strprefix(p1, "proto ")
-        || strprefix(p1, "tls-auth ")
+        || streq(p1, "tls-auth")
         || strprefix(p1, "tun-ipv6")
         || strprefix(p1, "cipher "))
     {
@@ -4661,7 +4659,8 @@ in_src_get(const struct in_src *is, char *line, const int size)
 }
 
 static char *
-read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
+read_inline_file(struct in_src *is, const char *close_tag,
+                 int *num_lines, struct gc_arena *gc)
 {
     char line[OPTION_LINE_SIZE];
     struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE);
@@ -4670,6 +4669,7 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
 
     while (in_src_get(is, line, sizeof(line)))
     {
+        (*num_lines)++;
         char *line_ptr = line;
         /* Remove leading spaces */
         while (isspace(*line_ptr))
@@ -4703,10 +4703,10 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
     return ret;
 }
 
-static bool
+static int
 check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
 {
-    bool is_inline = false;
+    int num_inline_lines = 0;
 
     if (p[0] && !p[1])
     {
@@ -4719,16 +4719,15 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
             p[0] = string_alloc(arg + 1, gc);
             close_tag = alloc_buf(strlen(p[0]) + 4);
             buf_printf(&close_tag, "</%s>", p[0]);
-            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
+            p[1] = read_inline_file(is, BSTR(&close_tag), &num_inline_lines, gc);
             p[2] = NULL;
             free_buf(&close_tag);
-            is_inline = true;
         }
     }
-    return is_inline;
+    return num_inline_lines;
 }
 
-static bool
+static int
 check_inline_file_via_fp(FILE *fp, char *p[], struct gc_arena *gc)
 {
     struct in_src is;
@@ -4737,7 +4736,7 @@ check_inline_file_via_fp(FILE *fp, char *p[], struct gc_arena *gc)
     return check_inline_file(&is, p, gc);
 }
 
-static bool
+static int
 check_inline_file_via_buf(struct buffer *multiline, char *p[],
                           struct gc_arena *gc)
 {
@@ -4808,13 +4807,12 @@ read_config_file(struct options *options,
                 }
                 if (parse_line(line + offset, p, SIZE(p)-1, file, line_num, msglevel, &options->gc))
                 {
-                    bool is_inline;
-
                     bypass_doubledash(&p[0]);
-                    is_inline = check_inline_file_via_fp(fp, p, &options->gc);
-                    add_option(options, p, is_inline, file, line_num, level,
+                    int lines_inline = check_inline_file_via_fp(fp, p, &options->gc);
+                    add_option(options, p, lines_inline, file, line_num, level,
                                msglevel, permission_mask, option_types_found,
                                es);
+                    line_num += lines_inline;
                 }
             }
             if (fp != stdin)
@@ -4857,12 +4855,11 @@ read_config_string(const char *prefix,
         ++line_num;
         if (parse_line(line, p, SIZE(p)-1, prefix, line_num, msglevel, &options->gc))
         {
-            bool is_inline;
-
             bypass_doubledash(&p[0]);
-            is_inline = check_inline_file_via_buf(&multiline, p, &options->gc);
-            add_option(options, p, is_inline, prefix, line_num, 0, msglevel,
+            int lines_inline = check_inline_file_via_buf(&multiline, p, &options->gc);
+            add_option(options, p, lines_inline, prefix, line_num, 0, msglevel,
                        permission_mask, option_types_found, es);
+            line_num += lines_inline;
         }
         CLEAR(p);
     }
@@ -5311,13 +5308,14 @@ add_option(struct options *options,
         }
         if (good)
         {
-#if 0
-            /* removed for now since ECHO can potentially include
-             * security-sensitive strings */
-            msg(M_INFO, "%s:%s",
-                pull_mode ? "ECHO-PULL" : "ECHO",
-                BSTR(&string));
-#endif
+            /* only message-related ECHO are logged, since other ECHOs
+             * can potentially include security-sensitive strings */
+            if (strncmp(p[1], "msg", 3) == 0)
+            {
+                msg(M_INFO, "%s:%s",
+                    pull_mode ? "ECHO-PULL" : "ECHO",
+                    BSTR(&string));
+            }
 #ifdef ENABLE_MANAGEMENT
             if (management)
             {
diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
index f9bbfb5..3f472ef 100644
--- a/src/openvpn/pf.c
+++ b/src/openvpn/pf.c
@@ -639,8 +639,17 @@ pf_init_context(struct context *c)
         }
         if (!c->c2.pf.enabled)
         {
-            msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client.");
-            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
+            /* At some point in openvpn history, this code just printed a
+             * warning and signalled itself (SIGUSR1, "plugin-pf-init-failed")
+             * to terminate the client instance.  This got broken at one of
+             * the client auth state refactorings (leading to SIGSEGV crashes)
+             * and due to "pf will be removed anyway" reasons the easiest way
+             * to prevent crashes is to REQUIRE that plugins succeed - so if
+             * the plugin fails, we cleanly abort OpenVPN
+             *
+             * see also: https://community.openvpn.net/openvpn/ticket/1377
+             */
+            msg(M_FATAL, "FATAL: failed to init PF plugin, must succeed.");
             return;
         }
     }
diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index 2089e6b..5d76078 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -983,14 +983,38 @@ is_openvpn_protocol(const struct buffer *buf)
     const int len = BLEN(buf);
     if (len >= 3)
     {
-        return p[0] == 0
-               && p[1] >= 14
-               && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)
-                   || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT));
+        int plen = (p[0] << 8) | p[1];
+
+        if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT))
+        {
+            /* WKc is at least 290 byte (not including metadata):
+             *
+             * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit
+             *
+             * This is increased by the normal length of client handshake +
+             * tls-crypt overhead (32)
+             *
+             * For metadata tls-crypt-v2.txt does not explicitly specify
+             * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN
+             * as 1024 bytes. We err on the safe side with 255 extra overhead
+             *
+             * We don't do the 2 byte check for tls-crypt-v2 because it is very
+             * unrealistic to have only 2 bytes available.
+             */
+            return  (plen >= 336 && plen < (1024 + 255));
+        }
+        else
+        {
+            /* For non tls-crypt2 we assume the packet length to valid between
+             * 14 and 255 */
+            return plen >= 14 && plen <= 255
+                   && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT));
+        }
     }
     else if (len >= 2)
     {
-        return p[0] == 0 && p[1] >= 14;
+        int plen = (p[0] << 8) | p[1];
+        return plen >= 14 && plen <= 255;
     }
     else
     {
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index c6ba812..d7494c2 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -434,8 +434,6 @@ ssl_set_auth_nocache(void)
 {
     passbuf.nocache = true;
     auth_user_pass.nocache = true;
-    /* wait for push-reply, because auth-token may still need the username */
-    auth_user_pass.wait_for_push = true;
 }
 
 /*
@@ -2358,14 +2356,15 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
         }
         /* if auth-nocache was specified, the auth_user_pass object reaches
          * a "complete" state only after having received the push-reply
-         * message.
+         * message. The push message might contain an auth-token that needs
+         * the username of auth_user_pass.
          *
          * For this reason, skip the purge operation here if no push-reply
          * message has been received yet.
          *
          * This normally happens upon first negotiation only.
          */
-        if (!auth_user_pass.wait_for_push)
+        if (!session->opt->pull)
         {
             purge_user_pass(&auth_user_pass, false);
         }
@@ -4104,8 +4103,7 @@ done:
 }
 
 void
-delayed_auth_pass_purge(void)
+ssl_clean_user_pass(void)
 {
-    auth_user_pass.wait_for_push = false;
     purge_user_pass(&auth_user_pass, false);
 }
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 005628f..97d721b 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -598,7 +598,10 @@ void extract_x509_field_test(void);
  */
 bool is_hard_reset_method2(int op);
 
-void delayed_auth_pass_purge(void);
+/**
+ * Cleans the saved user/password unless auth-nocache is in use.
+ */
+void ssl_clean_user_pass(void);
 
 
 /*
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 8315a42..1767420 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -5212,7 +5212,7 @@ netsh_command(const struct argv *a, int n, int msglevel)
     for (i = 0; i < n; ++i)
     {
         bool status;
-        management_sleep(1);
+        management_sleep(0);
         netcmd_semaphore_lock();
         argv_msg_prefix(M_INFO, a, "NETSH");
         status = openvpn_execve_check(a, NULL, 0, "ERROR: netsh command failed");
@@ -5240,7 +5240,6 @@ ipconfig_register_dns(const struct env_set *es)
                 WIN_IPCONFIG_PATH_SUFFIX);
     argv_msg(D_TUNTAP_INFO, &argv);
     openvpn_execve_check(&argv, es, 0, err);
-    argv_free(&argv);
 
     argv_printf(&argv, "%s%s /registerdns",
                 get_win_sys_path(),
@@ -6160,35 +6159,35 @@ tuntap_set_ip_addr(struct tuntap *tt,
                 status,
                 strerror_win32(status, &gc));
         }
-    }
 
-    /*
-     * If the TAP-Windows driver is masquerading as a DHCP server
-     * make sure the TCP/IP properties for the adapter are
-     * set correctly.
-     */
-    if (dhcp_masq_post)
-    {
-        /* check dhcp enable status */
-        if (dhcp_status(index) == DHCP_STATUS_DISABLED)
+        /*
+         * If the TAP-Windows driver is masquerading as a DHCP server
+         * make sure the TCP/IP properties for the adapter are
+         * set correctly.
+         */
+        if (dhcp_masq_post)
         {
-            msg(M_WARN, "WARNING: You have selected '--ip-win32 dynamic', which will not work unless the TAP-Windows TCP/IP properties are set to 'Obtain an IP address automatically'");
-        }
+            /* check dhcp enable status */
+            if (dhcp_status(index) == DHCP_STATUS_DISABLED)
+            {
+                msg(M_WARN, "WARNING: You have selected '--ip-win32 dynamic', which will not work unless the TAP-Windows TCP/IP properties are set to 'Obtain an IP address automatically'");
+            }
 
-        /* force an explicit DHCP lease renewal on TAP adapter? */
-        if (tt->options.dhcp_pre_release)
-        {
-            dhcp_release(tt);
+            /* force an explicit DHCP lease renewal on TAP adapter? */
+            if (tt->options.dhcp_pre_release)
+            {
+                dhcp_release(tt);
+            }
+            if (tt->options.dhcp_renew)
+            {
+                dhcp_renew(tt);
+            }
         }
-        if (tt->options.dhcp_renew)
+        else
         {
-            dhcp_renew(tt);
+            fork_dhcp_action(tt);
         }
     }
-    else
-    {
-        fork_dhcp_action(tt);
-    }
 
     if (tt->did_ifconfig_setup && tt->options.ip_win32_type == IPW32_SET_IPAPI)
     {
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 65bb106..5d5cbfe 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1104,7 +1104,7 @@ wmic_nicconfig_cmd(const wchar_t *action, const NET_IFINDEX if_index,
     }
     else
     {
-       fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s %s";
+       fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s \"%s\"";
     }
 
     size_t ncmdline = wcslen(fmt) + 20 + wcslen(action) /* max 20 for ifindex */