Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Divide-by-zero vulnerability in function scroll_cursor_bot #12528

Closed
fullwaywang opened this issue Jun 12, 2023 · 1 comment
Closed

Divide-by-zero vulnerability in function scroll_cursor_bot #12528

fullwaywang opened this issue Jun 12, 2023 · 1 comment
Labels

Comments

@fullwaywang
Copy link

fullwaywang commented Jun 12, 2023

  • I reported this vulnerability on huntr.dev but had not got a response for a week, so I think maybe disclosing it here is easier to keep in touch.

Description

Recently I have been reviewing CVEs, checking whether there are cognate/recurring bugs as known ones. While reviewing VIM, I found one quite similar to CVE-2023-0512.

The CVE-2023-0512 is an issue where VIM under ex mode falsely caculated the width of current window. In scroll_cursor_bot caculates the topline and bottomline likely, only in normal visual mode. In the caculation, width1 is assigned the width of current window minus widths of linenumber and foldcolumn, which can be negative. If foldcolumen is on, then width2 can be assigned 0, resulting in a Divide-by-zero fault in the following division.

Version of Vim

9.0.0908

Environment

CentOS 8 stream

Logs and stack traces

/data/project/exp/build/cloned/vim
➜ cat /tmp/vim_test.data
silent normal yy
silent normal 19p
wi0 19
vsplit
vertical resize 0
set foldcolumn=1
set number
set smoothscroll
silent normal G

/data/project/exp/build/cloned/vim
➜ src/vim -u NONE -i NONE -n -m -X -Z -S /tmp/vim_test.data -c :qa!
Vim: Caught deadly signal FPE
Vim: Finished.
[1]    3037351 floating point exception (core dumped)  src/vim -u NONE -i NONE -n -m -X -Z -S /tmp/vim_test.data -c
                                                                                                                                   %

GDB:

/data/project/exp/build/cloned/vim
➜ gdb src/vim
Reading symbols from src/vim...
(gdb) set args -u NONE -i NONE -n -m -X -Z -S /tmp/vim_test.data
(gdb) r
           i
d argument:
wfw=0  38:
E474: Program received signal SIGFPE, Arithmetic exception.
                                                           0x0000000000526911 in scroll_cursor_bot (min_scroll=1, set_topbot=0) at move.c:2593
2593			skip_lines += (curwin->w_skipcol - width1) / width2 + 1;
(gdb) list
2588		    int skip_lines = 0;
2589		    int width1 = curwin->w_width - curwin_col_off();
2590		    int width2 = width1 + curwin_col_off2();
2591		    // similar formula is used in curs_columns()
2592		    if (curwin->w_skipcol > width1)
2593			skip_lines += (curwin->w_skipcol - width1) / width2 + 1;
2594		    else if (curwin->w_skipcol > 0)
2595			skip_lines = 1;
2596
2597		    top_plines -= skip_lines;
(gdb) p width1
$1 = -8
(gdb) p width2
$2 = 0
(gdb) bt
#0  0x0000000000526911 in scroll_cursor_bot (min_scroll=1, set_topbot=0) at move.c:2593
#1  0x0000000000522389 in update_topline () at move.c:500
#2  0x000000000049c48d in update_topline_cursor () at ex_docmd.c:8612
#3  0x000000000049c90b in ex_normal (eap=0x7fffffffc140) at ex_docmd.c:8784
#4  0x0000000000491513 in do_one_cmd (cmdlinep=0x7fffffffc968, flags=7, cstack=0x7fffffffc330, fgetline=0x5befb1 <getsourceline>, cookie=0x7fffffffcab0)
    at ex_docmd.c:2580
#5  0x000000000048e8e5 in do_cmdline (cmdline=0x80a710 "wi0 19", fgetline=0x5befb1 <getsourceline>, cookie=0x7fffffffcab0, flags=7) at ex_docmd.c:993
#6  0x00000000005bdfad in do_source_ext (fname=0x7bc1a3 "/tmp/vim_test.data", check_other=0, is_vimrc=0, ret_sid=0x0, eap=0x0, clearvars=0)
    at scriptfile.c:1760
#7  0x00000000005be5d6 in do_source (fname=0x7bc1a3 "/tmp/vim_test.data", check_other=0, is_vimrc=0, ret_sid=0x0) at scriptfile.c:1906
#8  0x00000000005bd2e0 in cmd_source (fname=0x7bc1a3 "/tmp/vim_test.data", eap=0x7fffffffcd70) at scriptfile.c:1251
#9  0x00000000005bd32e in ex_source (eap=0x7fffffffcd70) at scriptfile.c:1277
#10 0x0000000000491513 in do_one_cmd (cmdlinep=0x7fffffffd598, flags=11, cstack=0x7fffffffcf60, fgetline=0x0, cookie=0x0) at ex_docmd.c:2580
#11 0x000000000048e8e5 in do_cmdline (cmdline=0x7bc100 "so /tmp/vim_test.data", fgetline=0x0, cookie=0x0, flags=11) at ex_docmd.c:993
#12 0x000000000048de8b in do_cmdline_cmd (cmd=0x7bc100 "so /tmp/vim_test.data") at ex_docmd.c:587
#13 0x00000000006d637c in exe_commands (parmp=0x7a1180 <params>) at main.c:3150
#14 0x00000000006d345c in vim_main2 () at main.c:782
#15 0x00000000006d2e2e in main (argc=12, argv=0x7fffffffd7d8) at main.c:433

Resolution

All occurences of caculated current window width (minus linenum and foldmark widths) should be checked.

For this bug, a patch is ready: HEAD...fullwaywang:fix-divide-by-zero

Discoverer

fullwaywang from Tencent

@fullwaywang
Copy link
Author

A simplified PoC and accompanied text file (20 blank lines) is given, see the logs above.

fullwaywang pushed a commit to fullwaywang/vim that referenced this issue Jun 15, 2023
zeertzjq added a commit to zeertzjq/vim that referenced this issue Jun 25, 2023
The test for vim#12528 doesn't fail without the fix because the original
steps to reproduce run Vim with `-u NONE -i NONE` which puts Vim in
Vi-compatible mode where 'cpoptions' contains the "n" flag, whereas
RunVimInTerminal() runs Vim with `--clean` which doesn't add the "n"
flag to 'cpoptions'.

Adding the "n" flag to 'cpoptions' is enough to make the test work
properly. The `:winsize` command is also unnecessary as the zero-width
window created by `:vsplit` and `:vertical resize 0` is enough.
zeertzjq added a commit to zeertzjq/vim that referenced this issue Jun 26, 2023
The test for vim#12528 doesn't fail without the fix because the original
steps to reproduce run Vim with `-u NONE -i NONE` which puts Vim in
Vi-compatible mode where 'cpoptions' contains the "n" flag, whereas
RunVimInTerminal() runs Vim with `--clean` which doesn't add the "n"
flag to 'cpoptions'.

Adding the "n" flag to 'cpoptions' is enough to make the test work
properly. The `:winsize` command is also unnecessary as the zero-width
window created by `:vsplit` and `:vertical resize 0` is enough.
brammool pushed a commit that referenced this issue Jun 26, 2023
Problem:    Regression test doesn't fail when fix is reverted.
Solution:   Add "n" to 'cpoptions' instead of using :winsize. (closes #12587,
            issue #12528)
zeertzjq added a commit to zeertzjq/neovim that referenced this issue Jun 27, 2023
… set

Problem:    Divide by zero when scrolling with 'smoothscroll' set.
Solution:   Avoid using a negative width. (closes vim/vim#12540, closes vim/vim#12528)

vim/vim@8154e64

Co-authored-by: fullwaywang <fullwaywang@tencent.com>
zeertzjq added a commit to zeertzjq/neovim that referenced this issue Jun 27, 2023
Problem:    Regression test doesn't fail when fix is reverted.
Solution:   Add "n" to 'cpoptions' instead of using :winsize. (closes vim/vim#12587,
            issue vim/vim#12528)

vim/vim@e429893

Co-authored-by: zeertzjq <zeertzjq@outlook.com>
zeertzjq added a commit to zeertzjq/neovim that referenced this issue Jun 27, 2023
… set

Problem:    Divide by zero when scrolling with 'smoothscroll' set.
Solution:   Avoid using a negative width. (closes vim/vim#12540, closes vim/vim#12528)

vim/vim@8154e64

Co-authored-by: fullwaywang <fullwaywang@tencent.com>
zeertzjq added a commit to zeertzjq/neovim that referenced this issue Jun 27, 2023
Problem:    Regression test doesn't fail when fix is reverted.
Solution:   Add "n" to 'cpoptions' instead of using :winsize. (closes vim/vim#12587,
            issue vim/vim#12528)

vim/vim@e429893
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant