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> );

Take a look at [perlsec](http://perldoc.perl.org/perlsec.html) which tells you about "Taint" mode. Taint mode will mark as tainted any data that comes from the environment outside your program. Tainted data cannot be written back to the environment without being scrubbed. For example, what if the "EDITOR" was set to "rm -rf /& #"? — David W., Dec 15 '10 at 21:04

canavanin · Accepted Answer · 2010-12-16T08:38:09.770

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 :)

your humble suggestion conforms with best practice, as enforced by taint mode. — Narveson, Dec 15 '10 at 22:53

Jonathan Leffler · Answer 2 · 2010-12-15T17:31:19.873

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);)

score 1 · Answer 3 · answered Dec 15 '10 at 23:29

1

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.

answered Dec 15 '10 at 23:29

Sinan Ünür

116,958
15
196
339

then I would have to store settings... if I asked, because i know I wouldn't want to answer every time... and what are these variables but settings – xenoterracide Dec 17 '10 at 14:04
@xenoterracide Which is why I would normally not be worried about sanitizing these variables. Surely, a malicious attacker who was able to modify the user's environment would have been able to modify your configuration file as well. However, if you have to be paranoid, you might have to impose this inconvenience on the user. On the other hand, if you need that level of paranoia, there is no guarantee that `/usr/bin/vi` is `vi`. – Sinan Ünür Dec 17 '10 at 14:21

score 0 · Answer 4 · answered Dec 15 '10 at 17:15

0

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).

answered Dec 15 '10 at 17:15

Rosh Oxymoron

20,355
6
41
43

I'm sure environmental variables are safer than, say, user input, but still they're not under your own control, so the questioner is right, it's best practice to sanitize them. – Narveson Dec 15 '10 at 22:51
@Narveson: This begs the question of who “you” are when you’re talking about something not under “your” control. If “you” are whoever’s running the code, then there is no problem. If “you” are some other entity on whose behalf the code is being run, this is a taint situation. It’s all a matter of perspective. I see no cause for laundering your editor variable; it makes no sense from my perspective. – tchrist Dec 16 '10 at 07:43

How do I sanitize EDITOR, etc, environment variables?

4 Answers4