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 <cb@256bit.org>

diff --git a/src/map.c b/src/map.c
index 5988445bd1588bb40854a1597d8cbb23628d794e..98785e722ce535066136951bcb07aceb964b9d40 100644 (file)
--- 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
index d5d20d7674aa60c42fa427b4595f3f6bdbe0aada..572788559cdfd0f55c9af650a0d5feef97d061a9 100644 (file)
--- 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
index 2d1ca2cdeaf66d526494be045e3d9ce9044d1397..646056bf111b440df534650c0fcfe51576693e92 100644 (file)
--- 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
index b7cdcc4511c7cbe95d2fb7fb434f12e5f1fa642d..84c77cb0a0e2592fb07a52261c461318137d25e8 100644 (file)
--- 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 **values, int list)
  * 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
index 22601ba996b8f3b60a3732389029423f645e8341..4ce93212dd948f6cd5ec9381976d354f93516a1a 100644 (file)
--- 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
index 4e081b860ef459bef9fd52db0442d4adb639110f..6d9dcbb2ab17d6212d560be60b172d9a5d8e7668 100644 (file)
--- 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 (file)
index 0000000..f77145b
--- /dev/null
+++ b/src/testdir/crash/poc_did_set_langmap
@@ -0,0 +1 @@
+se lmap=°xÿ7sil;drlmap=°xÿ\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\10\107sil;drmo: pm31\v3"
\ No newline at end of file

diff --git a/src/testdir/test_crash.vim b/src/testdir/test_crash.vim
index eef1731454d9ceb8707e72d50d29672d9968cc74..1d4f435e4d8bd362893d80fe3d9785d2b4e35988 100644 (file)
--- 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 .. "\<cr>")
   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 .. "\<cr>")
+  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
index ecb5c22304941a937d2e24e9abfd90417ee0defc..28325a7fa4f7555688119ce105324ef99621e810 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -704,6 +704,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    2142,
 /**/
     2141,
 /**/