From 7402655132f12f4181707dfc307636a2f6a21863 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 2 Dec 2023 10:00:46 +0800 Subject: [PATCH 1/4] vim-patch:9.0.2140: [security]: use-after-free in win-enter Problem: [security]: use-after-free in win-enter Solution: validate window pointer before calling win_enter() win_goto() may stop visual mode, if it is active. However, this may in turn trigger the ModeChanged autocommand, which could potentially free the wp pointer which was valid before now became stale and points to now freed memory. So before calling win_enter(), let's verify one more time, that the wp pointer still points to a valid window structure. Reported by @henices, thanks! https://github.com/vim/vim/commit/eec0c2b3a4cfab93dd8d4adaa60638d47a2bbc8a Co-authored-by: Christian Brabandt --- src/nvim/window.c | 17 ++++++++++++----- test/functional/legacy/crash_spec.lua | 9 ++++++++- test/old/testdir/crash/poc_win_enter_ext | Bin 0 -> 1958 bytes test/old/testdir/test_crash.vim | 8 ++++++++ 4 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 test/old/testdir/crash/poc_win_enter_ext diff --git a/src/nvim/window.c b/src/nvim/window.c index 7728efde33..ef1ef89d95 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -4470,11 +4470,12 @@ void tabpage_move(int nr) redraw_tabline = true; } -// Go to another window. -// When jumping to another buffer, stop Visual mode. Do this before -// changing windows so we can yank the selection into the '*' register. -// When jumping to another window on the same buffer, adjust its cursor -// position to keep the same Visual area. +/// Go to another window. +/// When jumping to another buffer, stop Visual mode. Do this before +/// changing windows so we can yank the selection into the '*' register. +/// (note: this may trigger ModeChanged autocommand!) +/// When jumping to another window on the same buffer, adjust its cursor +/// position to keep the same Visual area. void win_goto(win_T *wp) { win_T *owp = curwin; @@ -4485,11 +4486,17 @@ void win_goto(win_T *wp) } if (wp->w_buffer != curbuf) { + // careful: triggers ModeChanged autocommand reset_VIsual_and_resel(); } else if (VIsual_active) { wp->w_cursor = curwin->w_cursor; } + // autocommand may have made wp invalid + if (!win_valid(wp)) { + return; + } + win_enter(wp, true); // Conceal cursor line in previous window, unconceal in current window. diff --git a/test/functional/legacy/crash_spec.lua b/test/functional/legacy/crash_spec.lua index 5094f81847..979ebd9e33 100644 --- a/test/functional/legacy/crash_spec.lua +++ b/test/functional/legacy/crash_spec.lua @@ -6,7 +6,6 @@ local feed = helpers.feed before_each(clear) --- oldtest: Test_crash1() it('no crash when ending Visual mode while editing buffer closes window', function() command('new') command('autocmd ModeChanged v:n ++once close') @@ -14,3 +13,11 @@ it('no crash when ending Visual mode while editing buffer closes window', functi command('enew') assert_alive() end) + +it('no crash when ending Visual mode close the window to switch to', function() + command('new') + command('autocmd ModeChanged v:n ++once only') + feed('v') + command('wincmd p') + assert_alive() +end) diff --git a/test/old/testdir/crash/poc_win_enter_ext b/test/old/testdir/crash/poc_win_enter_ext new file mode 100644 index 0000000000000000000000000000000000000000..73f53b575b699046d6bdc5402083dfa9c278769a GIT binary patch literal 1958 zcmd^9L2FY%5Z-|Akcri43qb{O0-=Jo65`23usv7{K}3RhNhsFcCWV*x@V$s03`vig z3ZB)g1ia{>{sPZAu{VSUu{8i6hx_5y zV1H?;Fu;W1y>n;zB5;HdNPy=Djlu&&Jc;wfKg8RU^(Dls38aAV5#k*C{mn)p#Af$| z`_^@#O{F|YY774GP3w1|nFH6m;?@05iY&*IDKeH14NE@rb*m9yZO%k)GE0tt&REH6DU6#T$ z#*r{Kj8aT{N(U-x(Fo;TIcxe^J2xc^Y!uUJ#}zO4;68N02xHq1Xd;fI!iYqXfZA&a zJhu*th}qW{48x<|4ml9wOuSsIp1F0F4}baKxNWgJp*I?6>4>JMw0t@J#?0EZI~wZK z7!7lqoRx>~Q6)R<-Yok_wc)H7`#;QxJ&*DKw_0*|F#k+XhwtMt)l`hkj-4wLhv-wB kbK+a;_22+f{79z`^d3}HQGA0`3ZCqE(kJ^S^AjU~0iVaYUjP6A literal 0 HcmV?d00001 diff --git a/test/old/testdir/test_crash.vim b/test/old/testdir/test_crash.vim index b093b053c5..11c5f4e014 100644 --- a/test/old/testdir/test_crash.vim +++ b/test/old/testdir/test_crash.vim @@ -128,6 +128,13 @@ func Test_crash1_2() \ ' && echo "crash 1: [OK]" > '.. result .. "\") call TermWait(buf, 150) + let file = 'crash/poc_win_enter_ext' + let cmn_args = "%s -u NONE -i NONE -n -e -s -S %s -c ':qa!'" + let args = printf(cmn_args, vim, file) + call term_sendkeys(buf, args .. + \ ' && echo "crash 2: [OK]" >> '.. result .. "\") + call TermWait(buf, 350) + " clean up exe buf .. "bw!" @@ -135,6 +142,7 @@ func Test_crash1_2() let expected = [ \ 'crash 1: [OK]', + \ 'crash 2: [OK]', \ ] call assert_equal(expected, getline(1, '$')) From 01edcd6db85ab2abffa95bc4dce6cfb8de617bca Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 2 Dec 2023 10:07:13 +0800 Subject: [PATCH 2/4] vim-patch:9.0.2141: [security]: buffer-overflow in suggest_trie_walk Problem: [security]: buffer-overflow in suggest_trie_walk Solution: Check n before using it as index into byts array Basically, n as an index into the byts array, can point to beyond the byts array. So let's double check, that n is within the expected range after incrementing it from sp->ts_curi and bail out if it would be invalid. Reported by @henices, thanks! https://github.com/vim/vim/commit/0fb375aae608d7306b4baf9c1f906961f32e2abf Co-authored-by: Christian Brabandt --- src/nvim/spellsuggest.c | 6 ++++++ test/old/testdir/crash/poc_suggest_trie_walk | Bin 0 -> 100 bytes test/old/testdir/test_crash.vim | 8 ++++++++ 3 files changed, 14 insertions(+) create mode 100644 test/old/testdir/crash/poc_suggest_trie_walk diff --git a/src/nvim/spellsuggest.c b/src/nvim/spellsuggest.c index bdac5aa587..efd647a5c3 100644 --- a/src/nvim/spellsuggest.c +++ b/src/nvim/spellsuggest.c @@ -1941,6 +1941,12 @@ static void suggest_trie_walk(suginfo_T *su, langp_T *lp, char *fword, bool soun // - Skip the byte if it's equal to the byte in the word, // accepting that byte is always better. n += sp->ts_curi++; + + // break out, if we would be accessing byts buffer out of bounds + if (byts == slang->sl_fbyts && n >= slang->sl_fbyts_len) { + got_int = true; + break; + } c = byts[n]; if (soundfold && sp->ts_twordlen == 0 && c == '*') { // Inserting a vowel at the start of a word counts less, diff --git a/test/old/testdir/crash/poc_suggest_trie_walk b/test/old/testdir/crash/poc_suggest_trie_walk new file mode 100644 index 0000000000000000000000000000000000000000..c79b6eeb5c340057d353903142910f4df292581a GIT binary patch literal 100 zcmXR;RY=WC&QHnAOSer)O)5=i;L6K4P%ua|s0!CL++e^}oSCDTmtT}CR#0MTAn}5k Y0U=^wT4agDlK`;|Kx$QxM5}BW0AW)fkN^Mx literal 0 HcmV?d00001 diff --git a/test/old/testdir/test_crash.vim b/test/old/testdir/test_crash.vim index 11c5f4e014..eef1731454 100644 --- a/test/old/testdir/test_crash.vim +++ b/test/old/testdir/test_crash.vim @@ -135,6 +135,13 @@ func Test_crash1_2() \ ' && echo "crash 2: [OK]" >> '.. result .. "\") call TermWait(buf, 350) + let file = 'crash/poc_suggest_trie_walk' + let cmn_args = "%s -u NONE -i NONE -n -e -s -S %s -c ':qa!'" + let args = printf(cmn_args, vim, file) + call term_sendkeys(buf, args .. + \ ' && echo "crash 3: [OK]" >> '.. result .. "\") + call TermWait(buf, 150) + " clean up exe buf .. "bw!" @@ -143,6 +150,7 @@ func Test_crash1_2() let expected = [ \ 'crash 1: [OK]', \ 'crash 2: [OK]', + \ 'crash 3: [OK]', \ ] call assert_equal(expected, getline(1, '$')) From 9cc346119bee505e0be3827b35c573701a307001 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 2 Dec 2023 10:10:25 +0800 Subject: [PATCH 3/4] vim-patch:9.0.2142: [security]: stack-buffer-overflow in option callback functions Problem: [security]: stack-buffer-overflow in option callback functions Solution: pass size of errbuf down the call stack, use snprintf() instead of sprintf() We pass the error buffer down to the option callback functions, but in some parts of the code, we simply use sprintf(buf) to write into the error buffer, which can overflow. So let's pass down the length of the error buffer and use sprintf(buf, size) instead. Reported by @henices, thanks! https://github.com/vim/vim/commit/b39b240c386a5a29241415541f1c99e2e6b8ce47 Co-authored-by: Christian Brabandt --- src/nvim/option.c | 4 ++-- src/nvim/option_defs.h | 1 + src/nvim/option_vars.h | 2 ++ src/nvim/optionstr.c | 2 +- test/functional/legacy/crash_spec.lua | 10 ++++++++++ test/functional/legacy/messages_spec.lua | 5 ----- test/old/testdir/crash/poc_did_set_langmap | 1 + test/old/testdir/test_crash.vim | 8 ++++++++ 8 files changed, 25 insertions(+), 8 deletions(-) create mode 100644 test/old/testdir/crash/poc_did_set_langmap diff --git a/src/nvim/option.c b/src/nvim/option.c index 7a7cda2fa0..ba9d1262d4 100644 --- a/src/nvim/option.c +++ b/src/nvim/option.c @@ -1455,7 +1455,7 @@ int do_set(char *arg, int opt_flags) } else { char *startarg = arg; // remember for error message const char *errmsg = NULL; - char errbuf[80]; + char errbuf[ERR_BUFLEN]; do_one_set_option(opt_flags, &arg, &did_show, errbuf, sizeof(errbuf), &errmsg); @@ -3845,7 +3845,7 @@ const char *set_option_value(const char *const name, const OptVal value, int opt int opt_idx = findoption(name); if (opt_idx < 0) { - snprintf(errbuf, IOSIZE, _(e_unknown_option2), name); + snprintf(errbuf, sizeof(errbuf), _(e_unknown_option2), name); return errbuf; } diff --git a/src/nvim/option_defs.h b/src/nvim/option_defs.h index b2e8081a08..6d0401f319 100644 --- a/src/nvim/option_defs.h +++ b/src/nvim/option_defs.h @@ -66,6 +66,7 @@ typedef struct { /// is parameterized, then the "os_errbuf" buffer is used to store the error /// message (when it is not NULL). char *os_errbuf; + /// length of the error buffer size_t os_errbuflen; void *os_win; diff --git a/src/nvim/option_vars.h b/src/nvim/option_vars.h index b0e9ff9434..66185940ca 100644 --- a/src/nvim/option_vars.h +++ b/src/nvim/option_vars.h @@ -938,6 +938,8 @@ enum { // Value for b_p_ul indicating the global value must be used. #define NO_LOCAL_UNDOLEVEL (-123456) +#define ERR_BUFLEN 80 + #define SB_MAX 100000 // Maximum 'scrollback' value. #define MAX_NUMBERWIDTH 20 // used for 'numberwidth' and 'statuscolumn' diff --git a/src/nvim/optionstr.c b/src/nvim/optionstr.c index 0a7d77e817..544524dd42 100644 --- a/src/nvim/optionstr.c +++ b/src/nvim/optionstr.c @@ -442,7 +442,7 @@ int check_signcolumn(win_T *wp) const char *check_stl_option(char *s) { int groupdepth = 0; - static char errbuf[80]; + static char errbuf[ERR_BUFLEN]; while (*s) { // Check for valid keys after % sequences diff --git a/test/functional/legacy/crash_spec.lua b/test/functional/legacy/crash_spec.lua index 979ebd9e33..094bea253e 100644 --- a/test/functional/legacy/crash_spec.lua +++ b/test/functional/legacy/crash_spec.lua @@ -21,3 +21,13 @@ it('no crash when ending Visual mode close the window to switch to', function() command('wincmd p') assert_alive() end) + +it('no crash when truncating overlong message', function() + pcall(command, 'source test/old/testdir/crash/vim_msg_trunc_poc') + assert_alive() +end) + +it('no crash with very long option error message', function() + pcall(command, 'source test/old/testdir/crash/poc_did_set_langmap') + assert_alive() +end) diff --git a/test/functional/legacy/messages_spec.lua b/test/functional/legacy/messages_spec.lua index e0cc1dc79c..146b00acb0 100644 --- a/test/functional/legacy/messages_spec.lua +++ b/test/functional/legacy/messages_spec.lua @@ -801,9 +801,4 @@ describe('messages', function() ]]) os.remove('b.txt') end) - - it('no crash when truncating overlong message', function() - pcall(command, 'source test/old/testdir/crash/vim_msg_trunc_poc') - assert_alive() - end) end) diff --git a/test/old/testdir/crash/poc_did_set_langmap b/test/old/testdir/crash/poc_did_set_langmap new file mode 100644 index 0000000000..f77145b9d1 --- /dev/null +++ b/test/old/testdir/crash/poc_did_set_langmap @@ -0,0 +1 @@ +se lmap=°xÿ7sil;drlmap=°xÿ7sil;drmo: pm31 3" \ No newline at end of file diff --git a/test/old/testdir/test_crash.vim b/test/old/testdir/test_crash.vim index eef1731454..1d4f435e4d 100644 --- a/test/old/testdir/test_crash.vim +++ b/test/old/testdir/test_crash.vim @@ -142,6 +142,13 @@ func Test_crash1_2() \ ' && echo "crash 3: [OK]" >> '.. result .. "\") call TermWait(buf, 150) + let file = 'crash/poc_did_set_langmap' + let cmn_args = "%s -u NONE -i NONE -n -X -m -n -e -s -S %s -c ':qa!'" + let args = printf(cmn_args, vim, file) + call term_sendkeys(buf, args .. + \ ' ; echo "crash 4: [OK]" >> '.. result .. "\") + call TermWait(buf, 150) + " clean up exe buf .. "bw!" @@ -151,6 +158,7 @@ func Test_crash1_2() \ 'crash 1: [OK]', \ 'crash 2: [OK]', \ 'crash 3: [OK]', + \ 'crash 4: [OK]', \ ] call assert_equal(expected, getline(1, '$')) From 9d7544ac4cd553c9b7c8b41926b7292c5ee85943 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 2 Dec 2023 10:17:43 +0800 Subject: [PATCH 4/4] vim-patch:9.0.2143: [security]: buffer-overflow in ex_substitute Problem: [security]: buffer-overflow in ex_substitute Solution: clear memory after allocating When allocating the new_start pointer in ex_substitute() the memory pointer points to some garbage that the following for loop in ex_cmds.c:4743 confuses and causes it to accessing the new_start pointer beyond it's size, leading to a buffer-overlow. So fix this by using alloc_clear() instead of alloc(), which will clear the memory by NUL and therefore cause the loop to terminate correctly. Reported by @henices, thanks! closes: vim/vim#13596 https://github.com/vim/vim/commit/abfa13ebe92d81aaf66669c428d767847b577453 Co-authored-by: Christian Brabandt --- src/nvim/ex_cmds.c | 5 ++++- test/old/testdir/crash/poc_ex_substitute | Bin 0 -> 135 bytes test/old/testdir/test_crash.vim | 13 ++++++++++++- 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 test/old/testdir/crash/poc_ex_substitute diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c index 2c51d64972..0711d82fe5 100644 --- a/src/nvim/ex_cmds.c +++ b/src/nvim/ex_cmds.c @@ -3144,7 +3144,7 @@ static char *sub_grow_buf(char **new_start, int *new_start_len, int needed_len) // substitution into (and some extra space to avoid // too many calls to xmalloc()/free()). *new_start_len = needed_len + 50; - *new_start = xmalloc((size_t)(*new_start_len)); + *new_start = xcalloc(1, (size_t)(*new_start_len)); **new_start = NUL; new_end = *new_start; } else { @@ -3154,8 +3154,11 @@ static char *sub_grow_buf(char **new_start, int *new_start_len, int needed_len) size_t len = strlen(*new_start); needed_len += (int)len; if (needed_len > *new_start_len) { + size_t prev_new_start_len = (size_t)(*new_start_len); *new_start_len = needed_len + 50; + size_t added_len = (size_t)(*new_start_len) - prev_new_start_len; *new_start = xrealloc(*new_start, (size_t)(*new_start_len)); + memset(*new_start + prev_new_start_len, 0, added_len); } new_end = *new_start + len; } diff --git a/test/old/testdir/crash/poc_ex_substitute b/test/old/testdir/crash/poc_ex_substitute new file mode 100644 index 0000000000000000000000000000000000000000..bcf1286512f21a787dcd048ac1eb7f3611db47e3 GIT binary patch literal 135 zcmXR;RY=WC&QHnAOSer)O)5=SuvAFp$|=!T;NmL&|DQpvm`h(%KSqJ8nE5}Ky=_rR zP>y|BrUI7%SBW1?NoFor{)89|;iAP{>OiUN)YO6kg?tUof@B4*vSKcS;^d> '.. result .. "\") call TermWait(buf, 150) + let file = 'crash/poc_ex_substitute' + let cmn_args = "%s -u NONE -i NONE -n -e -s -S %s -c ':qa!'" + let args = printf(cmn_args, vim, file) + " just make sure it runs, we don't care about the resulting echo + call term_sendkeys(buf, args .. "\") + " There is no output generated in Github CI for the asan clang build. + " so just skip generating the ouput. + " call term_sendkeys(buf, args .. + " \ ' && echo "crash 5: [OK]" >> '.. result .. "\") + call TermWait(buf, 150) + " clean up exe buf .. "bw!"