Add missing calls to the shellescape() function #1099
Conversation
The following improvements were made... - Use variable sigils - Shorten a local variable name - Prefer an early return over testing for a negative - Switch to single quotes - Call "shellescape()" to pass a command argument [IMPORTANT!] The final change is a critical fix for the security and reliability of this function (see ":h system()"). Similar fixes for the other functions in this script will follow.
This commit makes several style improvements and adds a missing call to the "shellescape()" function. See also: 56cfbcf
Refer to: 56cfbcf
This commit handles the edge case where a user invokes the "reveal" function on "/" on a Linux box. There is nothing to do but open the root directory itself since "/" has no parent.
I initially thought that there were several more locations where a call to "shellescape()" was required but omitted. However, there are only two. I suppose I should have taken the time to look. Fixing these was easy. I would be surprised if this change breaks anything on the user side.
|
Note: I went ahead and added the remaining I have a few more improvements I want to make that are related to this, so be on the look-out for future PRs. As far as I can tell, though, this PR is done. Since this is security related, please review carefully. |
Use a more fitting description of the change...
lifecrisis
changed the title
| @@ -388,44 +388,67 @@ endfunction | |||
|
|
|||
| " FUNCTION: NERDTreeQuickLook() {{{1 | |||
| function! NERDTreeQuickLook() | |||
| let treenode = g:NERDTreeFileNode.GetSelected() | |||
| if treenode !=# {} | |||
| call system("qlmanage -p 2>/dev/null '" . treenode.path.str() . "'") | |||
There was a problem hiding this comment.
@lifecrisis , do you have a way to test this? These single quotes are thwarting my attempts to do the command injection in the old code.
There was a problem hiding this comment.
Most of these will allow injection when operating on a file that looks like...
'; touch FOOBAR ; echo '
Opening this file on Linux with mo, and the file FOOBAR will appear in Vim's current working directory. This snippet provides a simplified example of the mechanism:
func! g:Foo()
return "'; touch FOOBAR ; echo '"
endfunc
call system("xdg-open '" . g:Foo() . "' &")This problem seems like it would never happen, but you can do some funky things in the terminal... like concealing characters that actually are contained in the path using terminal escape codes. It's definitely better to use shellescape().
| @@ -199,7 +199,7 @@ function! s:Path.copy(dest) | |||
| let cmd_prefix = (self.isDirectory ? g:NERDTreeCopyDirCmd : g:NERDTreeCopyFileCmd) | |||
| endif | |||
|
|
|||
| let cmd = cmd_prefix . ' ' . escape(self.str(), self._escChars()) . ' ' . escape(a:dest, self._escChars()) | |||
| let cmd = cmd_prefix . ' ' . shellescape(self.str()) . ' ' . shellescape(a:dest) | |||
There was a problem hiding this comment.
Does shellescape() take into account the OS and the &shellslash setting?
There was a problem hiding this comment.
I don't know.
Because this is security related, though, I would recommend using it and only reverting it if people complain.
| call system("open '" . treenode.path.str() . "'") | ||
| let l:node = g:NERDTreeFileNode.GetSelected() | ||
|
|
||
| if empty(l:node) |
There was a problem hiding this comment.
Are you following a certain pattern here, instead of
if !empty()
do the thing
endifThere was a problem hiding this comment.
I always prefer early returns. They seem cleaner to me; i.e., I think they're simpler and read better.
Strictly speaking, though, we should probably be using try statements here. I always try to follow EAFP when possible.
Description of Changes
This is a security fix for
nerdtree_plugin/fs_menu.vimthat adds several missing calls toshellescape()that are actually required. It also adds missingshellescape()calls in two other places. This is necessary to prevent shell command injection.It is quite an unlikely exploit, however.
N.B. I suggest that other calls to
system()throughout the NERDTree code be audited for correct usage ofshellescape(). Some of them seem to use a custom escaping function that may or may not be correct. These probably should be converted to use theshellescape()function.See commit messages for more details about some of the ancillary changes.
New Version Info
Author's Instructions
MAJOR.MINOR.PATCHversion number. Increment the:MAJORversion when you make incompatible API changesMINORversion when you add functionality in a backwards-compatible mannerPATCHversion when you make backwards-compatible bug fixesCollaborator's Instructions
git tag -a 3.1.4 -m "v3.1.4" && git push origin --tags