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 <cb@256bit.org>
Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>

diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt
index 090399af03e897ab983329cb7881f366c047d614..a24dd0490de30fd9bc41727d46a427d7c783ab73 100644 (file)
--- a/runtime/doc/options.txt
+++ b/runtime/doc/options.txt
@@ -4928,6 +4928,8 @@ A jump table for the options with a short description can be found at |Q_op|.
        "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
index d2096a5f0ddc55a494bcc670d1e4e1b3cdb02807..c1d80fa81545dcd6f88c14908c861aca9ae94587 100644 (file)
--- a/src/option.c
+++ b/src/option.c
@@ -1314,13 +1314,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
@@ -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
index ced4f3e4b75223f5faa1a7f0684d4a192d41ef15..396c568d49852e58b4a228e01a87c4f40de8d36d 100644 (file)
--- 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
index 08de5bf406e8c28965cc6f4a3adb3d7902bf31fe..f850d19b1cf1af1536ac345a78b78b7616e81a22 100644 (file)
--- 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
index 88532ee5996b01fe5edb32653ee81ab928b426ef..5a3dd6e758dfd7d151233481f3865a58bb0b49ce 100644 (file)
--- 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
index b4d9dfc95dc15001e23c248dd6121640fec1842c..48cccea8ccd9420168ddebde6be0a410ed055204 100644 (file)
--- 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
index d05ae872d5ba5eb0adbdd766dad19bf44ffe7242..1508f4791427f4650d10f78a594774e8515834d2 100644 (file)
--- 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
index 1782d83f327b761d7eee907916ebaafa576e1f35..85ed5740a10fc15977bf6876030a673c75ce7e46 100644 (file)
--- 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
index f1c31dee0415956442a5617a67f65e4c85a48b82..bb6d67172585e67c135ad92ff2e3f8561ce60ac5 100644 (file)
--- 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\<CR>", 'xt')
   call assert_equal('set bs=2 key= ts=8', histget(':'))
+
+  call assert_fails("call feedkeys(':set bs=2 key-=abc ts=8\<CR>', '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
index 32d4910ccb423d315c9d024d5baf33ee19549a41..8cd50694c9818ee0d790004cc39534b2f0fefdce 100644 (file)
--- 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\<Tab>\<C-B>\"\<CR>", 'xt')
   call assert_equal("\"set spellsuggest=best,file:test_options.vim", @:)
 
-  " Expand value for 'key'
-  set key=abcd
-  call feedkeys(":set key=\<Tab>\<C-B>\"\<CR>", 'xt')
-  call assert_equal('"set key=*****', @:)
-  call feedkeys(":set key-=\<Tab>\<C-B>\"\<CR>", 'xt')
-  call assert_equal('"set key-=*****', @:)
-  set key=
+  " Expanding value for 'key' is disallowed
+  if exists('+key')
+    set key=abcd
+    call feedkeys(":set key=\<Tab>\<C-B>\"\<CR>", 'xt')
+    call assert_equal('"set key=', @:)
+    call feedkeys(":set key-=\<Tab>\<C-B>\"\<CR>", 'xt')
+    call assert_equal('"set key-=', @:)
+    set key=
+  endif
 
   " Expand values for 'filetype'
   call feedkeys(":set filetype=sshdconfi\<Tab>\<C-B>\"\<CR>", 'xt')

diff --git a/src/version.c b/src/version.c
index a46db2a853879991f53b8c5e12e3966b01a59e59..1be14627684c95bb5cc944562f3f30f119c4821f 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 */
+/**/
+    1968,
 /**/
     1967,
 /**/