How do I sanitize EDITOR, etc, environment variables?

Question

I've got the following code, but I'm thinking that I need to sanitize the env variables, but I'm not sure how exactly I should sanitize them. I realize there's probably a limit to how much I can sanitize them, but what can I do?

#!/usr/bin/perl
use 5.012;
use warnings;
use autodie;
use Env qw( EDITOR VISUAL );
use File::Temp qw( :seekable );

my $editor = '/usr/bin/nano';
if ( $VISUAL ) {
    $editor = $VISUAL;
}
elsif ( $EDITOR ) {
    $editor = $EDITOR;
} else {
    warn 'set VISUAL and EDITOR env variables not set falling back to nano'
    . "\n";
}

my $tmpf = File::Temp->new;

system $editor, $tmpf->filename;

open $tmpf, '<', $tmpf->filename;

print while ( <$tmpf> );

Answer 1

I have only ever done something like this in CGI scripts, so perhaps this is not at all what you're looking for; I'm just hoping it'll help a bit. Here's a modified version of the selection of allowed characters I used, and a code suggestion:

   my $editor = '/usr/bin/nano';
   my $allowed = 'a-zA-Z0-9.\-_/';

   # this is what I did, but you will probably not want to do this... 
   #$file =~ s/[^$allowed]//go; # Remove every character thats NOT in the OK-list

   # check that the variables contain only allowed characters
   if ($VISUAL =~ m/^[$allowed]+$/) {
      $editor = $VISUAL;
   }
   elsif ($EDITOR =~ m/^[$allowed]+$/) {
      $editor = $EDITOR;
   } 
   else {
      # message
   }

   # The code I have given above should also leave $editor in its default
   # state if neither $VISUAL nor $EDITOR has been set, as the condition
   # will not be true for empty strings/undef values.

Obviously, you cannot change the environment variables if you notice characters in them which you think shouldn't be there (i.e. characters which are not in the $allowed string), but you could check for the presence of such characters and fall back on your default editor in such a case. This is just my humble suggestion; perhaps an expert on the topic will reply in a while, and you'll get her/his wisdom served on a silver platter :)

Answer 2

Why do you need to sanitize them at all? What damage is going to occur if the user of your script has VISUAL="rm -f" and EDITOR to something else bizarre? You'd be checking that the editor operation succeeded, and that the file was opened, and that its contents made sense after the edit, before you did anything dangerous. But if the user is only able to damage their own files (you don't run a system where they can damage your files, do you?) then there isn't much need for sanitizing them. Providing a default is reasonable; local circumstances dictate whether nano is a better choice than, say, vim.

If the user can't damage your stuff by abusing VISUAL and EDITOR, then I would not worry too much about them choosing to damage their own stuff.

If you are writing something that will run with raised privileges - with SetUID or SetGID privileges, for example - then you have to worry a lot more about it. (The Perl code is missing checks that are more fundamental than worrying about the editor to use. Oh, but that use autodie; means the script automatically aborts on errors. That sounds a tad radical to me. But it probably does excuse you - though I note that it doesn't handle system or exec unless you have use autodie qw(:all);)

Answer 3

I think you need to think slightly differently about this. After all, your script seems to need user interaction. Therefore, it would not be unreasonable to ask the user which editor to use. You can issue a prompt right before executing the editor as in:

Which editor would you like to use [/usr/bin/vi]?

where the default would be filled in from $ENV{EDITOR}, $ENV{VISUAL} or if neither is defined, /usr/bin/nano. That way, the user can judge whether the value makes sense.

If you are paranoid about where $ENV{EDITOR} points, you might also have to be paranoid about whether someone could have placed a malicious executable in the user's path and named it greateditor or some such.

Answer 4

Using system with more than one argument doesn't require you to sanitize anything because no shell will be invoked. You might want to check if the executable exists first but it will complicate your program unnecessarily, and you'd have to look in $PATH. Also environmental variables like these are trusted in general.

You might want to call the exit status from system, though. And you might try calling $VISUAL first, and if it fails, to call $EDITOR (if $VISUAL is set, $EDITOR is supposed to act as a fallback).