0

I have written a little bit of code for checking a file input for the extension and only letting through if its allowed but it doesnt seem to be working.

I call the function by using the following:

$vcFileType = check_filetype($HTTP_POST_FILES['cv']['name'],"That filetype isnt allowed punk",array('doc','docx','pdf'));

and the function is as follows:

function check_filetype($file, $problem='', $types){
    $fileExt = substr($file, strrpos($file, '.') + 1);
    if(in_array($fileExt, $types)){
        show_error($problem);
    }
    return $file;
}

If someone could let me know where im going wrong, that would be grand.

Cheers,

Cecil
  • 1,609
  • 5
  • 21
  • 26
  • 2
    You're assuming that file extensions have anything to do with the type of a file (sure, they might on your machine, but will they on the "punk"'s machine?) – derobert Jan 18 '11 at 07:08

4 Answers4

2
if (in_array($fileExt, $types)) {
    show_error($problem);
}

"If the extension is one of the allowed extensions, show the error..."

Apart from this logic glitch: never trust the user supplied file name!
It's completely arbitrary and easily faked.
Try to actually process the file or find its MIME type instead.

Community
  • 1
  • 1
deceze
  • 510,633
  • 85
  • 743
  • 889
  • +1 for MIME types, although you can still hide stuff in a file without changing its MIME type :) – nico Jan 18 '11 at 07:40
  • @nico True, (carefully) processing the file is the only way to find out if it is what it says it is. – deceze Jan 18 '11 at 07:44
1

Exactly, checking a file name for it's extensions is not a save way to go. Someone can still hide a malicious file in a PDF file for example.

Checking for an extension can be a good way for an initial check though to skip the mime check if the extension is not right already.

Sander
  • 1,244
  • 1
  • 12
  • 24
1

It looks like you're only grabbing the first character of the extension. string position + 1.

Also, since you're looking for the first instance of "." your program will break with anything named something like inc.functions.php.

try this instead:

function check_filetype($file, $problem='', $types){
    $file_parts = explode(".", $file);
    $fileExt = array_pop($file_parts);
    if(in_array($fileExt, $types)){
        show_error($problem);
    }
    return $file;
}

This will turn the file name into an array based on the "." regardless of how many many be in the name and then return the last element of that array - the extension.

jerebear
  • 6,503
  • 4
  • 31
  • 38
  • `strrpos` finds the *last* occurrence of a string, and `+ 1` just excludes the `.` itself. The extension-grabbing part of that code happens to be the correct part. Apart from not using [`pathinfo`](http://php.net/manual/en/function.pathinfo.php) of course. ;) – deceze Jan 18 '11 at 07:41
0

If you're on Unix, you may use the file command: it examines the contents of the files to deduce their types.

ChrisJ
  • 5,161
  • 25
  • 20