patch 9.1.1361: [security]: possible use-after-free when closing a buffer

Problem:  [security]: Possible to open more windows into a closing
          buffer without splitting, bypassing existing "b_locked_split"
          checks and triggering use-after-free
Solution: Disallow switching to a closing buffer. Editing a closing
          buffer (via ":edit", etc.) was fixed in v9.1.0764, but add an
          error message and check just "b_locked_split", as "b_locked"
          is necessary only when the buffer shouldn't be wiped, and may
          be set for buffers that are in-use but not actually closing.
          (Sean Dewar)

closes: #17246

Signed-off-by: Sean Dewar <6256228+seandewar@users.noreply.github.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

diff --git a/src/buffer.c b/src/buffer.c
index 697efa30cfac9dde282dd49da1b1111698979011..b4481b2f1373be0d41e79b5b871ee9cee20a7f63 100644 (file)
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -526,12 +526,6 @@ can_unload_buffer(buf_T *buf)
     return can_unload;
 }
 
-    int
-buf_locked(buf_T *buf)
-{
-    return buf->b_locked || buf->b_locked_split;
-}
-
 /*
  * Close the link to a buffer.
  * "action" is used when there is no longer a window for the buffer.
@@ -1432,12 +1426,19 @@ do_buffer_ext(
     if ((flags & DOBUF_NOPOPUP) && bt_popup(buf) && !bt_terminal(buf))
        return OK;
 #endif
-    if (
-       action == DOBUF_GOTO
-       && buf != curbuf
-       && !check_can_set_curbuf_forceit((flags & DOBUF_FORCEIT) ? TRUE : FALSE))
-      // disallow navigating to another buffer when 'winfixbuf' is applied
-      return FAIL;
+    if (action == DOBUF_GOTO && buf != curbuf)
+    {
+       if (!check_can_set_curbuf_forceit((flags & DOBUF_FORCEIT) != 0))
+           // disallow navigating to another buffer when 'winfixbuf' is applied
+           return FAIL;
+       if (buf->b_locked_split)
+       {
+           // disallow navigating to a closing buffer, which like splitting,
+           // can result in more windows displaying it
+           emsg(_(e_cannot_switch_to_a_closing_buffer));
+           return FAIL;
+       }
+    }
 
     if ((action == DOBUF_GOTO || action == DOBUF_SPLIT)
                                                  && (buf->b_flags & BF_DUMMY))

diff --git a/src/errors.h b/src/errors.h
index 90cc2b143ef0555a332fd0b11b2a7a446a40ae8a..d718507e0131dfbd4d397b7e7145dbf9098b4eec 100644 (file)
--- a/src/errors.h
+++ b/src/errors.h
@@ -3728,3 +3728,5 @@ EXTERN char e_failed_resizing_quickfix_stack[]
 EXTERN char e_no_quickfix_stack[]
        INIT(= N_("E1545: Quickfix list stack unavailable"));
 #endif
+EXTERN char e_cannot_switch_to_a_closing_buffer[]
+       INIT(= N_("E1546: Cannot switch to a closing buffer"));

diff --git a/src/ex_cmds.c b/src/ex_cmds.c
index e1e6c4ee7c22431d8b4526644de6e10d3df7abc2..4eb67e31facbc9cd87dff5f136dad1935877a721 100644 (file)
--- a/src/ex_cmds.c
+++ b/src/ex_cmds.c
@@ -2743,9 +2743,9 @@ do_ecmd(
        }
        if (buf == NULL)
            goto theend;
-       // autocommands try to edit a file that is going to be removed,
-       // abort
-       if (buf_locked(buf))
+       // autocommands try to edit a closing buffer, which like splitting, can
+       // result in more windows displaying it; abort
+       if (buf->b_locked_split)
        {
            // window was split, but not editing the new buffer,
            // reset b_nwindows again
@@ -2753,6 +2753,7 @@ do_ecmd(
                    && curwin->w_buffer != NULL
                    && curwin->w_buffer->b_nwindows > 1)
                --curwin->w_buffer->b_nwindows;
+           emsg(_(e_cannot_switch_to_a_closing_buffer));
            goto theend;
        }
        if (curwin->w_alt_fnum == buf->b_fnum && prev_alt_fnum != 0)

diff --git a/src/proto/buffer.pro b/src/proto/buffer.pro
index 8384601ceb110fb772974aaaacb00a7a4dda16f8..9044bae83f9d3a339aaf7e4c448b60317fb0791e 100644 (file)
--- a/src/proto/buffer.pro
+++ b/src/proto/buffer.pro
@@ -5,7 +5,6 @@ int open_buffer(int read_stdin, exarg_T *eap, int flags_arg);
 void set_bufref(bufref_T *bufref, buf_T *buf);
 int bufref_valid(bufref_T *bufref);
 int buf_valid(buf_T *buf);
-int buf_locked(buf_T *buf);
 int close_buffer(win_T *win, buf_T *buf, int action, int abort_if_last, int ignore_abort);
 void buf_clear_file(buf_T *buf);
 void buf_freeall(buf_T *buf, int flags);

diff --git a/src/structs.h b/src/structs.h
index 898f620efe133a0fa581902cb56f35e5e047f87a..caf61eda8b9d48bab4575d29ffb852d62e0be7f7 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -3072,7 +3072,7 @@ struct file_buffer
     int                b_locked;       // Buffer is being closed or referenced, don't
                                // let autocommands wipe it out.
     int                b_locked_split; // Buffer is being closed, don't allow opening
-                               // a new window with it.
+                               // it in more windows.
 
     /*
      * b_ffname has the full path of the file (NULL for no name).

diff --git a/src/testdir/test_autocmd.vim b/src/testdir/test_autocmd.vim
index d5d516ce24f08fe6ff5211eeaf84121187742852..8d083db742cad4ebc8d824cf378d6ff1c367efd8 100644 (file)
--- a/src/testdir/test_autocmd.vim
+++ b/src/testdir/test_autocmd.vim
@@ -5035,7 +5035,8 @@ func Test_autocmd_BufWinLeave_with_vsp()
   exe "e " fname
   vsp
   augroup testing
-    exe "au BufWinLeave " .. fname .. " :e " dummy .. "| vsp " .. fname
+    exe 'au BufWinLeave' fname 'e' dummy
+          \ '| call assert_fails(''vsp' fname ''', ''E1546:'')'
   augroup END
   bw
   call CleanUpTestAuGroup()

diff --git a/src/testdir/test_buffer.vim b/src/testdir/test_buffer.vim
index 757ba05784f79e72c50f0fa0e4f61e5bb7668f12..36a6ef50350c862c0ac6860bfec524bc5486c81e 100644 (file)
--- a/src/testdir/test_buffer.vim
+++ b/src/testdir/test_buffer.vim
@@ -569,4 +569,39 @@ func Test_buflist_alloc_failure()
   call assert_fails('cexpr "XallocFail6:10:Line10"', 'E342:')
 endfunc
 
+func Test_closed_buffer_still_in_window()
+  %bw!
+
+  let s:w = win_getid()
+  new
+  let s:b = bufnr()
+  setl bufhidden=wipe
+
+  augroup ViewClosedBuffer
+    autocmd!
+    autocmd BufUnload * ++once call assert_fails(
+          \ 'call win_execute(s:w, "' .. s:b .. 'b")', 'E1546:')
+  augroup END
+  quit!
+  " Previously resulted in s:b being curbuf while unloaded (no memfile).
+  call assert_equal(1, bufloaded(bufnr()))
+  call assert_equal(0, bufexists(s:b))
+
+  let s:w = win_getid()
+  split
+  new
+  let s:b = bufnr()
+
+  augroup ViewClosedBuffer
+    autocmd!
+    autocmd BufWipeout * ++once call win_gotoid(s:w)
+          \| call assert_fails(s:b .. 'b', 'E1546:') | wincmd p
+  augroup END
+  bw! " Close only this buffer first; used to be a heap UAF.
+
+  unlet! s:w s:b
+  autocmd! ViewClosedBuffer
+  %bw!
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

diff --git a/src/version.c b/src/version.c
index c0c98b40ba225621aa4b88d389a0dab77daf06fb..7d29d7020bc492414b3a1c8dff0e5e660907186c 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -704,6 +704,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1361,
 /**/
     1360,
 /**/