security and un-tainting paths

wrinkles has asked for the wisdom of the Perl Monks concerning the following question:

Mighty Monks,

I have been looking at a CMS that builds a filepath to a template based on a base directory path (from config), a relative page path (a query parameter), an optional directory path (a query parameter). and finally a template extension (from config).

The resulting file path is executed with "do EXPR".

As neither of the query parameters were untainted, I had trouble running the calling cgi script in taint mode. So I added the untainting regexes which you can see in the code block below. And the code now successfully runs in taint mode.

My questions:

Have I done this untaint thing properly?
Is the regex appropriate for capturing directories and filenames?
Has my effort improved security?

I appreciate all advice/criticism on coding style etc.

sub load {
    my $self = shift;
    
    my $conf = $self->conf;
    my $query = $self->query;
    
    my @tainted_dirs = File::Spec->splitdir( $query->{dir} );
    my ($tainted_page_volume,$tainted_page_path,$tainted_page_file) = 
+File::Spec->splitpath( $query->{page} );
    my @tainted_page_dirs = File::Spec->splitdir( $tainted_page_path )
+;

    no locale;
    # untaint dir
    my @untainted_dirs;
    foreach (@tainted_dirs) {
        if (m/^([\w.-]+)$/) {
            push (@untainted_dirs, "$1") ;
        } elsif ($_) {
            croak ("TAINTED DIR PARAM: $_: $!");
        }
    }

    my @untainted_page_dirs;
    foreach (@tainted_page_dirs) {
        if (m/^([\w.-]+)$/) {
            push (@untainted_page_dirs, "$1") ;
        } elsif ($_) {
            croak ("TAINTED PAGE DIR PARAM: $_: $!");
        }
    }

    my $untainted_page_file;
    if ($tainted_page_file and $tainted_page_file=~ m/^([\w.-]+)$/) {
        $untainted_page_file = $1;
    } else {
        croak ("TAINTED PAGE FILE PARAM: $tainted_page_file: $!");
    }
    
    my $template_file = File::Spec->catfile(
        $conf->{templates},
        @untainted_dirs,
        @untainted_page_dirs,
        $untainted_page_file . $conf->{template_extension},
    );
    
    my $template = do $template_file
        or croak "Failed to load template file [$template_file] ($!) (
+$@)";

    return $self->template( $template );
}
[download]

Comment on security and un-tainting paths Download Code

Replies are listed 'Best First'.
Re: security and un-tainting paths by graff (Chancellor) on Oct 22, 2011 at 01:55 UTC
Have I done this untaint thing properly? Your regexes are imposing sensible limits on the range of characters allowed for input, but you aren't ruling out things like "../../../../.." as part of the final resulting path string. More on this below. Is the regex appropriate for capturing directories and filenames? Provided that your directories and file names are in fact limited to using the characters that your regexes allow, yes. You are explicitly not allowing names that include spaces or any punctuation other than period and hyphen, and (IMHO) that's reasonable, prudent, and basically good. But you really should guard against "../../../../.." -- I'd limit the range of acceptable paths further, to exclude any name that begins with "." -- change your regexes to: `m/^(\w[\w.-])$/; # path component must begin with alphnumeric` [download] Has my effort improved security?* Well, if the only difference is that the earlier version used the tainted variables in the `File::Spec->catfile()` call, I think you probably have provided some extra protection, by virtue of limiting the variety of path names that can be passed to `do` -- but you really should fix the "../../.." problem. Even after fixing that, there's still a question whether you've improved security enough. The basic risk that remains is whether a hacker is able to plant files and use this app to execute those files. The OP code is only using read access to the directories and files, but if you don't enforce a suitable level of protection for write access, files could be planted by other means, and executed via your app. An alternative method for protection is for the app to know which paths and template files are safe, have those in a hash or other data structure, and "do" a path/file name selected from that structure (because it matched the input exactly). For that matter, if the purpose of the "do" is to run a template for presenting content, why not use a real templating module (e.g. Template::Toolkit? Calling a template module method is likely safer than executing perl code directly from a file. One last point: assuming this is a web app, I would hope that the caller is wrapping its call to this sub inside an "eval", to trap the croaking and present some appropriate response to the (misguided) client, rather than just dying. (Personally, I'd have this function return a recognizable "fail" result, and let the caller decide whether to die or not.)	[reply] [d/l] [select]
Re^2: security and un-tainting paths by wrinkles (Pilgrim) on Oct 23, 2011 at 06:47 UTC
Graff, thanks for the detailed reply, very much appreciated. I will fix the ../ possibility in my regex. In fact, the application checks a configuration hash for allowed files, as you suggested. I had suspected that was a security feature, now I know for sure. The base template and page templates are generally hashrefs which specify HTML::Template templates and other data, but may also run perl code. It does look like the main executable is called by eval. My desire to run in taint mode is motivated more by my desire to catch newbie mistakes on my part, than by a distrust in the base application (though I do try to cultivate a healthy distrust by default). Again, thanks for your help, learning opportunities like this help me build a mental framework upon which I can develop with reading on my own.	[reply]
Re: security and un-tainting paths by afresh1 (Hermit) on Oct 22, 2011 at 02:31 UTC
For keeping user supplied paths sane, you probably want the no_upwards method from File::Spec (and the other methods from the same module you are already using). l8rZ, -- andrew	[reply]