By using this site, you agree to our updated Privacy Policy and our Terms of Use. Manage your Cookies Settings.
455,847 Members | 1,326 Online
Bytes IT Community
+ Ask a Question
Need help? Post your question and get tips & solutions from a community of 455,847 IT Pros & Developers. It's quick & easy.

Checking for file type.

Kelicula
Expert 100+
P: 176
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)
Expand|Select|Wrap|Line Numbers
  1.  
  2.  
  3. if(my $file = $q->param('avatar')){
  4.  
  5. $CGI::POST_MAX = 1024 * 39; # Limit to 39 kb.
  6.  
  7.   my $dir = '../img/avatars';
  8.   my $charsok = 'a-zA-Z0-9_.-';
  9.   my ($filename,undef,$ext) = fileparse($file,qr{\..*});
  10.  
  11.   if($ext !~ /jpeg|gif|png/gi){
  12.       $emess .= qq* <li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li> *;
  13.         }
  14.   else{
  15.  
  16.   $filename .= $ext;
  17.   $filename =~ tr/ /_/;
  18.   $filename =~ s/[^$charsok]//g;
  19.  
  20.     if($filename =~ /^([$charsok]+)$/){
  21.       $filename = $1;
  22.  
  23.  
  24.     my $upload_file = $q->upload('avatar');
  25.  
  26.     open(FILE, ">$dir/$filename") || Tools->listErr( $! );
  27.     binmode FILE;
  28.  
  29.     while (<$upload_file>){
  30.     print FILE;
  31.     }
  32.  
  33.     close(FILE);
  34.  
  35.     $sth = $dbh->prepare(qq~ UPDATE users SET avatar=? WHERE id=? ~);
  36.     $sth->execute( "$dir/$filename", $user->{id}) || Tools->listErr( $sth->errstr );
  37.     $sth->finish;
  38.     }
  39.     else{
  40.     $emess .= "<li>Filename not valid, may only contain these characters: $charsok</li>";
  41.    }
  42.   }
  43. }
  44.  
Anyone know why the $ext variable isn't matching?
I can upload a .exe and it won't skip a beat....

NOT good.
Feb 17 '08 #1
Share this Question
Share on Google+
12 Replies


KevinADC
Expert 2.5K+
P: 4,059
looks like it should work

Expand|Select|Wrap|Line Numbers
  1. use warnings;
  2. use File::Basename;
  3. foreach my $file qw(path/to/foo.exe path/to/foo.jpg) {
  4.   my ($filename,$dir,$ext) = fileparse($file,qr{\..*});
  5.   print "$filename,$dir,$ext\n";
  6.   if($ext !~ /jpeg|gif|png/gi){
  7.         print qq*  That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.*;
  8.   }
  9.   else {
  10.      print "  It's good $ext\n";
  11.   }
  12. }
  13.  
I'd tighten up the regexp but it should work the way you have it.
Feb 17 '08 #2

Kelicula
Expert 100+
P: 176
looks like it should work

Expand|Select|Wrap|Line Numbers
  1. use warnings;
  2. use File::Basename;
  3. foreach my $file qw(path/to/foo.exe path/to/foo.jpg) {
  4.   my ($filename,$dir,$ext) = fileparse($file,qr{\..*});
  5.   print "$filename,$dir,$ext\n";
  6.   if($ext !~ /jpeg|gif|png/gi){
  7.         print qq*  That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.*;
  8.   }
  9.   else {
  10.      print "  It's good $ext\n";
  11.   }
  12. }
  13.  
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?
Feb 18 '08 #3

eWish
Expert 100+
P: 971
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.

Expand|Select|Wrap|Line Numbers
  1. if (not $ext =~ /^\.?(?i:jpe?g|gif|png)$/) {
  2.  
--Kevin
Feb 18 '08 #4

KevinADC
Expert 2.5K+
P: 4,059
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.

Expand|Select|Wrap|Line Numbers
  1. if (not $ext =~ /^\.?(?i:jpe?g|gif|png)$/) {
  2.  
--Kevin

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

Expand|Select|Wrap|Line Numbers
  1. 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:

Expand|Select|Wrap|Line Numbers
  1. 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.
Feb 18 '08 #5

Kelicula
Expert 100+
P: 176
That is pretty much just what I was going to suggest:

Expand|Select|Wrap|Line Numbers
  1. 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:

Expand|Select|Wrap|Line Numbers
  1. 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:
Expand|Select|Wrap|Line Numbers
  1. #!/usr/bin/perl -Tw
  2. use strict;
  3.  
  4. if($ext !~ /\.+(?:jpeg|jpg|gif|png)/xoi){
  5.       $emess .= qq*<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>*;
  6.   }
  7.  
  8.  
  9. # Then later...
  10.  
  11. if($filename =~ /^([$charsok]+)$/){
  12.       $filename = $1;
  13.   }
  14.  

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!
Feb 18 '08 #6

eWish
Expert 100+
P: 971
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.
Expand|Select|Wrap|Line Numbers
  1. .....jpegggy
  2. .giftsarecool
  3. .pngreen
  4.  
--Kevin
Feb 18 '08 #7

Plater
Expert 5K+
P: 7,872
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...
Feb 18 '08 #8

Kelicula
Expert 100+
P: 176
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.
Expand|Select|Wrap|Line Numbers
  1. .....jpegggy
  2. .giftsarecool
  3. .pngreen
  4.  
--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...
Expand|Select|Wrap|Line Numbers
  1. #
  2. if($ext !~ /\.(?:jpeg|jpg|gif|png)\z/oi){
  3.  
  4.  
Thanks! eWish..
Feb 18 '08 #9

eWish
Expert 100+
P: 971
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
Feb 18 '08 #10

Kelicula
Expert 100+
P: 176
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;
Expand|Select|Wrap|Line Numbers
  1.  
  2. if($ext !~ /^\w*\.(?:jpeg|jpg|gif|png)$/oi)
  3.  
  4.  

Seems to work for me in this test..
Expand|Select|Wrap|Line Numbers
  1. foreach my $e qw(/path/too/foo.jpeg /path/too/foo.exe /peth/too/foo.gif.gif /path/to/foo...gif){
  2. my ($filename,undef,$ext) = fileparse($e,qr{\..*});
  3.   print $ext."\n";
  4.   if($ext !~ /^\w*\.(?:jpeg|jpg|gif|png)$/oi){
  5.       print qq*<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>\n\n*;
  6.   }else{
  7.   print "OK\n\n";
  8.   }
  9. }
  10.  
  11.  
With only .jpeg going through...
Feb 18 '08 #11

eWish
Expert 100+
P: 971
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.
Expand|Select|Wrap|Line Numbers
  1. if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {
  2. # OR
  3. if ($ext !~ /^\.?(?i:jpe?g|gif|png)$/) {
  4. # OR
  5. 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
Feb 18 '08 #12

Kelicula
Expert 100+
P: 176
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.
Expand|Select|Wrap|Line Numbers
  1. if ($ext !~ /^ \.jpe?g | \.gif | \.png $/iox) {
  2. # OR
  3. if ($ext !~ /^\.?(?i:jpe?g|gif|png)$/) {
  4. # OR
  5. 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;
Expand|Select|Wrap|Line Numbers
  1. if($ext !~ /^\.(?:jpe?g|gif|png)$/oi){
  2.   $emess .= qq{<li>That is an invalid file, avatars MUST be in "jpeg", "gif" or "png" format.</li>};
  3.   }
  4.  
Also changed the "*" character to a simple {.

Thanks guys!!
Feb 19 '08 #13

Post your reply

Sign in to post your reply or Sign up for a free account.