Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [day] [month] [year] [list]
Date: Tue, 22 Nov 2016 11:19:15 -0500
From: James McCoy <jamessan@...ian.org>
To: oss-security@...ts.openwall.com
Subject: vim/neovim: Arbitrary command execution (CVE-2016-1248)

Hi all,

CVE-2016-1248 was assigned for a vulnerability in Vim which would allow
arbitrary shell commands to be run if a user opened a file with a
malicious modeline.  This is due to lack of validation of values for a
few options.  Those options' values are then used in Vim's scripts to
build a command string that's evaluated by :execute, which is what
allows the shell commands to be run.

This has been fixed in Vim by patch 8.0.0056[0], and new Windows builds
of Vim have been published with the fix, however the implications have
not yet been disclosed.

Since Neovim shares this code, it is also vulnerable.  It is fixed by
commit 4fad66f[1], but has not yet had a release.

This affects Vim at least as far back as 7.0.  I didn't check any older
versions.

This affects all released versions of Neovim.

Thanks to Florian Larysch for discovering this issue.

[0]: https://github.com/vim/vim/releases/tag/v8.0.0056
[1]: https://github.com/neovim/neovim/commit/4fad66fbe637818b6b3d6bc5d21923ba72795040

Cheers,
-- 
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB

commit d0b5138ba4bccff8a744c99836041ef6322ed39a
Author: Bram Moolenaar <Bram@....org>
Date:   Fri Nov 4 15:23:45 2016 +0100

    patch 8.0.0056
    Problem:    When setting 'filetype' there is no check for a valid name.
    Solution:   Only allow valid characters in 'filetype', 'syntax' and 'keymap'.

diff --git a/src/option.c b/src/option.c
index ebf443b..8eea1f8 100644
--- a/src/option.c
+++ b/src/option.c
@@ -5823,6 +5823,21 @@ set_string_option(
 }
 
 /*
+ * Return TRUE if "val" is a valid 'filetype' name.
+ * Also used for 'syntax' and 'keymap'.
+ */
+    static int
+valid_filetype(char_u *val)
+{
+    char_u *s;
+
+    for (s = val; *s != NUL; ++s)
+	if (!ASCII_ISALNUM(*s) && vim_strchr((char_u *)".-_", *s) == NULL)
+	    return FALSE;
+    return TRUE;
+}
+
+/*
  * Handle string options that need some action to perform when changed.
  * Returns NULL for success, or an error message for an error.
  */
@@ -6235,8 +6250,11 @@ did_set_string_option(
 #ifdef FEAT_KEYMAP
     else if (varp == &curbuf->b_p_keymap)
     {
-	/* load or unload key mapping tables */
-	errmsg = keymap_init();
+	if (!valid_filetype(*varp))
+	    errmsg = e_invarg;
+	else
+	    /* load or unload key mapping tables */
+	    errmsg = keymap_init();
 
 	if (errmsg == NULL)
 	{
@@ -7222,6 +7240,22 @@ did_set_string_option(
     }
 #endif
 
+#ifdef FEAT_AUTOCMD
+    else if (gvarp == &p_ft)
+    {
+	if (!valid_filetype(*varp))
+	    errmsg = e_invarg;
+    }
+#endif
+
+#ifdef FEAT_SYN_HL
+    else if (gvarp == &p_syn)
+    {
+	if (!valid_filetype(*varp))
+	    errmsg = e_invarg;
+    }
+#endif
+
     /* Options that are a list of flags. */
     else
     {
diff --git a/src/testdir/test_options.vim b/src/testdir/test_options.vim
index 21dd7fe..dee435c 100644
--- a/src/testdir/test_options.vim
+++ b/src/testdir/test_options.vim
@@ -48,3 +48,52 @@ func Test_signcolumn()
   endif
 endfunc
 
+func Test_filetype_valid()
+  set ft=valid_name
+  call assert_equal("valid_name", &filetype)
+  set ft=valid-name
+  call assert_equal("valid-name", &filetype)
+
+  call assert_fails(":set ft=wrong;name", "E474:")
+  call assert_fails(":set ft=wrong\\\\name", "E474:")
+  call assert_fails(":set ft=wrong\\|name", "E474:")
+  call assert_fails(":set ft=wrong/name", "E474:")
+  call assert_fails(":set ft=wrong\\\nname", "E474:")
+  call assert_equal("valid-name", &filetype)
+
+  exe "set ft=trunc\x00name"
+  call assert_equal("trunc", &filetype)
+endfunc
+
+func Test_syntax_valid()
+  set syn=valid_name
+  call assert_equal("valid_name", &syntax)
+  set syn=valid-name
+  call assert_equal("valid-name", &syntax)
+
+  call assert_fails(":set syn=wrong;name", "E474:")
+  call assert_fails(":set syn=wrong\\\\name", "E474:")
+  call assert_fails(":set syn=wrong\\|name", "E474:")
+  call assert_fails(":set syn=wrong/name", "E474:")
+  call assert_fails(":set syn=wrong\\\nname", "E474:")
+  call assert_equal("valid-name", &syntax)
+
+  exe "set syn=trunc\x00name"
+  call assert_equal("trunc", &syntax)
+endfunc
+
+func Test_keymap_valid()
+  call assert_fails(":set kmp=valid_name", "E544:")
+  call assert_fails(":set kmp=valid_name", "valid_name")
+  call assert_fails(":set kmp=valid-name", "E544:")
+  call assert_fails(":set kmp=valid-name", "valid-name")
+
+  call assert_fails(":set kmp=wrong;name", "E474:")
+  call assert_fails(":set kmp=wrong\\\\name", "E474:")
+  call assert_fails(":set kmp=wrong\\|name", "E474:")
+  call assert_fails(":set kmp=wrong/name", "E474:")
+  call assert_fails(":set kmp=wrong\\\nname", "E474:")
+
+  call assert_fails(":set kmp=trunc\x00name", "E544:")
+  call assert_fails(":set kmp=trunc\x00name", "trunc")
+endfunc
diff --git a/src/version.c b/src/version.c
index 0b62f9c..f63041e 100644
--- a/src/version.c
+++ b/src/version.c
@@ -765,6 +765,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    56,
+/**/
     55,
 /**/
     54,

commit 4fad66fbe637818b6b3d6bc5d21923ba72795040
Author: James McCoy <jamessan@...essan.com>
Date:   Sun Nov 20 08:42:38 2016 -0700

    vim-patch:8.0.0056
    
    Problem:    When setting 'filetype' there is no check for a valid name.
    Solution:   Only allow valid characters in 'filetype', 'syntax' and 'keymap'.
    
    https://github.com/vim/vim/commit/d0b5138ba4bccff8a744c99836041ef6322ed39a

diff --git a/src/nvim/option.c b/src/nvim/option.c
index 0f95974..469aeec 100644
--- a/src/nvim/option.c
+++ b/src/nvim/option.c
@@ -2399,6 +2399,18 @@ static char *set_string_option(const int opt_idx, const char *const value,
   return r;
 }
 
+/// Return true if "val" is a valid 'filetype' name.
+/// Also used for 'syntax' and 'keymap'.
+static bool valid_filetype(char_u *val)
+{
+  for (char_u *s = val; *s != NUL; s++) {
+    if (!ASCII_ISALNUM(*s) && vim_strchr((char_u *)".-_", *s) == NULL) {
+      return false;
+    }
+  }
+  return true;
+}
+
 /*
  * Handle string options that need some action to perform when changed.
  * Returns NULL for success, or an error message for an error.
@@ -2623,8 +2635,12 @@ did_set_string_option (
     xfree(p_penc);
     p_penc = p;
   } else if (varp == &curbuf->b_p_keymap) {
-    /* load or unload key mapping tables */
-    errmsg = keymap_init();
+    if (!valid_filetype(*varp)) {
+      errmsg = e_invarg;
+    } else {
+      // load or unload key mapping tables
+      errmsg = keymap_init();
+    }
 
     if (errmsg == NULL) {
       if (*curbuf->b_p_keymap != NUL) {
@@ -3118,8 +3134,16 @@ did_set_string_option (
       if (check_opt_strings(p_icm, p_icm_values, false) != OK) {
         errmsg = e_invarg;
       }
-  // Options that are a list of flags.
+  } else if (gvarp == &p_ft) {
+    if (!valid_filetype(*varp)) {
+      errmsg = e_invarg;
+    }
+  } else if (gvarp == &p_syn) {
+    if (!valid_filetype(*varp)) {
+      errmsg = e_invarg;
+    }
   } else {
+    // Options that are a list of flags.
     p = NULL;
     if (varp == &p_ww)
       p = (char_u *)WW_ALL;
diff --git a/src/nvim/testdir/test_options.vim b/src/nvim/testdir/test_options.vim
index cceb180..93657f8 100644
--- a/src/nvim/testdir/test_options.vim
+++ b/src/nvim/testdir/test_options.vim
@@ -38,3 +38,53 @@ function! Test_path_keep_commas()
 
   set path&
 endfunction
+
+func Test_filetype_valid()
+  set ft=valid_name
+  call assert_equal("valid_name", &filetype)
+  set ft=valid-name
+  call assert_equal("valid-name", &filetype)
+
+  call assert_fails(":set ft=wrong;name", "E474:")
+  call assert_fails(":set ft=wrong\\\\name", "E474:")
+  call assert_fails(":set ft=wrong\\|name", "E474:")
+  call assert_fails(":set ft=wrong/name", "E474:")
+  call assert_fails(":set ft=wrong\\\nname", "E474:")
+  call assert_equal("valid-name", &filetype)
+
+  exe "set ft=trunc\x00name"
+  call assert_equal("trunc", &filetype)
+endfunc
+
+func Test_syntax_valid()
+  set syn=valid_name
+  call assert_equal("valid_name", &syntax)
+  set syn=valid-name
+  call assert_equal("valid-name", &syntax)
+
+  call assert_fails(":set syn=wrong;name", "E474:")
+  call assert_fails(":set syn=wrong\\\\name", "E474:")
+  call assert_fails(":set syn=wrong\\|name", "E474:")
+  call assert_fails(":set syn=wrong/name", "E474:")
+  call assert_fails(":set syn=wrong\\\nname", "E474:")
+  call assert_equal("valid-name", &syntax)
+
+  exe "set syn=trunc\x00name"
+  call assert_equal("trunc", &syntax)
+endfunc
+
+func Test_keymap_valid()
+  call assert_fails(":set kmp=valid_name", "E544:")
+  call assert_fails(":set kmp=valid_name", "valid_name")
+  call assert_fails(":set kmp=valid-name", "E544:")
+  call assert_fails(":set kmp=valid-name", "valid-name")
+
+  call assert_fails(":set kmp=wrong;name", "E474:")
+  call assert_fails(":set kmp=wrong\\\\name", "E474:")
+  call assert_fails(":set kmp=wrong\\|name", "E474:")
+  call assert_fails(":set kmp=wrong/name", "E474:")
+  call assert_fails(":set kmp=wrong\\\nname", "E474:")
+
+  call assert_fails(":set kmp=trunc\x00name", "E544:")
+  call assert_fails(":set kmp=trunc\x00name", "trunc")
+endfunc


[ CONTENT OF TYPE application/pgp-signature SKIPPED ]

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ