Remove --memstats feature

The ``--mememstat`` was largely undocumented and there is no known
user of this feature.  This feature provided very limited statistics
(number of users, link bytes read/written) and we do not except any
usage because of this.

The only documentation was a mention in --help without any mention of
the (binary) format of the mmap file or other usage instructions.

This deals also with issues reported by zeropath regarding potentially
insecure handling of the file permission of the memory mapped file.

Reported-by: Joshua Rogers <[email protected]>
Found-by: ZeroPath (https://zeropath.com/)
Change-Id: I5f57e7bf52e3f6289462ef05e1f6e81ab0133d0d
Signed-off-by: Arne Schwabe <[email protected]>
Acked-by: Gert Doering <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1329
Message-Id: <[email protected]>
URL: https://www.mail-archive.com/[email protected]/msg34021.html
Signed-off-by: Gert Doering <[email protected]>

Author: schwabe

CMakeLists.txt

@@ -490,8 +490,6 @@ set(SOURCE_FILES
     src/openvpn/mroute.h
     src/openvpn/mss.c
     src/openvpn/mss.h
-    src/openvpn/mstats.c
-    src/openvpn/mstats.h
     src/openvpn/mtcp.c
     src/openvpn/mtcp.h
     src/openvpn/mtu.c

Changes.rst

@@ -217,6 +217,11 @@ Compression on send has been removed.
     ``--allow-compression yes`` is now an alias for
     ``--allow-compression asym``.
 
+``--memstats`` feature removed
+    The ``--mememstat`` was largely undocumented and there is no known
+    user of this feature.  This feature provided very limited statistics
+    (number of users, link bytes read/written) and we do not except any
+    usage because of this.
 
 User-visible Changes
 --------------------

src/openvpn/Makefile.am

@@ -90,7 +90,6 @@ openvpn_SOURCES = \
 	mbedtls_compat.h \
 	mroute.c mroute.h \
 	mss.c mss.h \
-	mstats.c mstats.h \
 	mtcp.c mtcp.h \
 	mtu.c mtu.h \
 	mudp.c mudp.h \

src/openvpn/error.c

@@ -37,8 +37,6 @@
 #include "status.h"
 #include "integer.h"
 #include "ps.h"
-#include "mstats.h"
-
 
 #if SYSLOG_CAPABILITY
 #ifndef LOG_OPENVPN
@@ -723,10 +721,6 @@ openvpn_exit(const int status)
         }
 #endif
 
-#ifdef ENABLE_MEMSTATS
-        mstats_close();
-#endif
-
 #ifdef ABORT_ON_ERROR
         if (status == OPENVPN_EXIT_STATUS_ERROR)
         {

src/openvpn/forward.c

@@ -44,8 +44,6 @@
 
 #include "memdbg.h"
 
-#include "mstats.h"
-
 counter_type link_read_bytes_global;  /* GLOBAL */
 counter_type link_write_bytes_global; /* GLOBAL */
 
@@ -992,12 +990,6 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
     {
         c->c2.link_read_bytes += c->c2.buf.len;
         link_read_bytes_global += c->c2.buf.len;
-#ifdef ENABLE_MEMSTATS
-        if (mmap_stats)
-        {
-            mmap_stats->link_read_bytes = link_read_bytes_global;
-        }
-#endif
         c->c2.original_recv_size = c->c2.buf.len;
     }
     else
@@ -1821,12 +1813,6 @@ process_outgoing_link(struct context *c, struct link_socket *sock)
                 c->c2.max_send_size_local = max_int(size, c->c2.max_send_size_local);
                 c->c2.link_write_bytes += size;
                 link_write_bytes_global += size;
-#ifdef ENABLE_MEMSTATS
-                if (mmap_stats)
-                {
-                    mmap_stats->link_write_bytes = link_write_bytes_global;
-                }
-#endif
             }
         }
 

src/openvpn/init.c

@@ -44,7 +44,6 @@
 #include "ps.h"
 #include "lladdr.h"
 #include "ping.h"
-#include "mstats.h"
 #include "ssl_verify.h"
 #include "ssl_ncp.h"
 #include "tls_crypt.h"
@@ -908,22 +907,6 @@ init_static(void)
     return false;
 #endif
 
-#ifdef MSTATS_TEST
-    {
-        int i;
-        mstats_open("/dev/shm/mstats.dat");
-        for (i = 0; i < 30; ++i)
-        {
-            mmap_stats->n_clients += 1;
-            mmap_stats->link_write_bytes += 8;
-            mmap_stats->link_read_bytes += 16;
-            sleep(1);
-        }
-        mstats_close();
-        return false;
-    }
-#endif
-
     return true;
 }
 
@@ -1233,13 +1216,6 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
             }
         }
 
-#ifdef ENABLE_MEMSTATS
-        if (c->first_time && c->options.memstats_fn)
-        {
-            mstats_open(c->options.memstats_fn);
-        }
-#endif
-
 #ifdef ENABLE_SELINUX
         /* Apply a SELinux context in order to restrict what OpenVPN can do
          * to _only_ what it is supposed to do after initialization is complete

src/openvpn/mstats.c

@@ -37,7 +37,6 @@
 #include "run_command.h"
 #include "otime.h"
 #include "gremlin.h"
-#include "mstats.h"
 #include "ssl_verify.h"
 #include "ssl_ncp.h"
 #include "vlan.h"
@@ -80,17 +79,6 @@ set_cc_config(struct multi_instance *mi, struct buffer_list *cc_config)
 }
 #endif
 
-static inline void
-update_mstat_n_clients(const int n_clients)
-{
-#ifdef ENABLE_MEMSTATS
-    if (mmap_stats)
-    {
-        mmap_stats->n_clients = n_clients;
-    }
-#endif
-}
-
 static bool
 learn_address_script(const struct multi_context *m, const struct multi_instance *mi, const char *op,
                      const struct mroute_addr *addr)
@@ -589,7 +577,6 @@ multi_close_instance(struct multi_context *m, struct multi_instance *mi, bool sh
 
     /* adjust current client connection count */
     m->n_clients += mi->n_clients_delta;
-    update_mstat_n_clients(m->n_clients);
     mi->n_clients_delta = 0;
 
     /* prevent dangling pointers */
@@ -2799,7 +2786,6 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
     /* increment number of current authenticated clients */
     ++m->n_clients;
-    update_mstat_n_clients(m->n_clients);
     --mi->n_clients_delta;
 
 #ifdef ENABLE_MANAGEMENT

src/openvpn/mstats.h

@@ -323,9 +323,6 @@ static const char usage_message[] =
     "                  via a VRF present on the system.\n"
 #endif
     "--txqueuelen n  : Set the tun/tap TX queue length to n (Linux only).\n"
-#ifdef ENABLE_MEMSTATS
-    "--memstats file : Write live usage stats to memory mapped binary file.\n"
-#endif
     "--mlock         : Disable Paging -- ensures key material and tunnel\n"
     "                  data will never be written to disk.\n"
     "--up cmd        : Run command cmd after successful tun device open.\n"
@@ -6333,13 +6330,6 @@ add_option(struct options *options, char *p[], bool is_inline, const char *file,
         options->log = true;
         redirect_stdout_stderr(p[1], true);
     }
-#ifdef ENABLE_MEMSTATS
-    else if (streq(p[0], "memstats") && p[1] && !p[2])
-    {
-        VERIFY_PERMISSION(OPT_P_GENERAL);
-        options->memstats_fn = p[1];
-    }
-#endif
     else if (streq(p[0], "mlock") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);