neovim

commit bebdf1dab345471222f6755c574d04596fea92fd
parent 0e11bf0e1af5b3422db49222ab739a64d233b353
Author: zeertzjq <zeertzjq@outlook.com>
Date:   Sun,  3 Sep 2023 13:47:55 +0800

vim-patch:9.0.1848: [security] buffer-overflow in vim_regsub_both() (#25001)

Problem:  buffer-overflow in vim_regsub_both()
Solution: Check remaining space

https://github.com/vim/vim/commit/ced2c7394aafdc90fb7845e09b3a3fee23d48cb1

The change to do_sub() looks confusing. Maybe it's an overflow check?
Then the crash may not be applicable to Nvim because of different casts.
The test also looks confusing. It seems to source itself recursively.
Also don't call strlen() twice on evaluation result.

N/A patches for version.c:
vim-patch:9.0.1849: CI error on different signedness in ex_cmds.c
vim-patch:9.0.1853: CI error on different signedness in regexp.c

Co-authored-by: Christian Brabandt <cb@256bit.org>
Diffstat:
M
src/nvim/ex_cmds.c
 | 
10
+++++++---
M
src/nvim/regexp.c
 | 
5
+++--
A
test/old/testdir/crash/vim_regsub_both
 | 
10
++++++++++
M
test/old/testdir/test_crash.vim
 | 
11
++++++++++-

4 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c
@@ -3953,15 +3953,19 @@ static int do_sub(exarg_T *eap, const proftime_T timeout, const long cmdpreview_
             p1 = ml_get(sub_firstlnum + (linenr_T)nmatch - 1);
             nmatch_tl += nmatch - 1;
           }
-          size_t copy_len = (size_t)(regmatch.startpos[0].col - copycol);
+          int copy_len = regmatch.startpos[0].col - copycol;
           new_end = sub_grow_buf(&new_start, &new_start_len,
                                  (colnr_T)strlen(p1) - regmatch.endpos[0].col
-                                 + (colnr_T)copy_len + sublen + 1);
+                                 + copy_len + sublen + 1);
 
           // copy the text up to the part that matched
-          memmove(new_end, sub_firstline + copycol, copy_len);
+          memmove(new_end, sub_firstline + copycol, (size_t)copy_len);
           new_end += copy_len;
 
+          if (new_start_len - copy_len < sublen) {
+            sublen = new_start_len - copy_len - 1;
+          }
+
           // Finally, at this point we can know where the match actually will
           // start in the new text
           int start_col = (int)(new_end - new_start);
diff --git a/src/nvim/regexp.c b/src/nvim/regexp.c
@@ -1765,9 +1765,10 @@ static int vim_regsub_both(char *source, typval_T *expr, char *dest, int destlen
     // "flags & REGSUB_COPY" == 0 to the call with
     // "flags & REGSUB_COPY" != 0.
     if (copy) {
-      if (eval_result[nested] != NULL) {
+      size_t reslen = eval_result[nested] != NULL ? strlen(eval_result[nested]) : 0;
+      if (eval_result[nested] != NULL && reslen < (size_t)destlen) {
         STRCPY(dest, eval_result[nested]);
-        dst += strlen(eval_result[nested]);
+        dst += reslen;
         XFREE_CLEAR(eval_result[nested]);
       }
     } else {
diff --git a/test/old/testdir/crash/vim_regsub_both b/test/old/testdir/crash/vim_regsub_both
@@ -0,0 +1,10 @@
+fu R()
+sil!norm0z=
+endf
+cal R()
+s/\%')/\=R()
+d
+no0 normyynore sm:vs0@vvvvvvvvvvse()dir(¼Xtest=csd{so88
+vs
+0scr
+so
diff --git a/test/old/testdir/test_crash.vim b/test/old/testdir/test_crash.vim
@@ -6,7 +6,7 @@ CheckScreendump
 
 func Test_crash1()
   " The following used to crash Vim
-  let opts = #{wait_for_ruler: 0}
+  let opts = #{wait_for_ruler: 0, rows: 20}
   let args = ' -u NONE -i NONE -n -e -s -S '
   let buf = RunVimInTerminal(args .. ' crash/poc_huaf1', opts)
   call VerifyScreenDump(buf, 'Test_crash_01', {})
@@ -22,4 +22,13 @@ func Test_crash1()
 
 endfunc
 
+func Test_crash2()
+  " The following used to crash Vim
+  let opts = #{wait_for_ruler: 0, rows: 20}
+  let args = ' -u NONE -i NONE -n -e -s -S '
+  let buf = RunVimInTerminal(args .. ' crash/vim_regsub_both', opts)
+  call VerifyScreenDump(buf, 'Test_crash_01', {})
+  exe buf .. "bw!"
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab