in reply to Testing file input validity in FF

What all should I be allowing?

Somewhat depends on what exactly you're doing with the files and filenames...

Just as an example, you sometimes wouldn't want people to be able to get outside of a certain subdirectory tree with things like /../../../, in which case you'd need to disallow two dots in a row (as a directory name)...

If checking against a whitelist would work in your case, I'd do that (e.g. using a hash).

Some more details/context might help.

Replies are listed 'Best First'.
Re^2: Testing file input validity in FF
by ksublondie (Friar) on Feb 17, 2009 at 22:01 UTC
    When uploading the file, the file isn't saved, but the data within the file is processed. Here's the code for processing the uploaded file (I've cleaned it up and simplified. It's a program I inherited.):
    sub upload { my $self = shift; my $q = $self->query; if(untaintFile($q->param('file')) && $q->upload('file')) { my $filename=untaintFile($q->param('file')); ($filename) = $filename =~ /([[:ascii:]]+)/; my $file = $q->upload('file'); my $data; while(<$file>) { $data .= $_; } if($data) { my $thisfile = File->parse($data); if(my $format_error = $thisfile->check_formatt +ing()) { ...warn the user if there are any file + format errors... return; } my $file_id = $self->file_upload({name => $fil +ename, data => $data}); } } $self->header_type('redirect'); $self->header_props(-url => "$CONFIG{userbase}/user.pl"); return; } sub parse { my $proto = shift; my $origfile = shift; my @lines = (); my $file = $origfile; LINE: while(length($file) >= 94) { $file =~ s/^\D*//; push @lines, substr($file, 0, 94, ''); if(@lines && substr($lines[-1], 0, 1) eq '9') { last LINE; # Don't need to parse any more afte +r the file control record } } my @batches = (); for(my $i = 0; $i < @lines; $i++) { if(substr($lines[$i], 0, 1) eq '5') { # Batch Header my @tempbatch = (); for(; $i < @lines; $i++) { push @tempbatch, $lines[$i]; if(substr($lines[$i], 0, 1) eq '8') { +# Batch Control last; } } push @batches, File::Batches->new(\@tempbatch) +; } } my $self = { file => $origfile, lines => \@lines, batches => \@batches, total_subtract => substr($lines[-1], 31, 12), total_add => substr($lines[-1], 43, 12), }; bless $self, $proto; return $self; } sub file_upload { my $self = shift; my %params = validate(@_, { name => { type => SCALAR }, data => { type => SCALAR }, }); my $thisFile = File->parse($params{data}); my @effective_dates; tie @effective_dates, 'Array::Unique'; @effective_dates = $thisFile->get_effective_dates; ...insert data into database... return $id; #return the primary key }
      No browser will send a full path for input type file.
        IE does...

      Here's a few checks you could easily add:

      • Was a file name entered?
      • Does this file contain data?
      • Is the data of the expected size/type/format?

      It looks like you're assuming that it's a text file, and you read it into memory line by line. But what if someone accidentally sends you a large binary file (maybe it's the correct file, only it's been zipped)?

      Here's a revised version of your upload() function that uses read() to read the file into a buffer 1kb at a time, and abort if it looks too big (you can set your own limit).

      sub upload { my $self = shift; my $q = $self->query; if(untaintFile($q->param('file')) && $q->upload('file')) { my $filename=untaintFile($q->param('file')); ($filename) = $filename =~ /([[:ascii:]]+)/; unless($filename) { croak "no filename given!"; } my $file = $q->upload('file'); unless($file) { croak "can't get filehandle for upload!"; } my $data; my $buffer; my $bytesread = 0; my $totalbytes = 0; while($bytesread = read($file, $buffer, 1024)) { $totalbytes += $bytesread; if($totalbytes > $MAX_DATA_LIMIT) { croak "upload file too big!"; } $data .= $buffer; } unless ($data) { croak "no data!"; } else { my $thisfile = File->parse($data); if(my $format_error = $thisfile->check_formatting()) { # warn if any format errors found return; } my $file_id = $self->file_upload({ name => $filename, data => $data, }); } } $self->header_type('redirect'); $self->header_props(-url => "$CONFIG{userbase}/user.pl"); return; }
        Thanks for the input. I'll try that. What about taint? Will that satisfy taint?