Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window.  Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space. If a user puts junk in EDITOR, they deserve what they get (much more than virsh will misbehave); furthermore, sudo scrubs EDITOR by default.  So the blind use of metacharacters in EDITOR should not be considered too much of a security issue. * tools/virsh.c (editFile): Prefer VISUAL over EDITOR.  Don't reject shell metacharacters in EDITOR. * tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL. ---

@@ -639,10 +639,15 @@ of C<virsh>  The hypervisor to connect to by default. Set this to a URI, in the same  format as accepted by the B<connect> option. -=item EDITOR +=item VISUAL  The editor to use by the B<edit> and B<net-edit> options. +=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. +

Common Unix practice is to prefer VISUAL over EDITOR, particularly if the editor of choice spawns a new window. Thus, it is also common to see settings like EDITOR='emacs -nw', with the expectation that the shell will parse this as an argument to 'emacs' and not try to invoke a file containing a space. If a user puts junk in EDITOR, they deserve what they get (much more than virsh will misbehave); furthermore, sudo scrubs EDITOR by default. So the blind use of metacharacters in EDITOR should not be considered too much of a security issue. * tools/virsh.c (editFile): Prefer VISUAL over EDITOR. Don't reject shell metacharacters in EDITOR. * tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL. --- tools/virsh.c | 19 ++++++++----------- tools/virsh.pod | 15 ++++++++++----- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index aa85ee6..9f3b197 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7381,20 +7381,17 @@ editFile (vshControl *ctl, const char *filename) char *command; int command_ret; - editor = getenv ("EDITOR"); + editor = getenv ("VISUAL"); + if (!editor) editor = getenv ("EDITOR"); if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */ - /* Check the editor doesn't contain shell meta-characters, and if - * it does, refuse to run. + /* Check that filename doesn't contain shell meta-characters, and + * if it does, refuse to run. Follow the Unix conventions for + * EDITOR: the user can intentionally specify command options, so + * we don't protect any shell metacharacters there. Lots more + * than virsh will misbehave if EDITOR has bogus contents (which + * is why sudo scrubs it by default). */ - if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) { - vshError(ctl, - _("%s: $EDITOR environment variable contains shell meta or " - "other unacceptable characters"), - editor); - return -1; - } - /* Same for the filename. */ if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) { vshError(ctl, _("%s: temporary filename contains shell meta or other " diff --git a/tools/virsh.pod b/tools/virsh.pod index 302de18..3707916 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -332,8 +332,8 @@ This is equivalent to: virsh define domain.xml except that it does some error checking. -The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>. =item B<migrate> optional I<--live> I<--suspend> I<domain-id> I<desturi> I<migrateuri> @@ -557,8 +557,8 @@ This is equivalent to: virsh define network.xml except that it does some error checking. -The editor used can be supplied by the C<$EDITOR> environment -variable, or if that is not defined defaults to C<vi>. +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>. =item B<net-list> optional I<--inactive> or I<--all> @@ -639,10 +639,15 @@ of C<virsh> The hypervisor to connect to by default. Set this to a URI, in the same format as accepted by the B<connect> option. -=item EDITOR +=item VISUAL The editor to use by the B<edit> and B<net-edit> options. +=item EDITOR + +The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL> +is not set. + =item LIBVIRT_DEBUG=LEVEL Turn on verbose debugging of all libvirt API calls. Valid levels are --