• What if in the future you add the possibility for users to add their own directories, and then forget to go back and change this code to check them before using them?
• What if through some other means someone manages to add a directory with a weird name?

foreach (@dir) {
  s/^(\w+)$/$1/ || do { generate some error message, or 
                        skip this directory };
}
[download]

A similar untainting should probably be applied to user-supplied data, plus checking that it is in the list of valid directories.

--ZZamboni

ZZamboni wrote:

I'm not sure if data read from readdir() is considered tainted or not, but you should check everything.

Your advice about checking everything is good. I just wanted to mention that the list of directory names from readdir is will result in tainted data. Perl assumes that everything from outside of the program is tainted, even directory names that have been read in.

Cheers,
Ovid

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

Hi ZZamboni,
I haven't sanitized everything yet, but here's the code I am using. Could you look it over and tell me what you think as far as security goes?

#!c:\perl\bin\perl.exe -wT
use strict;

use CGI qw(:standard);
my $cgi = new CGI;

my $element = $cgi->param('element');
my $type = $cgi->param('type');
my $page = $cgi->param('page');
my $dir="$page";
my @dir;
my $safedir;
opendir(TEXTFILES, "text_files") || die "Couldn't open the text file d
+irectory: $!";
@dir = grep { $_ ne "." && $_ ne ".." && -d "./text_files/$_" } readdi
+r (TEXTFILES);
closedir(TEXTFILES);
foreach (@dir) {
    if ($_ eq "$dir") {
        $safedir = "text_files/$_";
        last;
    }
}
######################################################################
+###########
# SPIT OUT THE FORM
######################################################################
+###########
if ($type eq "text") {
    opendir(CONTENTFILES, "$safedir") || die "Couldn't open the $safed
+ir directory: $!";
    my @files=grep(/\.txt$/i, readdir CONTENTFILES);
    closedir(CONTENTFILES);
    my $file_to_change;
    foreach (@files) {
        if ($_ eq "$element.txt") {
            $file_to_change = "$_";
            last;
        }
    }
    if ($file_to_change =~ /(\w+\.txt)/) {
        my $safe_file_to_change = $1;
        open(FILE, "$safedir/$safe_file_to_change") || die "Couldn't o
+pen $safe_file_to_change: $!";
        my @text_to_change=<FILE>;
        close(FILE);
        use HTMLTMPL;
        my $templ = new HTMLTMPL;
        $templ->src('text_form.html');
        my $title=ucfirst($element);
        $templ->title($title);
        $templ->element($element);
        $templ->text_to_change(@text_to_change);
        $templ->page($page);
        $templ->output('Content-Type: text/html');
    }
}

######################################################################
+###########
# CHANGE THE FILE
######################################################################
+###########
if ($type eq "text_change") {
    my $text = $cgi->param('text');
    opendir(CONTENTFILES, "$safedir") || die "Couldn't open the $safed
+ir directory: $!";
    my @files=grep(/\.txt$/i, readdir CONTENTFILES);
    closedir(CONTENTFILES);
    my $file_to_change;
    foreach (@files) {
        if ($_ eq "$element.txt") {
            $file_to_change = "$_";
            last;
        }
    }
    $file_to_change = "$safedir/$file_to_change";
    if ($file_to_change =~ /(text_files\/\w+\/\w+\.txt)/) {
        my $safe_file_to_change = $1;
        open(FILE, ">$safe_file_to_change") || die "Couldn't open $saf
+e_file_to_change: $!";
        print FILE $text;
        close(FILE);
    }
    use HTMLTMPL;
    my $templ = new HTMLTMPL;
    $templ->src('sucess_message.html');
    my $title=ucfirst($element);
    $templ->title($title);
    $templ->text($text);
    $templ->page($page);
    $templ->output('Content-Type: text/html');
}
[download]