Re: Possible Security Problem

[ZZamboni]

I don't see why you should get flamed for asking this. On the contrary, everyone should get feedback on security issues before writing CGI programs :-)

My answer is: it depends on the setup, and what you do with the directory afterwards. Judging from your previous post, I assume that is going to be your "list of valid subdirectories".

Are these directories predefined and created by you? If they are not (i.e. they can be created by users) then you should be careful with them, even if they pass your "validity" check. I think using -d is OK because it does not interpret any metacharacters AFAIK, but if you later use that name in something that does (like open or system), you will get in trouble if you don't sanitize the names before allowing them to pass.

If the valid directories can only be created by you, as long as you are careful with their names, I think you should be OK. In any case, it's best if you do something to untaint the data before using it in any possibly vulnerable commands.

--ZZamboni

[Stamp_Guy]

ZZamboni: you are correct in your assumption. Yea, the directories are completely under my control, not the user's. So I need to use taint checking on the results from that directory listing too or are you saying to just untaint the vars I receive from the CGI input? I later use the directory in an open command. Is that ok? I'm a bit confused as to how far I need to go.

[ZZamboni]

I would sanitize everything anyway. Although the directories are under your control, think about these:
• 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?
I'm not sure if data read from readdir() is considered tainted or not, but you should check everything. Come up with a regex that describes all the valid directory names that you expect to have, and check against that. Something like this:

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

Which will allow directory names with any alphanumeric characters plus the underscore. Remember not to check for invalid characters (you could miss some), but to only allow valid ones (like the code above).

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

--ZZamboni

[Ovid]

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.

[Stamp_Guy]

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