CODEWITHSUNDEEP
phpsecurityvalidationimage-uploading
crazytrain999
crazytrain999

Reputation: 21

validating file upload server side - allow only images with php

new guy here asking a question that what should be met with simple solutions.

I have tried a bunch of code. It seems I can get the file stream for getimagesize and get other things to work without crashing.

I'm dusting off an old project that needs to limit the files uploaded so that they are only image files and nothing evil.

This code always give me an error message no matter what

$imageinfo = getimagesize($_FILES['bf_file'][$key]['tmp_name']);  
if($imageinfo['mime'] != 'image/gif' && $imageinfo['mime'] != 'image/jpeg') {  
alert ("Sorry, we only accept GIF and JPEG images");  
exit;  
}

Here is the black list effort

$blacklist = array(".php", ".phtml", ".php3", ".php4", ".js", ".shtml", ".pl" ,".py"   
,".txt", ".doc");

foreach ($blacklist as $file)
{
if(preg_match("/$file\$/i", $_FILES['bf_file'][$key]['tmp_name']))
{
alert "ERROR: Uploading executable files Not Allowed\n";
exit();
}
}

Here is another getimagesize

$size = getimagesize($_FILES[bf_file][$key][tmp_name]);
$fp = fopen($_FILES[bf_file][$key][tmp_name], "rb");
if ($size && $fp) {
header("Content-type: {$size['mime']}");
fpassthru($fp);
continue;
} else
// error
alert("Inappropriate file type");

On each of these I get the error message no matter if a file is uploaded or not.

I just need to place these controls somewhere in my file so that if the uploaded file passes the checks then everything just passes through as the uploader and everything else works like it should but without the benefit of these limiters and checks.

Also, the user should not be required to upload file. There are 3 fields, subject, body and file upload. Only subject and body are required to have data and that works right now.

Any help will be greatly appreciated.

Thanks,

James

Upvotes: 2

Views: 2941

Answers (3)

BenLanc
BenLanc

Reputation: 2434

By far the safest approach is to prevent your webserver from executing dynamic stuff in folders that user can upload to entirely. Then it doesn't matter what they upload.

An .htaccess file in your uploads directory containing php_flag engine off will prevent php. Other executable stuff should be disabled by default anyway, but you should definitely check.

Upvotes: 0

Baba
Baba

Reputation: 95121

Your script is all over the place example 

$imageinfo = getimagesize($_FILES['bf_file'][$key]['tmp_name']);

To get temp name should be $_FILES['bf_file']['tmp_name'][$key] and also file size is already returned via $_FILES['bf_file']['size'][$key]

Why don't you take a look at for details example on similar question

multi image upload wrong quantity on file-upload

Uploading images with the help of arrays and fetch errors

Upvotes: 1

null
null

Reputation: 11869

In first snippet. Well, getimagesize() actually returns MIME type (contrary to what Baba says), but you shouldn't depend on it. It's entirely possible to make file which at beginning looks like PNG GIF (any reason to block PNG?), but after header has <?php dangerous_code(); ?>. Also, I don't know what are you trying with [$key]. I don't know what it does and array looks like $_FILES[$form_name][$file_field] (for example $_FILES['file_input']['tmp_size']. There is no third field. Unless you are doing multiple files uploads, then look into what Baba said (it's very hacky feature). Next, PHP doesn't have alert() - you probably meant echo.

In second snippet, I see you're doing stuff incorrectly. Dot is meta character in regular expressions, but in this case it doesn't really matter. Blacklist approaches are flawed anyways as you don't know if your server doesn't support .php5 extension for example. And even if it doesn't, somebody can abuse content negotiation in Apache by making file hack.php.fr (Apache thinks that .fr is language). Your approach is flawed - just give PNG files .png extension no matter what original extension was and so on.

In third example, you activate wrong variable - but also use barewords (you shouldn't, while I'm aware that you should have constants uppercase (so contrary to what PHP says, barewords aren't so dangerous if you have common sense), they are extremally slow, way slower than normal string and make a lot of errors if you have E_NOTICE (hint: you should)). Next, continue is not for if conditionals - it's for loop conditionals (it also works on switch (as break), but I guess it's just for consistency).

As for not having to upload files, it's easy. Just do conditional over isset($_FILES['file_input_name']).

tl;dr - Learn PHP properly

Upvotes: 1

Related Questions