patch 9.2.0842: [security]: stack buffer overflow in socket server

Problem:  [security]: stack buffer overflow in socket server
          (tdjackey)
Solution: Cap accepted socketserver clients (Yasuhiro Matsumoto)

Github Security Advisory:
https://github.com/vim/vim/security/advisories/GHSA-49m8-wwxj-mr69

Signed-off-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

diff --git a/src/channel.c b/src/channel.c
index d7fa3bd2c729ebf184ecce18940724cf9858fd73..0519a157be3fb0b409aeb0604e5e124e19735d2f 100644 (file)
--- a/src/channel.c
+++ b/src/channel.c
@@ -4028,6 +4028,9 @@ channel_fill_wfds(int maxfd_arg, fd_set *wfds)
        chanpart_T  *in_part = &ch->ch_part[PART_IN];
 
        if (in_part->ch_fd != INVALID_FD
+# ifdef FD_SETSIZE
+               && (int)in_part->ch_fd < FD_SETSIZE
+# endif
                && is_channel_write_remaining(in_part))
        {
            FD_SET((int)in_part->ch_fd, wfds);
@@ -4165,7 +4168,7 @@ channel_wait(channel_T *channel, sock_T fd, int timeout)
 #else
        for (;;)
        {
-           struct pollfd   fds[MAX_OPEN_CHANNELS + 1];
+           struct pollfd   fds[MAX_OPEN_CHANNELS + 1 + MAX_CLIENT_CHANNELS];
            int             nfd = 1;
 
            fds[0].fd = fd;
@@ -5415,6 +5418,12 @@ channel_select_setup(
                }
                else
                {
+# ifdef FD_SETSIZE
+                   // An fd that does not fit in the fd_set cannot be watched
+                   // with select(); skip it rather than overflow the set.
+                   if ((int)fd >= FD_SETSIZE)
+                       continue;
+# endif
                    FD_SET((int)fd, rfds);
                    if (maxfd < (int)fd)
                        maxfd = (int)fd;
@@ -5447,7 +5456,11 @@ channel_select_check(int ret_in, void *rfds_in, void *wfds_in)
        {
            sock_T fd = channel->ch_part[part].ch_fd;
 
-           if (ret > 0 && fd != INVALID_FD && FD_ISSET(fd, rfds))
+           if (ret > 0 && fd != INVALID_FD
+# ifdef FD_SETSIZE
+                   && (int)fd < FD_SETSIZE
+# endif
+                   && FD_ISSET(fd, rfds))
            {
                channel_read(channel, part, "channel_select_check");
                FD_CLR(fd, rfds);
@@ -5462,6 +5475,9 @@ channel_select_check(int ret_in, void *rfds_in, void *wfds_in)
 
        in_part = &channel->ch_part[PART_IN];
        if (ret > 0 && in_part->ch_fd != INVALID_FD
+# ifdef FD_SETSIZE
+                   && (int)in_part->ch_fd < FD_SETSIZE
+# endif
                                            && FD_ISSET(in_part->ch_fd, wfds))
        {
            // Clear the flag first, ch_fd may change in channel_write_input().

diff --git a/src/os_unix.c b/src/os_unix.c
index 904421772ea9b791fb3e28573751d6b757b521d0..85b4353c43fb9901aafe064b7b841a91bcfd68ac 100644 (file)
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -6798,7 +6798,7 @@ RealWaitForChar(int fd, long msec, int *check_for_gpm UNUSED, int *interrupted)
 # endif
 # ifndef HAVE_SELECT
                        // each channel may use in, out and err
-       struct pollfd   fds[7 + 3 * MAX_OPEN_CHANNELS];
+       struct pollfd   fds[7 + 3 * MAX_OPEN_CHANNELS + 2 * MAX_CLIENT_CHANNELS];
        int             nfd;
 #  ifdef FEAT_WAYLAND_CLIPBOARD
        int             wayland_idx = -1;

diff --git a/src/socketserver.c b/src/socketserver.c
index 44107bd4a33d27836b0f423b8ac93730073090ec..e693123e133407fb5f065e55e9fa95b786dffc71 100644 (file)
--- a/src/socketserver.c
+++ b/src/socketserver.c
@@ -57,6 +57,7 @@ static char_u     *server_addr_cwd = NULL; // CWD when server was started, used
 static garray_T            server_replies;
 
 static channel_T    *client_channels = NULL;
+static int         client_channel_count = 0;
 
 # define FOR_ALL_CLIENTS(ch) \
     for (ch = client_channels; ch != NULL; ch = ch->ch_ss_next)
@@ -538,6 +539,8 @@ socketserver_client_close(channel_T *channel)
        channel->ch_ss_prev->ch_ss_next = channel->ch_ss_next;
     if (channel->ch_ss_next != NULL)
        channel->ch_ss_next->ch_ss_prev = channel->ch_ss_prev;
+    if (client_channel_count > 0)
+       --client_channel_count;
 
     ch_log(NULL, "socketserver: client channel closed");
 }
@@ -548,6 +551,16 @@ socketserver_client_close(channel_T *channel)
     static void
 socketserver_accept(channel_T *channel)
 {
+    if (client_channel_count >= MAX_CLIENT_CHANNELS)
+    {
+       // Too many client connections: refuse this one instead of letting the
+       // channel's file descriptor overflow the select()/poll() sets.
+       ch_log(NULL, "socketserver: too many clients, refusing connection");
+       channel_close(channel, FALSE);
+       channel_unref(channel);
+       return;
+    }
+
     channel->ch_socketserver = true;
     channel->ch_ss_close_cb = socketserver_client_close;
 
@@ -568,6 +581,7 @@ socketserver_accept(channel_T *channel)
        last->ch_ss_next = channel;
        channel->ch_ss_prev = last;
     }
+    ++client_channel_count;
 
     // We will read the command from the client later in the input loop.
     ch_log(NULL, "socketserver: accepted new client");
@@ -872,7 +886,11 @@ socketserver_wait(channel_T *channel, int timeout)
 
        FOR_ALL_CLIENTS(ch)
        {
-           if (ch->CH_SOCK_FD != INVALID_FD)
+           if (ch->CH_SOCK_FD != INVALID_FD
+#  ifdef FD_SETSIZE
+                   && (int)ch->CH_SOCK_FD < FD_SETSIZE
+#  endif
+                   )
            {
                FD_SET(ch->CH_SOCK_FD, &rfds);
                if (maxfd < (int)ch->CH_SOCK_FD)
@@ -903,6 +921,9 @@ socketserver_wait(channel_T *channel, int timeout)
 
            FOR_ALL_CLIENTS(ch)
                if (ch->CH_SOCK_FD != INVALID_FD
+#  ifdef FD_SETSIZE
+                       && (int)ch->CH_SOCK_FD < FD_SETSIZE
+#  endif
                        && FD_ISSET(ch->CH_SOCK_FD, &rfds))
                    channel_check(ch, PART_SOCK);
 
@@ -920,7 +941,7 @@ socketserver_wait(channel_T *channel, int timeout)
            continue;
        }
 # else
-       struct pollfd   fds[MAX_OPEN_CHANNELS + 1];
+       struct pollfd   fds[MAX_OPEN_CHANNELS + 1 + MAX_CLIENT_CHANNELS];
        int             nfd = 0;
        int             channel_idx = -1;
        int             server_idx = -1;

diff --git a/src/testdir/test_clientserver.vim b/src/testdir/test_clientserver.vim
index 3c39d01c240b7ee14fa7d2ce80285008a46919a2..d5c243fda64ac6fcd39467d4c6eab0c85e5eaa68 100644 (file)
--- a/src/testdir/test_clientserver.vim
+++ b/src/testdir/test_clientserver.vim
@@ -373,6 +373,60 @@ func Test_client_server_socketserver_address()
   endtry
 endfunc
 
+" A flood of client connections must not crash the socketserver: connections
+" beyond the accept cap are refused instead of overflowing the fd_set / pollfd
+" sets, and the server keeps working once they are closed again.
+func Test_client_server_socketserver_connection_flood()
+  CheckFeature socketserver
+  CheckNotMSWindows
+
+  let g:test_is_flaky = 1
+  let cmd = GetVimCommand()
+  if cmd == ''
+    throw 'GetVimCommand() failed'
+  endif
+
+  let actual = cmd .. ' --clientserver socket --servername channel:2001'
+  let job = job_start(actual, {'stoponexit': 'kill', 'out_io': 'null'})
+  call WaitForAssert({-> assert_equal("run", job_status(job))})
+  call WaitForAssert({-> assert_match('channel:2001',
+        \ system(actual .. ' --remote-expr "v:servername"'))})
+
+  " Open many more connections than the server accepts.
+  let channels = []
+  for i in range(150)
+    let ch = ch_open('localhost:2001', {'mode': 'raw', 'waittime': 100})
+    if ch_status(ch) == 'open'
+      call add(channels, ch)
+    endif
+  endfor
+
+  " The server must survive, and must have refused the excess connections.
+  call assert_equal("run", job_status(job))
+  call WaitForAssert({-> assert_inrange(1, 100,
+        \ len(filter(copy(channels), {_, c -> ch_status(c) == 'open'})))})
+
+  " Close them again.  Afterwards the server must accept connections once
+  " more, which also verifies the client count is decremented.
+  for ch in channels
+    if ch_status(ch) == 'open'
+      call ch_close(ch)
+    endif
+  endfor
+  call WaitForAssert({-> assert_match('channel:2001',
+        \ system(actual .. ' --remote-expr "v:servername"'))})
+
+  call system(actual .. " --remote-expr 'execute(\"qa!\")'")
+  try
+    call WaitForAssert({-> assert_equal("dead", job_status(job))})
+  finally
+    if job_status(job) != 'dead'
+      call assert_report('Server did not exit')
+      call job_stop(job, 'kill')
+    endif
+  endtry
+endfunc
+
 " Test if --remote-wait works properly with multiple files
 func Test_client_server_multiple_remote_wait()
   CheckRunVimInTerminal

diff --git a/src/version.c b/src/version.c
index 9afff74b32cce98f7d1f12eaf1c95b55c749336c..a5cca3061b8acf97b5473588b02940ad94a32620 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -758,6 +758,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    842,
 /**/
     841,
 /**/

diff --git a/src/vim.h b/src/vim.h
index c04e5f389cd5fc88b01e3c0e2e06c1a4e66ea21c..408cf711d1184b04790dc85baaa983a875b72503 100644 (file)
--- a/src/vim.h
+++ b/src/vim.h
@@ -2904,6 +2904,15 @@ typedef int (*opt_expand_cb_T)(optexpand_T *args, int *numMatches, char_u ***mat
 # define MAX_OPEN_CHANNELS 0
 #endif
 
+// Maximum number of simultaneously accepted socketserver client channels.
+// Bounds the channels (and file descriptors) added to the select()/poll()
+// sets so a connection flood cannot overflow the fd_set / pollfd arrays.
+#ifdef FEAT_SOCKETSERVER
+# define MAX_CLIENT_CHANNELS 100
+#else
+# define MAX_CLIENT_CHANNELS 0
+#endif
+
 #if defined(MSWIN)
 # define MAX_NAMED_PIPE_SIZE 65535
 #endif