# HG changeset patch # User Christian Brabandt # Date 1696145403 -7200 # Node ID aa7cd2253130890fb018cbf82dc3114f1ad7684a # Parent 68fc658ae847c1cab40872fb086851e072627068 patch 9.0.1968: cmdline completion should consider key option Commit: https://github.com/vim/vim/commit/6ee7b521fa7531ef356ececc8be7575c3800f872 Author: Yee Cheng Chin Date: Sun Oct 1 09:13:22 2023 +0200 patch 9.0.1968: cmdline completion should consider key option Problem: cmdline completion should consider key option Solution: Disable cmdline completion for key option, slightly refactor how P_NO_CMD_EXPAND is handled Harden crypto 'key' option: turn off cmdline completion, disable set-= "set-=" can be used maliciously with a crypto key, as it allows an attacker (who either has access to the computer or a plugin author) to guess a substring by observing the modified state. Simply turn off set+=/-=/^= for this option as there is no good reason for them to be used. Update docs to make that clear as well. Also, don't allow cmdline completion for 'key' as it just shows ***** which is not useful and confusing to the user what it means (if the user accidentally hits enter they will have replaced their key with "*****" instead). Move logic to better location, don't use above 32-bit for flags Move P_NO_CMD_EXPAND to use the unused 0x20 instead of going above 32-bits, as currently the flags parameter is only 32-bits on some systems. Left a comment to warn that future additions will need to change how the flags work either by making it 64-bit or split into two member vars. Also, move the logic for detecting P_NO_CMD_EXPAND earlier so it's not up to each handler to decide, and you won't see the temporary "..." that Vim shows while waiting for completion handler to complete. closes: #13224 Signed-off-by: Christian Brabandt Co-authored-by: Yee Cheng Chin diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt --- a/runtime/doc/options.txt +++ b/runtime/doc/options.txt @@ -4928,6 +4928,8 @@ A jump table for the options with a shor "echo &key". This is to avoid showing it to someone who shouldn't know. It also means you cannot see it yourself once you have set it, be careful not to make a typing error! + You also cannot use |:set-=|, |:set+=|, |:set^=| on this option to + prevent an attacker from guessing substrings in your key. You can use "&key" in an expression to detect whether encryption is enabled. When 'key' is set it returns "*****" (five stars). diff --git a/src/option.c b/src/option.c --- a/src/option.c +++ b/src/option.c @@ -1315,13 +1315,6 @@ ex_set(exarg_T *eap) * :set operator types */ typedef enum { - OP_NONE = 0, - OP_ADDING, // "opt+=arg" - OP_PREPENDING, // "opt^=arg" - OP_REMOVING, // "opt-=arg" -} set_op_T; - -typedef enum { PREFIX_NO = 0, // "no" prefix PREFIX_NONE, // no prefix PREFIX_INV, // "inv" prefix @@ -1935,7 +1928,7 @@ do_set_option_string( char_u **argp, int nextchar, set_op_T op_arg, - int flags, + long_u flags, int cp_val, char_u *varp_arg, char *errbuf, @@ -2037,7 +2030,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, value_checked); + opt_flags, op, value_checked); secure = secure_saved; } @@ -7376,6 +7369,15 @@ set_context_in_set_cmd( else expand_option_idx = opt_idx; + if (!is_term_option) + { + if (options[opt_idx].flags & P_NO_CMD_EXPAND) + { + xp->xp_context=EXPAND_UNSUCCESSFUL; + return; + } + } + xp->xp_pattern = p + 1; expand_option_start_col = (int)(p + 1 - xp->xp_line); diff --git a/src/option.h b/src/option.h --- a/src/option.h +++ b/src/option.h @@ -25,6 +25,7 @@ // the same. #define P_EXPAND 0x10 // environment expansion. NOTE: P_EXPAND can // never be used for local or hidden options! +#define P_NO_CMD_EXPAND 0x20 // don't perform cmdline completions #define P_NODEFAULT 0x40 // don't set to default value #define P_DEF_ALLOCED 0x80 // default value is in allocated memory, must // use vim_free() when assigning new value @@ -61,6 +62,10 @@ #define P_MLE 0x20000000L // under control of 'modelineexpr' #define P_FUNC 0x40000000L // accept a function reference or a lambda #define P_COLON 0x80000000L // values use colons to create sublists +// Warning: Currently we have used all 32 bits for option flags. On some 32-bit +// systems, the flags are stored as a 32-bit integer, and adding more +// flags will overflow it. Adding another flag will need to change how +// it's stored first. // Returned by get_option_value(). typedef enum { diff --git a/src/optiondefs.h b/src/optiondefs.h --- a/src/optiondefs.h +++ b/src/optiondefs.h @@ -1467,7 +1467,7 @@ static struct vimoption options[] = {"jumpoptions", "jop", P_STRING|P_VI_DEF|P_VIM|P_ONECOMMA|P_NODUP, (char_u *)&p_jop, PV_NONE, did_set_jumpoptions, expand_set_jumpoptions, {(char_u *)"", (char_u *)0L} SCTX_INIT}, - {"key", NULL, P_STRING|P_ALLOCED|P_VI_DEF|P_NO_MKRC, + {"key", NULL, P_STRING|P_ALLOCED|P_VI_DEF|P_NO_MKRC|P_NO_CMD_EXPAND, #ifdef FEAT_CRYPT (char_u *)&p_key, PV_KEY, did_set_cryptkey, NULL, {(char_u *)"", (char_u *)0L} diff --git a/src/optionstr.c b/src/optionstr.c --- a/src/optionstr.c +++ b/src/optionstr.c @@ -579,7 +579,7 @@ set_string_option( } #endif if ((errmsg = did_set_string_option(opt_idx, varp, oldval, value, errbuf, - opt_flags, &value_checked)) == NULL) + opt_flags, OP_NONE, &value_checked)) == NULL) did_set_option(opt_idx, opt_flags, TRUE, value_checked); #if defined(FEAT_EVAL) @@ -1578,6 +1578,10 @@ did_set_cryptkey(optset_T *args) // history. remove_key_from_history(); + if (args->os_op != OP_NONE) + // Don't allow set+=/-=/^= as they can allow for substring guessing + return e_invalid_argument; + if (STRCMP(curbuf->b_p_key, args->os_oldval.string) != 0) { // Need to update the swapfile. @@ -4209,6 +4213,7 @@ did_set_string_option( char_u *value, // new value of the option char *errbuf, // buffer for errors, or NULL 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 // need to set P_INSECURE { @@ -4247,6 +4252,7 @@ did_set_string_option( args.os_varp = (char_u *)varp; args.os_idx = opt_idx; args.os_flags = opt_flags; + args.os_op = op; args.os_oldval.string = oldval; args.os_newval.string = value; args.os_errbuf = errbuf; diff --git a/src/proto/optionstr.pro b/src/proto/optionstr.pro --- a/src/proto/optionstr.pro +++ b/src/proto/optionstr.pro @@ -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, int *value_checked); +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); 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 @@ -588,6 +588,13 @@ typedef enum { XP_PREFIX_INV, // "inv" prefix for bool option } xp_prefix_T; +typedef enum { + OP_NONE = 0, + OP_ADDING, // "opt+=arg" + OP_PREPENDING, // "opt^=arg" + OP_REMOVING, // "opt-=arg" +} set_op_T; + /* * used for completion on the command line */ @@ -4876,6 +4883,7 @@ typedef struct char_u *os_varp; int os_idx; int os_flags; + set_op_T os_op; // old value of the option (can be a string, number or a boolean) union diff --git a/src/testdir/test_crypt.vim b/src/testdir/test_crypt.vim --- a/src/testdir/test_crypt.vim +++ b/src/testdir/test_crypt.vim @@ -438,4 +438,27 @@ func Test_crypt_set_key_segfault() bwipe! endfunc +func Test_crypt_set_key_disallow_append_subtract() + new Xtest4 + + set key=foobar + call assert_true(&modified) + setl nomodified + + call assert_fails('set key-=foo', 'E474:') + call assert_fails('set key-=bar', 'E474:') + call assert_fails('set key-=foobar', 'E474:') + call assert_fails('set key-=test1', 'E474:') + + call assert_false(&modified) + call assert_equal('*****', &key) + + call assert_fails('set key+=test2', 'E474:') + call assert_fails('set key^=test3', 'E474:') + + call assert_false(&modified) + set key= + bwipe! +endfunc + " vim: shiftwidth=2 sts=2 expandtab diff --git a/src/testdir/test_history.vim b/src/testdir/test_history.vim --- a/src/testdir/test_history.vim +++ b/src/testdir/test_history.vim @@ -244,8 +244,13 @@ endfunc " Test for making sure the key value is not stored in history func Test_history_crypt_key() CheckFeature cryptv + call feedkeys(":set bs=2 key=abc ts=8\", 'xt') call assert_equal('set bs=2 key= ts=8', histget(':')) + + call assert_fails("call feedkeys(':set bs=2 key-=abc ts=8\', 'xt')") + call assert_equal('set bs=2 key-= ts=8', histget(':')) + set key& bs& ts& endfunc diff --git a/src/testdir/test_options.vim b/src/testdir/test_options.vim --- a/src/testdir/test_options.vim +++ b/src/testdir/test_options.vim @@ -365,13 +365,15 @@ func Test_set_completion() call feedkeys(":set spellsuggest=best,file:test_options.v\\\"\", 'xt') call assert_equal("\"set spellsuggest=best,file:test_options.vim", @:) - " Expand value for 'key' - set key=abcd - call feedkeys(":set key=\\\"\", 'xt') - call assert_equal('"set key=*****', @:) - call feedkeys(":set key-=\\\"\", 'xt') - call assert_equal('"set key-=*****', @:) - set key= + " Expanding value for 'key' is disallowed + if exists('+key') + set key=abcd + call feedkeys(":set key=\\\"\", 'xt') + call assert_equal('"set key=', @:) + call feedkeys(":set key-=\\\"\", 'xt') + call assert_equal('"set key-=', @:) + set key= + endif " Expand values for 'filetype' call feedkeys(":set filetype=sshdconfi\\\"\", 'xt') 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 */ /**/ + 1968, +/**/ 1967, /**/ 1966,