[vim/vim] Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c (PR #11923)
Pavel Mayorov
This issue was previously indicated in https://huntr.dev/bounties/ae933869-a1ec-402a-bbea-d51764c6618e. But a fix was proposed for only one of them. An additional check for the length of selected text should fix it.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/11923
Commit Summary
- 7357a95 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
File Changes
(1 file)
- M src/normal.c (7)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> This issue was previously indicated in
> https://huntr.dev/bounties/ae933869-a1ec-402a-bbea-d51764c6618e. But a
> fix was proposed for only one of them. An additional check for the
> length of selected text should fix it.
using a Vim script?
--
ASCII stupid question, get a stupid ANSI.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
I'll try to deal with testing (I'm not familiar with this subject yet), but the problem is detected by the sanitizer and in normal mode there are no signs of it ... and you answered "Unfortunately I have not been able to use it for a test case" for the first issue with function same_leader at textformat.c
.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- c187136 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- 79b31b5 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> I'll try to deal with testing (I'm not familiar with this subject
> yet), but the problem is detected by the sanitizer and in normal mode
> there are no signs of it ...
That's a start at least.
> and you answered "Unfortunately I have
> not been able to use it for a test case" for the first issue with
> function same_leader at textformat.c
two POC files mattered for patch 9.0.1225.
The poc file is used with:
./vim -u NONE -i NONE -n -m -X -Z -e -s -S ./POC2 -c :qa!
The tests run with other conditions, e.g. not in Ex mode (that is the -e
argument). Usually I can guess what matters and setup the context in
the test, but not always...
--
hundred-and-one symptoms of being an internet addict:
89. In addition to your e-mail address being on your business
cards you even have your own domain.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
Two problems in one issue is confusing. Now I don't know which of the
two POC files mattered for patch 9.0.1225.
This is POC1.... fix for same_leader at textformat.c
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
The tests run with other conditions, e.g. not in Ex mode (that is the -e
argument).
So I took the tests conditions and replace -S runtest.vim test.vim with -S ~/POC2, and got the same result with sanitizer:
VIMRUNTIME=../../runtime ../vim -f -u unix.vim --gui-dialog-file guidialog -U NONE --noplugin --not-a-term -S ~/_Work/vim/POC2 --cmd 'au SwapExists * let v:swapchoice = "e"' | LC_ALL=C LANG=C LANGUAGE=C awk '/Executing Test_/{match($0, "([0-9][0-9]:[0-9][0-9] *)?Executing Test_[^\\)]*\\)"); print substr($0, RSTART, RLENGTH) "\r"; fflush()}'
=================================================================
==124252==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000eb32 at pc 0x55f6fb405dde bp 0x7ffe186f4100 sp 0x7ffe186f40f0
READ of size 1 at 0x60200000eb32 thread T0
#0 0x55f6fb405ddd in utfc_ptr2len /home/pavel/_Work/vim/vim_build/src/mbyte.c:2145
#1 0x55f6fb500644 in get_visual_text /home/pavel/_Work/vim/vim_build/src/normal.c:3684
#2 0x55f6fb4f546e in nv_zg_zw /home/pavel/_Work/vim/vim_build/src/normal.c:2613
...
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@brammool I did some testing with POC2. Here is its content (for execution in ex-mode):
norm0R0
sp0
no0 R
sil0norm0000
norm<0x16><0xF7>
ba
norm8zW
I wrote the command norm<0x16><0xF7> in this form for convenience, but in reality, instead of <...>, the command uses the corresponding bytes: 0x16 0xF7.
After the first 5 commands, we have something like the following (most likely I couldn't correctly call norm<0x16><0xF7> by manually, but even so the final result is similar):
There are 2 windows: in the first 000 (active with the cursor on the third 0), in the second - 0. At the same time the visual mode is activated and the object VIsual is initialized as {lnum = 1, col = 2, coladd = 0}:
Hardware watchpoint 1: VIsual
Old value = {lnum = 1, col = 0, coladd = 0}
New value = {lnum = 1, col = 2, coladd = 0}
n_start_visual_mode (c=22) at normal.c:5572
5572 foldAdjustVisual();
(gdb) bt
#0 n_start_visual_mode (c=22) at normal.c:5572
#1 0x0000555556f1402f in nv_visual (cap=0x7fffffffbbc0) at normal.c:5509
#2 0x0000555556edee27 in normal_cmd (oap=0x7fffffffbce0, toplevel=1) at normal.c:938
#3 0x0000555556c05292 in exec_normal (was_typed=0, use_vpeekc=0, may_use_terminal_loop=0) at ex_docmd.c:8887
#4 0x0000555556c05051 in exec_normal_cmd (cmd=0x611000001444 <incomplete sequence \367>, remap=0, silent=0) at ex_docmd.c:8850
...
#17 0x00005555577ac409 in main (argc=15, argv=0x7fffffffde18) at main.c:433
(gdb)
After executing the command ba, the windows are swapped, and the cursor points to line 0:
But the state of the object VIsual doesn't change in any way. And then when norm8zW is executed, we get into the get_visual_text, and get the text under the cursor and its length:
Breakpoint 2, get_visual_text (cap=0x7fffffffbbc0, pp=0x7fffffffb980, lenp=0x7fffffffb970) at normal.c:3646
3646 if (VIsual_mode != 'V')
(gdb)
3647 unadjust_for_sel();
(gdb)
3648 if (VIsual.lnum != curwin->w_cursor.lnum)
(gdb)
3654 if (VIsual_mode == 'V')
(gdb)
3661 if (LT_POS(curwin->w_cursor, VIsual))
(gdb)
3663 *pp = ml_get_pos(&curwin->w_cursor);
(gdb)
3664 *lenp = VIsual.col - curwin->w_cursor.col + 1;
(gdb)
3671 if (**pp == NUL)
(gdb) p *pp
$2 = (char_u *) 0x6020000081b0 "0"
(gdb) p *lenp
$3 = 3
(gdb) p VIsual_active
$4 = 1
(gdb) p VIsual_mode
$5 = 22
(gdb)
Thus, the current text is obtained correctly, but the length of the text is wrong, because it is calculated from the previous state.
I guess that when the ba is executed, the state of the VIsual should also be updated, but I don't have enough knowledge about the internal workings of the project to do this correctly. I hope this information will be useful for @brammool and you can hint me or maybe fix it yourself.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> @brammool I did some testing with POC2. Here is its content (for
> execution in ex-mode):
The POC can be simplified and made clearer, I ended up with (xxd format):
00000000: 6e6f 726d 2052 300a 7370 6c69 7420 6f74 norm R0.split ot
00000010: 6865 720a 6e6f 726d 2052 3030 300a 6e6f her.norm R000.no
00000020: 726d 2016 c3b7 0a62 616c 6c0a 6e6f 726d rm ....ball.norm
00000030: 207a 57 zW
Readable form:
norm R0
split other
norm R000
norm <0x16><0xF7>
ball
norm zW
That 0xF7 is an invalid command, it should not matter, but removing it
makes the problem go away. I have not spent time on finding out why.
Anyway, the real problem is that ":ball" is executed with the Visual
area starting in column 2, and after the windows are rearraged that
position does not exist in the current buffer.
> I guess that when the `ba` is executed, the state of the `VIsual`
> should also be updated, but I don't have enough knowledge about the
> internal workings of the project to do this correctly. I hope this
> information will be useful for @brammool and you can hint me or maybe
> fix it yourself.
sense. The easiest would be that when using ":ball" then Visual mode is
cancelled. I don't see why it would be kept.
Adjusting the get_visual_text() function makes Vim work with the invalid
Visual position, but it most likely will fail with some other command.
Does this give enough info to fix the bug and write a test?
--
From "know your smileys":
=):-) Uncle Sam
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- 4953064 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
I've prepared new solution, but still can't implement this POC as a test... keep trying
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- 95456c9 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@brammool Finally I could create the test that triggers "heap-buffer-overflow" when AddressSanitizer is used (this corrupts the output in the terminal)
But this is still the first preparation, because it needs to add some kind of comparison of the current line and get the current state of the Visual mode (I don’t know yet if this allows by test API)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Dominique Pellé
@dpelle commented on this pull request.
In src/testdir/test_visual.vim:
> @@ -1523,4 +1523,15 @@ func Test_switch_buffer_ends_visual_mode() exe 'bwipe!' buf2 endfunc +func Test_heap_buffer_overflow() + set uc=0 + norm R0 + split other + norm R000 + exe "norm \<C-V>l" + ball + norm zW + bwipe!
We could set the updatecount back to its default value at the end of the test:
set updatecount&
I'd also prefer the full name 'updatecount' which is more readable than its abbreviation 'uc' at line 1527.
I tend to use the long names in script and the short names when using vim interactively.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Dominique Pellé
@dpelle commented on this pull request.
In src/testdir/test_visual.vim:
> @@ -1523,4 +1523,15 @@ func Test_switch_buffer_ends_visual_mode() exe 'bwipe!' buf2 endfunc +func Test_heap_buffer_overflow()
How about a comment which links to the bug?
Something like this?
" Check fix in Vim-9.0.???? for the heap-buffer-overflow bug reported at:
" https://huntr.dev/bounties/ae933869-a1ec-402a-bbea-d51764c6618e/
Otherwise it can be hard to know years later what was the goal of the test.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> @brammool Finally I could create the test that triggers
> "heap-buffer-overflow" when AddressSanitizer is used (this corrupts
> the output in the terminal)
>
> But this is still the first preparation, because it needs to add some
> kind of comparison of the current line and get the current state of
> the Visual mode (I don’t know yet if this allows by test API)
enough. However, it's a good idea to also check that the resulting
buffer contents is OK, so we know the test did what it was supposed to.
In this case there are two windows. Going to the first window, checking
the buffer contents, then doing the same in the second window should be
good.
And please do restore any changed options, when tests are added it can
be very confusing if tests change the context for other tests. Best
would be to run each test case in a new Vim instance, but that would be
quite slow. And sometimes running multiple tests actually uncovers a
problem.
--
From "know your smileys":
<|-( Chinese and doesn't like these kind of jokes
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #11923 (95456c9) into master (d6e4c75) will increase coverage by
21.57%.
The diff coverage is100.00%.
@@ Coverage Diff @@ ## master #11923 +/- ## =========================================== + Coverage 61.04% 82.62% +21.57% =========================================== Files 164 150 -14 Lines 190106 179588 -10518 Branches 43492 40401 -3091 =========================================== + Hits 116050 148382 +32332 + Misses 61914 18247 -43667 - Partials 12142 12959 +817
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.62% <100.00%> (?) |
|
| huge-gcc-unittests | ? |
|
| linux | 82.62% <100.00%> (+82.33%) |
⬆️ |
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/buffer.c | 85.55% <100.00%> (+17.30%) |
⬆️ |
| src/iscygpty.c | ||
| src/os_w32exe.c | ||
| src/gui_dwrite.cpp | ||
| src/xpm_w32.c | ||
| src/vimrun.c | ||
| src/os_w32dll.c | ||
| src/winclip.c | ||
| src/json_test.c | ||
| src/memfile_test.c | ||
| ... and 153 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz commented on this pull request.
In src/testdir/test_visual.vim:
> @@ -1523,4 +1523,15 @@ func Test_switch_buffer_ends_visual_mode() exe 'bwipe!' buf2 endfunc +func Test_heap_buffer_overflow() + set uc=0 + norm R0 + split other + norm R000 + exe "norm \<C-V>l" + ball + norm zW + bwipe!
Ok... I'll fix this...
I just found the "set uс=0" and it worked :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz commented on this pull request.
In src/testdir/test_visual.vim:
> @@ -1523,4 +1523,15 @@ func Test_switch_buffer_ends_visual_mode() exe 'bwipe!' buf2 endfunc +func Test_heap_buffer_overflow()
agree, will fix
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
If without the fix the test fails with ASAN, that should already be enough. However, it's a good idea to also check that the resulting buffer contents is OK, so we know the test did what it was supposed to. In this case there are two windows. Going to the first window, checking the buffer contents, then doing the same in the second window should be good. And please do restore any changed options, when tests are added it can be very confusing if tests change the context for other tests. Best would be to run each test case in a new Vim instance, but that would be quite slow. And sometimes running multiple tests actually uncovers a problem.
I tried the "new" command at the beginning of the test function, but unfortunately I couldn't get 'heap-buffer-overflow' in ASan with it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- 04d702f Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
Is there any way to get the state of 'VIsual.col' or 'VIsual_active' in the test?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> I tried the "new" command at the beginning of the test function, but
> unfortunately I couldn't get 'heap-buffer-overflow' in ASan with it.
get that with "enew".
For the "ball" command you don't want any other buffers getting in the
way. You can use "%bwipe!" to get rid of buffers. Not sure if this
should go before or after "enew".
--
hundred-and-one symptoms of being an internet addict:
all of you live in the same street.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> Is there any way to get the state of 'VIsual.col' or 'VIsual_active'
> in the test?
works once Visual mode has ended.
When Visual mode is active, you can use getpos("v").
The mode() function returns the character representing Visual mode, if
it is active.
If you want to have the text check if the correct text is selected, it
might be easier to use a screendump test.
--
hundred-and-one symptoms of being an internet addict:
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@speculzzz pushed 1 commit.
- a22f6f6 Fix heap-based buffer overflow in function utfc_ptr2len at mbyte.c
You are receiving this because you are subscribed to this thread.
Pavel Mayorov
@brammool thanks, your hints were very helpful :).
Finally, if we compile the vim without fix and with ASan, then the test will trigger "heap-buffer-overflow", if without ASan - asserts will work.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.