288 lines
8.0 KiB
Diff
288 lines
8.0 KiB
Diff
From 26c11c56888d01e298cd8044caf860f3c26f57bb Mon Sep 17 00:00:00 2001
|
|
From: Christian Brabandt <cb@256bit.org>
|
|
Date: Wed, 22 Nov 2023 21:26:41 +0100
|
|
Subject: [PATCH] patch 9.0.2121: [security]: use-after-free in ex_substitute
|
|
|
|
Problem: [security]: use-after-free in ex_substitute
|
|
Solution: always allocate memory
|
|
|
|
closes: #13552
|
|
|
|
A recursive :substitute command could cause a heap-use-after free in Vim
|
|
(CVE-2023-48706).
|
|
|
|
The whole reproducible test is a bit tricky, I can only reproduce this
|
|
reliably when no previous substitution command has been used yet
|
|
(which is the reason, the test needs to run as first one in the
|
|
test_substitute.vim file) and as a combination of the `:~` command
|
|
together with a :s command that contains the special substitution atom `~\=`
|
|
which will make use of a sub-replace special atom and calls a vim script
|
|
function.
|
|
|
|
There was a comment in the existing :s code, that already makes the
|
|
`sub` variable allocate memory so that a recursive :s call won't be able
|
|
to cause any issues here, so this was known as a potential problem
|
|
already. But for the current test-case that one does not work, because
|
|
the substitution does not start with `\=` but with `~\=` (and since
|
|
there does not yet exist a previous substitution atom, Vim will simply
|
|
increment the `sub` pointer (which then was not allocated dynamically)
|
|
and later one happily use a sub-replace special expression (which could
|
|
then free the `sub` var).
|
|
|
|
The following commit fixes this, by making the sub var always using
|
|
allocated memory, which also means we need to free the pointer whenever
|
|
we leave the function. Since sub is now always an allocated variable,
|
|
we also do no longer need the sub_copy variable anymore, since this one
|
|
was used to indicated when sub pointed to allocated memory (and had
|
|
therefore to be freed on exit) and when not.
|
|
|
|
Github Security Advisory:
|
|
https://github.com/vim/vim/security/advisories/GHSA-c8qm-x72m-q53q
|
|
|
|
Signed-off-by: Christian Brabandt <cb@256bit.org>
|
|
---
|
|
src/ex_cmds.c | 50 ++++++++++++++++++++++++---------
|
|
src/testdir/test_substitute.vim | 48 +++++++++++++++++++++++++++++--
|
|
2 files changed, 83 insertions(+), 15 deletions(-)
|
|
|
|
diff --git a/src/ex_cmds.c b/src/ex_cmds.c
|
|
index c5f912e7ee57f..a08682b071fb5 100644
|
|
--- a/src/ex_cmds.c
|
|
+++ b/src/ex_cmds.c
|
|
@@ -3686,7 +3686,6 @@ ex_substitute(exarg_T *eap)
|
|
int save_do_all; // remember user specified 'g' flag
|
|
int save_do_ask; // remember user specified 'c' flag
|
|
char_u *pat = NULL, *sub = NULL; // init for GCC
|
|
- char_u *sub_copy = NULL;
|
|
int delimiter;
|
|
int sublen;
|
|
int got_quit = FALSE;
|
|
@@ -3694,6 +3693,7 @@ ex_substitute(exarg_T *eap)
|
|
int temp;
|
|
int which_pat;
|
|
char_u *cmd;
|
|
+ char_u *p;
|
|
int save_State;
|
|
linenr_T first_line = 0; // first changed line
|
|
linenr_T last_line= 0; // below last changed line AFTER the
|
|
@@ -3774,8 +3774,12 @@ ex_substitute(exarg_T *eap)
|
|
* Small incompatibility: vi sees '\n' as end of the command, but in
|
|
* Vim we want to use '\n' to find/substitute a NUL.
|
|
*/
|
|
- sub = cmd; // remember the start of the substitution
|
|
+ p = cmd; // remember the start of the substitution
|
|
cmd = skip_substitute(cmd, delimiter);
|
|
+ sub = vim_strsave(p);
|
|
+ if (sub == NULL)
|
|
+ // out of memory
|
|
+ return;
|
|
|
|
if (!eap->skip)
|
|
{
|
|
@@ -3786,14 +3790,22 @@ ex_substitute(exarg_T *eap)
|
|
if (old_sub == NULL) // there is no previous command
|
|
{
|
|
emsg(_(e_no_previous_substitute_regular_expression));
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
- sub = old_sub;
|
|
+ vim_free(sub);
|
|
+ sub = vim_strsave(old_sub);
|
|
+ if (sub == NULL)
|
|
+ // out of memory
|
|
+ return;
|
|
}
|
|
else
|
|
{
|
|
vim_free(old_sub);
|
|
old_sub = vim_strsave(sub);
|
|
+ if (old_sub == NULL)
|
|
+ // out of memory
|
|
+ return;
|
|
}
|
|
}
|
|
}
|
|
@@ -3805,7 +3817,7 @@ ex_substitute(exarg_T *eap)
|
|
return;
|
|
}
|
|
pat = NULL; // search_regcomp() will use previous pattern
|
|
- sub = old_sub;
|
|
+ sub = vim_strsave(old_sub);
|
|
|
|
// Vi compatibility quirk: repeating with ":s" keeps the cursor in the
|
|
// last column after using "$".
|
|
@@ -3824,7 +3836,10 @@ ex_substitute(exarg_T *eap)
|
|
linenr_T joined_lines_count;
|
|
|
|
if (eap->skip)
|
|
+ {
|
|
+ vim_free(sub);
|
|
return;
|
|
+ }
|
|
curwin->w_cursor.lnum = eap->line1;
|
|
if (*cmd == 'l')
|
|
eap->flags = EXFLAG_LIST;
|
|
@@ -3851,6 +3866,7 @@ ex_substitute(exarg_T *eap)
|
|
save_re_pat(RE_SUBST, pat, magic_isset());
|
|
// put pattern in history
|
|
add_to_history(HIST_SEARCH, pat, TRUE, NUL);
|
|
+ vim_free(sub);
|
|
|
|
return;
|
|
}
|
|
@@ -3938,6 +3954,7 @@ ex_substitute(exarg_T *eap)
|
|
if (i <= 0 && !eap->skip && subflags.do_error)
|
|
{
|
|
emsg(_(e_positive_count_required));
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
else if (i >= INT_MAX)
|
|
@@ -3945,6 +3962,7 @@ ex_substitute(exarg_T *eap)
|
|
char buf[20];
|
|
vim_snprintf(buf, sizeof(buf), "%ld", i);
|
|
semsg(_(e_val_too_large), buf);
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
eap->line1 = eap->line2;
|
|
@@ -3963,17 +3981,22 @@ ex_substitute(exarg_T *eap)
|
|
if (eap->nextcmd == NULL)
|
|
{
|
|
semsg(_(e_trailing_characters_str), cmd);
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
}
|
|
|
|
if (eap->skip) // not executing commands, only parsing
|
|
+ {
|
|
+ vim_free(sub);
|
|
return;
|
|
+ }
|
|
|
|
if (!subflags.do_count && !curbuf->b_p_ma)
|
|
{
|
|
// Substitution is not allowed in non-'modifiable' buffer
|
|
emsg(_(e_cannot_make_changes_modifiable_is_off));
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
|
|
@@ -3981,6 +4004,7 @@ ex_substitute(exarg_T *eap)
|
|
{
|
|
if (subflags.do_error)
|
|
emsg(_(e_invalid_command));
|
|
+ vim_free(sub);
|
|
return;
|
|
}
|
|
|
|
@@ -4001,20 +4025,20 @@ ex_substitute(exarg_T *eap)
|
|
*/
|
|
if (sub[0] == '\\' && sub[1] == '=')
|
|
{
|
|
- sub = vim_strsave(sub);
|
|
- if (sub == NULL)
|
|
+ p = vim_strsave(sub);
|
|
+ vim_free(sub);
|
|
+ if (p == NULL)
|
|
return;
|
|
- sub_copy = sub;
|
|
+ sub = p;
|
|
}
|
|
else
|
|
{
|
|
- char_u *newsub = regtilde(sub, magic_isset());
|
|
+ p = regtilde(sub, magic_isset());
|
|
|
|
- if (newsub != sub)
|
|
+ if (p != sub)
|
|
{
|
|
- // newsub was allocated, free it later.
|
|
- sub_copy = newsub;
|
|
- sub = newsub;
|
|
+ vim_free(sub);
|
|
+ sub = p;
|
|
}
|
|
}
|
|
|
|
@@ -4848,7 +4872,7 @@ ex_substitute(exarg_T *eap)
|
|
#endif
|
|
|
|
vim_regfree(regmatch.regprog);
|
|
- vim_free(sub_copy);
|
|
+ vim_free(sub);
|
|
|
|
// Restore the flag values, they can be used for ":&&".
|
|
subflags.do_all = save_do_all;
|
|
diff --git a/src/testdir/test_substitute.vim b/src/testdir/test_substitute.vim
|
|
index 3ed159799f5cc..7c2bbb4767705 100644
|
|
--- a/src/testdir/test_substitute.vim
|
|
+++ b/src/testdir/test_substitute.vim
|
|
@@ -3,6 +3,32 @@ source shared.vim
|
|
source shared.vim
|
|
source check.vim
|
|
|
|
+" NOTE: This needs to be the first test to be
|
|
+" run in the file, since it depends on
|
|
+" that the previous substitution atom
|
|
+" was not yet set.
|
|
+"
|
|
+" recursive call of :s and sub-replace special
|
|
+" (did cause heap-use-after free in < v9.0.2121)
|
|
+func Test_aaaa_substitute_expr_recursive_special()
|
|
+ func R()
|
|
+ " FIXME: leaving out the 'n' flag leaks memory, why?
|
|
+ %s/./\='.'/gn
|
|
+ endfunc
|
|
+ new Xfoobar_UAF
|
|
+ put ='abcdef'
|
|
+ let bufnr = bufnr('%')
|
|
+ try
|
|
+ silent! :s/./~\=R()/0
|
|
+ "call assert_fails(':s/./~\=R()/0', 'E939:')
|
|
+ let @/='.'
|
|
+ ~g
|
|
+ catch /^Vim\%((\a\+)\)\=:E565:/
|
|
+ endtry
|
|
+ delfunc R
|
|
+ exe bufnr .. "bw!"
|
|
+endfunc
|
|
+
|
|
func Test_multiline_subst()
|
|
enew!
|
|
call append(0, ["1 aa",
|
|
@@ -146,7 +172,6 @@ func Test_substitute_repeat()
|
|
call feedkeys("Qsc\<CR>y", 'tx')
|
|
bwipe!
|
|
endfunc
|
|
-
|
|
" Test %s/\n// which is implemented as a special case to use a
|
|
" more efficient join rather than doing a regular substitution.
|
|
func Test_substitute_join()
|
|
@@ -1419,4 +1444,23 @@ func Test_substitute_tilde_too_long()
|
|
delfunc SubExpr
|
|
endfunc
|
|
|
|
+" recursive call of :s using test-replace special
|
|
+func Test_substitute_expr_recursive()
|
|
+ func Q()
|
|
+ %s/./\='foobar'/gn
|
|
+ return "foobar"
|
|
+ endfunc
|
|
+ func R()
|
|
+ %s/./\=Q()/g
|
|
+ endfunc
|
|
+ new Xfoobar_UAF
|
|
+ let bufnr = bufnr('%')
|
|
+ put ='abcdef'
|
|
+ silent! s/./\=R()/g
|
|
+ call assert_fails(':%s/./\=R()/g', 'E565:')
|
|
+ delfunc R
|
|
+ delfunc Q
|
|
+ exe bufnr .. "bw!"
|
|
+endfunc
|
|
+
|
|
" vim: shiftwidth=2 sts=2 expandtab
|