# HG changeset patch # User Christian Brabandt # Date 1702217764 -3600 # Node ID 6e4c686b6b5b77d83988d514df8bf1bb332a6150 # Parent 3b8089d550eb47e688f46d78f162e75d7f6f6ecc patch 9.0.2142: [security]: stack-buffer-overflow in option callback functions Commit: https://github.com/vim/vim/commit/b39b240c386a5a29241415541f1c99e2e6b8ce47 Author: Christian Brabandt Date: Wed Nov 29 11:34:05 2023 +0100 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! Signed-off-by: Christian Brabandt diff --git a/src/map.c b/src/map.c --- a/src/map.c +++ b/src/map.c @@ -3114,7 +3114,7 @@ did_set_langmap(optset_T *args UNUSED) { if (p[0] != ',') { - sprintf(args->os_errbuf, + snprintf(args->os_errbuf, args->os_errbuflen, _(e_langmap_extra_characters_after_semicolon_str), p); return args->os_errbuf; diff --git a/src/option.c b/src/option.c --- a/src/option.c +++ b/src/option.c @@ -1932,6 +1932,7 @@ do_set_option_string( int cp_val, char_u *varp_arg, char *errbuf, + int errbuflen, int *value_checked, char **errmsg) { @@ -2030,7 +2031,7 @@ do_set_option_string( // be triggered that can cause havoc. *errmsg = did_set_string_option( opt_idx, (char_u **)varp, oldval, newval, errbuf, - opt_flags, op, value_checked); + errbuflen, opt_flags, op, value_checked); secure = secure_saved; } @@ -2287,7 +2288,7 @@ do_set_option_value( { // string option if (do_set_option_string(opt_idx, opt_flags, &arg, nextchar, op, - flags, cp_val, varp, errbuf, + flags, cp_val, varp, errbuf, errbuflen, &value_checked, &errmsg) == FAIL) { if (errmsg != NULL) @@ -2579,12 +2580,12 @@ do_set( { int stopopteval = FALSE; char *errmsg = NULL; - char errbuf[80]; + char errbuf[ERR_BUFLEN]; char_u *startarg = arg; errmsg = do_set_option(opt_flags, &arg, arg_start, &startarg, &did_show, &stopopteval, errbuf, - sizeof(errbuf)); + ERR_BUFLEN); if (stopopteval) break; @@ -5347,7 +5348,8 @@ set_option_value( int opt_idx; char_u *varp; long_u flags; - static char errbuf[80]; + static char errbuf[ERR_BUFLEN]; + int errbuflen = ERR_BUFLEN; opt_idx = findoption(name); if (opt_idx < 0) @@ -5390,7 +5392,7 @@ set_option_value( } #endif if (flags & P_STRING) - return set_string_option(opt_idx, string, opt_flags, errbuf); + return set_string_option(opt_idx, string, opt_flags, errbuf, errbuflen); varp = get_varp_scope(&(options[opt_idx]), opt_flags); if (varp != NULL) // hidden option is not changed diff --git a/src/option.h b/src/option.h --- a/src/option.h +++ b/src/option.h @@ -1321,4 +1321,6 @@ enum // Value for b_p_ul indicating the global value must be used. #define NO_LOCAL_UNDOLEVEL (-123456) +#define ERR_BUFLEN 80 + #endif // _OPTION_H_ diff --git a/src/optionstr.c b/src/optionstr.c --- a/src/optionstr.c +++ b/src/optionstr.c @@ -229,11 +229,12 @@ trigger_optionset_string( #endif static char * -illegal_char(char *errbuf, int c) +illegal_char(char *errbuf, int errbuflen, int c) { if (errbuf == NULL) return ""; - sprintf((char *)errbuf, _(e_illegal_character_str), (char *)transchar(c)); + snprintf((char *)errbuf, errbuflen, _(e_illegal_character_str), + (char *)transchar(c)); return errbuf; } @@ -525,7 +526,8 @@ set_string_option( int opt_idx, char_u *value, int opt_flags, // OPT_LOCAL and/or OPT_GLOBAL - char *errbuf) + char *errbuf, + int errbuflen) { char_u *s; char_u **varp; @@ -579,7 +581,7 @@ set_string_option( } #endif if ((errmsg = did_set_string_option(opt_idx, varp, oldval, value, errbuf, - opt_flags, OP_NONE, &value_checked)) == NULL) + errbuflen, opt_flags, OP_NONE, &value_checked)) == NULL) did_set_option(opt_idx, opt_flags, TRUE, value_checked); #if defined(FEAT_EVAL) @@ -615,7 +617,8 @@ valid_filetype(char_u *val) check_stl_option(char_u *s) { int groupdepth = 0; - static char errbuf[80]; + static char errbuf[ERR_BUFLEN]; + int errbuflen = ERR_BUFLEN; while (*s) { @@ -656,7 +659,7 @@ check_stl_option(char_u *s) } if (vim_strchr(STL_ALL, *s) == NULL) { - return illegal_char(errbuf, *s); + return illegal_char(errbuf, errbuflen, *s); } if (*s == '{') { @@ -664,7 +667,7 @@ check_stl_option(char_u *s) if (reevaluate && *++s == '}') // "}" is not allowed immediately after "%{%" - return illegal_char(errbuf, '}'); + return illegal_char(errbuf, errbuflen, '}'); while ((*s != '}' || (reevaluate && s[-1] != '%')) && *s) s++; if (*s != '}') @@ -719,13 +722,17 @@ did_set_opt_strings(char_u *val, char ** * An option which is a list of flags is set. Valid values are in 'flags'. */ static char * -did_set_option_listflag(char_u *val, char_u *flags, char *errbuf) +did_set_option_listflag( + char_u *val, + char_u *flags, + char *errbuf, + int errbuflen) { char_u *s; for (s = val; *s; ++s) if (vim_strchr(flags, *s) == NULL) - return illegal_char(errbuf, *s); + return illegal_char(errbuf, errbuflen, *s); return NULL; } @@ -1461,7 +1468,7 @@ did_set_comments(optset_T *args) if (vim_strchr((char_u *)COM_ALL, *s) == NULL && !VIM_ISDIGIT(*s) && *s != '-') { - errmsg = illegal_char(args->os_errbuf, *s); + errmsg = illegal_char(args->os_errbuf, args->os_errbuflen, *s); break; } ++s; @@ -1517,7 +1524,7 @@ did_set_complete(optset_T *args) if (!*s) break; if (vim_strchr((char_u *)".wbuksid]tU", *s) == NULL) - return illegal_char(args->os_errbuf, *s); + return illegal_char(args->os_errbuf, args->os_errbuflen, *s); if (*++s != NUL && *s != ',' && *s != ' ') { if (s[-1] == 'k' || s[-1] == 's') @@ -1534,7 +1541,7 @@ did_set_complete(optset_T *args) { if (args->os_errbuf != NULL) { - sprintf((char *)args->os_errbuf, + snprintf((char *)args->os_errbuf, args->os_errbuflen, _(e_illegal_character_after_chr), *--s); return args->os_errbuf; } @@ -1634,7 +1641,8 @@ did_set_concealcursor(optset_T *args) { char_u **varp = (char_u **)args->os_varp; - return did_set_option_listflag(*varp, (char_u *)COCU_ALL, args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)COCU_ALL, args->os_errbuf, + args->os_errbuflen); } int @@ -1652,7 +1660,8 @@ did_set_cpoptions(optset_T *args) { char_u **varp = (char_u **)args->os_varp; - return did_set_option_listflag(*varp, (char_u *)CPO_ALL, args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)CPO_ALL, args->os_errbuf, + args->os_errbuflen); } int @@ -2281,7 +2290,8 @@ did_set_formatoptions(optset_T *args) { char_u **varp = (char_u **)args->os_varp; - return did_set_option_listflag(*varp, (char_u *)FO_ALL, args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)FO_ALL, args->os_errbuf, + args->os_errbuflen); } int @@ -2422,7 +2432,8 @@ did_set_guioptions(optset_T *args) char_u **varp = (char_u **)args->os_varp; char *errmsg; - errmsg = did_set_option_listflag(*varp, (char_u *)GO_ALL, args->os_errbuf); + errmsg = did_set_option_listflag(*varp, (char_u *)GO_ALL, args->os_errbuf, + args->os_errbuflen); if (errmsg != NULL) return errmsg; @@ -2926,8 +2937,8 @@ did_set_mouse(optset_T *args) { char_u **varp = (char_u **)args->os_varp; - return did_set_option_listflag(*varp, (char_u *)MOUSE_ALL, - args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)MOUSE_ALL, args->os_errbuf, + args->os_errbuflen); } int @@ -3364,7 +3375,8 @@ did_set_shortmess(optset_T *args) { char_u **varp = (char_u **)args->os_varp; - return did_set_option_listflag(*varp, (char_u *)SHM_ALL, args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)SHM_ALL, args->os_errbuf, + args->os_errbuflen); } int @@ -4030,7 +4042,7 @@ did_set_viminfo(optset_T *args) // Check it's a valid character if (vim_strchr((char_u *)"!\"%'/:<@cfhnrs", *s) == NULL) { - errmsg = illegal_char(args->os_errbuf, *s); + errmsg = illegal_char(args->os_errbuf, args->os_errbuflen, *s); break; } if (*s == 'n') // name is always last one @@ -4057,7 +4069,7 @@ did_set_viminfo(optset_T *args) { if (args->os_errbuf != NULL) { - sprintf(args->os_errbuf, + snprintf(args->os_errbuf, args->os_errbuflen, _(e_missing_number_after_angle_str_angle), transchar_byte(*(s - 1))); errmsg = args->os_errbuf; @@ -4140,7 +4152,8 @@ did_set_whichwrap(optset_T *args) // Add ',' to the list flags because 'whichwrap' is a flag // list that is comma-separated. - return did_set_option_listflag(*varp, (char_u *)(WW_ALL ","), args->os_errbuf); + return did_set_option_listflag(*varp, (char_u *)(WW_ALL ","), + args->os_errbuf, args->os_errbuflen); } int @@ -4341,6 +4354,7 @@ did_set_string_option( char_u *oldval, // previous value of the option char_u *value, // new value of the option char *errbuf, // buffer for errors, or NULL + int errbuflen, // length of error buffer int opt_flags, // OPT_LOCAL and/or OPT_GLOBAL set_op_T op, // OP_ADDING/OP_PREPENDING/OP_REMOVING int *value_checked) // value was checked to be safe, no @@ -4385,6 +4399,7 @@ did_set_string_option( args.os_oldval.string = oldval; args.os_newval.string = value; args.os_errbuf = errbuf; + args.os_errbuflen = errbuflen; // Invoke the option specific callback function to validate and apply // the new option value. errmsg = did_set_cb(&args); diff --git a/src/proto/optionstr.pro b/src/proto/optionstr.pro --- a/src/proto/optionstr.pro +++ b/src/proto/optionstr.pro @@ -8,7 +8,7 @@ void check_string_option(char_u **pp); void set_string_option_direct(char_u *name, int opt_idx, char_u *val, int opt_flags, int set_sid); void set_string_option_direct_in_win(win_T *wp, char_u *name, int opt_idx, char_u *val, int opt_flags, int set_sid); void set_string_option_direct_in_buf(buf_T *buf, char_u *name, int opt_idx, char_u *val, int opt_flags, int set_sid); -char *set_string_option(int opt_idx, char_u *value, int opt_flags, char *errbuf); +char *set_string_option(int opt_idx, char_u *value, int opt_flags, char *errbuf, int errbuflen); char *did_set_ambiwidth(optset_T *args); char *did_set_background(optset_T *args); char *did_set_backspace(optset_T *args); @@ -121,7 +121,7 @@ char *did_set_wildmode(optset_T *args); char *did_set_wildoptions(optset_T *args); char *did_set_winaltkeys(optset_T *args); char *did_set_wincolor(optset_T *args); -char *did_set_string_option(int opt_idx, char_u **varp, char_u *oldval, char_u *value, char *errbuf, int opt_flags, set_op_T op, int *value_checked); +char *did_set_string_option(int opt_idx, char_u **varp, char_u *oldval, char_u *value, char *errbuf, int errbuflen, int opt_flags, set_op_T op, int *value_checked); int expand_set_ambiwidth(optexpand_T *args, int *numMatches, char_u ***matches); int expand_set_background(optexpand_T *args, int *numMatches, char_u ***matches); int expand_set_backspace(optexpand_T *args, int *numMatches, char_u ***matches); diff --git a/src/structs.h b/src/structs.h --- a/src/structs.h +++ b/src/structs.h @@ -4968,6 +4968,8 @@ 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 + int os_errbuflen; } optset_T; /* diff --git a/src/testdir/crash/poc_did_set_langmap b/src/testdir/crash/poc_did_set_langmap new file mode 100644 --- /dev/null +++ b/src/testdir/crash/poc_did_set_langmap @@ -0,0 +1,1 @@ +se lmap=°xÿ7sil;drlmap=°xÿ7sil;drmo: pm31 3" \ No newline at end of file diff --git a/src/testdir/test_crash.vim b/src/testdir/test_crash.vim --- a/src/testdir/test_crash.vim +++ b/src/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, '$')) diff --git a/src/version.c b/src/version.c --- a/src/version.c +++ b/src/version.c @@ -705,6 +705,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ /**/ + 2142, +/**/ 2141, /**/ 2140,