Checking for file type.

[Kelicula]

I am trying to ensure that only files of a certain type can be uploaded.

Why doesn't this work??

(It's a code snippet..not the whole file)

 
 
if(my $file = $q->param('avatar')){
 
$CGI::POST_MAX = 1024 * 39; # Limit to 39 kb.
 
  my $dir = '../img/avatars';
  my $charsok = 'a-zA-Z0-9_.-';
  my ($filename,undef,$ext) = fileparse($file,qr{\..*});
 
  if($ext !~ /jpeg|gif|png/gi){
      $emess .= qq* <li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li> *;
        }
  else{
 
  $filename .= $ext;
  $filename =~ tr/ /_/;
  $filename =~ s/[^$charsok]//g;
 
    if($filename =~ /^([$charsok]+)$/){
      $filename = $1;
 
 
    my $upload_file = $q->upload('avatar');
 
    open(FILE, ">$dir/$filename") || Tools->listErr( $! );
    binmode FILE;
 
    while (<$upload_file>){
    print FILE;
    }
 
    close(FILE);
 
    $sth = $dbh->prepare(qq~ UPDATE users SET avatar=? WHERE id=? ~);
    $sth->execute( "$dir/$filename", $user->{id}) || Tools->listErr( $sth->errstr );
    $sth->finish;
    }
    else{
    $emess .= "<li>Filename not valid, may only contain these characters: $charsok</li>";
   }
  }
}
 

Anyone know why the $ext variable isn't matching?
I can upload a .exe and it won't skip a beat....

NOT good.

[KevinADC]

looks like it should work

use warnings;
use File::Basename;
foreach my $file qw(path/to/foo.exe path/to/foo.jpg) {
  my ($filename,$dir,$ext) = fileparse($file,qr{\..*});
  print "$filename,$dir,$ext\n";
  if($ext !~ /jpeg|gif|png/gi){
        print qq*  That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.*;
  }
  else {
     print "  It's good $ext\n";
  }
}
 

I'd tighten up the regexp but it should work the way you have it.

[Kelicula]

looks like it should work

use warnings;
use File::Basename;
foreach my $file qw(path/to/foo.exe path/to/foo.jpg) {
  my ($filename,$dir,$ext) = fileparse($file,qr{\..*});
  print "$filename,$dir,$ext\n";
  if($ext !~ /jpeg|gif|png/gi){
        print qq*  That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.*;
  }
  else {
     print "  It's good $ext\n";
  }
}
 

I'd tighten up the regexp but it should work the way you have it.

Ah Ha!
It was working, the problem was that I was trying to create the error message before the uploading occurred. So the error was being created after the upload...
I never waited long enough!!
silly me....

Guess I'll have to deploy some 'ole javascript.


PS- Kevin (ADC) any ideas, on tightening the regex?

[eWish]

I would also like to see what KevinADC offers, but I feel like this is fairly tight and just not sure on the efficiency of it though.

if (not $ext =~ /^\.?(?i:jpe?g|gif|png)$/) {
 

--Kevin

[KevinADC]

I would also like to see what KevinADC offers, but I feel like this is fairly tight and just not sure on the efficiency of it though.

if (not $ext =~ /^\.?(?i:jpe?g|gif|png)$/) {
 

--Kevin

That is pretty much just what I was going to suggest:

if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {

the dot might be optional at the beginning of some file extensions but I prefer to force that it is required instead of being optional.

It is also important to note that this does not stop people from uploading an .exe file renamed to .txt (for example) or maybe worse yet an .htaccess file. So if you allow users to rename files after uploading them make sure that any rename/move/copy routine also checks that users can't rename files with any illegal file extension. I have seen this in many scripts, they only check when a file is uploaded and never again to make sure the user isn't trying to do something dangerous or malicious.

There is a module File::Type (as well as others) that can be used to further check the type of file the user is uploading. But I noted a while back that the one review of the module calls into question some inefficiency issues so you may want to read the review on CPAN and see if you think it's worths implementing or not.

You can also use the uploadInfo method of the CGI module:

my $type = $query->uploadInfo($filename)->{'Content-Type'};

See the CGI documentation for details.

Also, you should be using Taint mode and untainting any filenames the user attempts to upload. All CGI scripts should operate in Taint mode.

[Kelicula]

That is pretty much just what I was going to suggest:

if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {

the dot might be optional at the beginning of some file extensions but I prefer to force that it is required instead of being optional.

It is also important to note that this does not stop people from uploading an .exe file renamed to .txt (for example) or maybe worse yet an .htaccess file. So if you allow users to rename files after uploading them make sure that any rename/move/copy routine also checks that users can't rename files with any illegal file extension. I have seen this in many scripts, they only check when a file is uploaded and never again to make sure the user isn't trying to do something dangerous or malicious.

There is a module File::Type (as well as others) that can be used to further check the type of file the user is uploading. But I noted a while back that the one review of the module calls into question some inefficiency issues so you may want to read the review on CPAN and see if you think it's worths implementing or not.

You can also use the uploadInfo method of the CGI module:

my $type = $query->uploadInfo($filename)->{'Content-Type'};

See the CGI documentation for details.

Also, you should be using Taint mode and untainting any filenames the user attempts to upload. All CGI scripts should operate in Taint mode.

Thanks guys for the quick replies!!

Yes absolutely, I AM in Taint mode.
But for this check I don't need to backtrack, so I have come up with this:

#!/usr/bin/perl -Tw
use strict;
 
if($ext !~ /\.+(?:jpeg|jpg|gif|png)/xoi){
      $emess .= qq*<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>*;
  }
 
 
# Then later...
 
if($filename =~ /^([$charsok]+)$/){
      $filename = $1;
  }
 

Using $1 as KevinADC recommends in his tutorial, allowing perl to untaint $filehandle.

I am NOT going to allow users to rename the files. They can either upload a new avatar, or keep this one, but they don't get a choice as to where it gets stored or even the name of it. Just as long as That same picture come up when they log-in they should be happy.

Lot's of good tips though.
I'll check out that File::type mod.
As well as, the "Uploadinfo" method.

Happy coding!

[eWish]

I would include the ^ and $ to your regex and not allow more than one . (dot) else things like this will still make it past your check point.

.....jpegggy
.giftsarecool
.pngreen
 

--Kevin

[Plater]

Be sure to watch out for MAC users and their * being a valid character.
I and my foolish code "lost" a directory on my unix server do to that once...

[Kelicula]

I would include the ^ and $ to your regex and not allow more than one . (dot) else things like this will still make it past your check point.

.....jpegggy
.giftsarecool
.pngreen
 

--Kevin

Ah HA!!
Yes I incorrectly chose "x" at the end of the matching separator. When I meant "\z", or "$" within it. I DO want it to match a end of string.

Also, I had thought of the dot, and somehow thought that if they had a dot in the name of the file, only allowing one dot would NDTRT. So I decided that having a dot in part of the file name is not so bad.
After all if you want to name your file "hello..." .gif, that's ok with me, I'm only concerned with the "extension".

so I guess it should be...

#
if($ext !~ /\.(?:jpeg|jpg|gif|png)\z/oi){
 
 

Thanks! eWish..

[eWish]

So lets take the hypothetically situation that may never arise, but it is still valid. Say the user uploads a file like this c:/path/to/some.example.image.jpg. File::Basename would capture the file extension as ".example.image.jpg". Then when you check the extension it will still pass. If you have the anchors at the start and end of the string then you would eliminate this.

The reason I mention this is because if you are going to store these in the database eventually this could cause you problems down the road.

--Kevin

[Kelicula]

So lets take the hypothetically situation that may never arise, but it is still valid. Say the user uploads a file like this c:/path/to/some.example.image.jpg. File::Base name would capture the file extension as ".example.image.jpg". Then when you check the extension it will still pass. If you have the anchors to the from and end of the string you would eliminate this.

The reason I mention this is because if you are going to store these in the database eventually this could cause you problems down the road.

--Kevin

Humm...
OK, so should it be;

 
if($ext !~ /^\w*\.(?:jpeg|jpg|gif|png)$/oi)
 
 

Seems to work for me in this test..

foreach my $e qw(/path/too/foo.jpeg /path/too/foo.exe /peth/too/foo.gif.gif /path/to/foo...gif){
my ($filename,undef,$ext) = fileparse($e,qr{\..*});
  print $ext."\n";
  if($ext !~ /^\w*\.(?:jpeg|jpg|gif|png)$/oi){
      print qq*<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>\n\n*;
  }else{
  print "OK\n\n";
  }
}
 
 

With only .jpeg going through...

[eWish]

Since you are checking the extension once File::Basename has done it's job you could use the regex either of the regexs that Kevin and myself suggested. They would eliminate the other problems. Also, it would help to prevent a user from using (.) in the filename and make the user use underscores as well.

Any of these should work.

if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {
# OR
if ($ext !~ /^\.?(?i:jpe?g|gif|png)$/) {
# OR
if($ext !~ /^\.(?:jpeg|jpg|gif|png)\z/oi){

This way the extension can only consist of a dot followed by j,p,e,g or j,p,g and so on.

--Kevin

[Kelicula]

Since you are checking the extension once File::Basename has done it's job you could use the regex either of the regexs that Kevin and myself suggested. They would eliminate the other problems. Also, it would help to prevent a user from using (.) in the filename and make the user use underscores as well.

Any of these should work.

if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {
# OR
if ($ext !~ /^\.?(?i:jpe?g|gif|png)$/) {
# OR
if($ext !~ /^\.(?:jpeg|jpg|gif|png)\z/oi){

This way the extension can only consist of a dot followed by j,p,e,g or j,p,g and so on.

--Kevin

OK, so for the forum purposes I settled on this;

if($ext !~ /^\.(?:jpe?g|gif|png)$/oi){
  $emess .= qq{<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>};
  }
 

Also changed the "*" character to a simple {.

Thanks guys!!