Testing Image upload size

PHP
1

Hi all,

The following statement tests the correct image type and size has been submitted. It checks for the correct file type but it is letting images of more than 1024kb through, and I was wondering if anyone has any idea why?

Many thanks!


function validateFileUploads($imageUpload, $documentUpload, $saveFile)
{
	$fErrors = array();
	if((!empty($imageUpload)) && ($imageUpload['error'] == 0))
	{
		//Check if the file is JPEG image and it's size is less than 350Kb
		$filename = basename($imageUpload['name']);
		$ext = substr($filename, strrpos($filename, '.') + 1);
		
		if((($ext == "jpg") && ($imageUpload["type"] == "image/jpeg")) 
		 ||(($ext == "gif") && ($imageUpload["type"] == "image/gif"))
		 && ($imageUpload["size"] < 1024))
		{
			if($saveFile)
			{
				//proceed with image file save
				saveUploadToNewDirectory($filename, $imageUpload["tmp_name"]);
			}	
		}
		else
		{
			array_push($fErrors, "Only .jpg or .gif images 1 megabyte or less are accepted for upload");
		}
2

Do not do this:


if((($ext == "jpg") && ($imageUpload["type"] == "image/jpeg"))  
         ||(($ext == "gif") && ($imageUpload["type"] == "image/gif"))

Tis bad.


$imgdata = getimagesize( $_FILE['tmp_name'] );
if ( !in_array( $image[2], array( IMAGETYPE_GIF, IMAGETYPE_JPEG ) ) )
  // bad

May not work, going off the top of my head. Furthermore, 1024 for the size is 1 KB not 1024 KB. You have to remember the size is in bytes.

3

errr Thanks ?

4

Here’s what’s happening.

if((($ext == "jpg") && ($imageUpload["type"] == "image/jpeg"))  
         ||(($ext == "gif") && ($imageUpload["type"] == "image/gif")) 
         && ($imageUpload["size"] < 1024))

Is it a jpg image? Yes - and an OR condition is there. Since (True || unknown) is always true, no need to check.
See the logical operators page where foo() never gets called.

The image upload check ends up being completely bypassed.

You have two options:

  1. Use parenthesis to provide more explicit instructions to PHP
  2. Or, separate the imageUpload check to a different if statement
5

Basically I want to check if an image is a Jpeg or a Gif, and it is less than 1024kb :slight_smile: - it could be either of these types

6

You have two options:

  1. Use parenthesis to provide more explicit instructions to PHP
  2. Or, separate the imageUpload check to a different if statement
7

I thought I was doing this, but clearly there is something wrong with my logic! Could you kindly give me an example?

8

Let’s looks at the following:

if ((($ext == "jpg") && ($imageUpload["type"] == "image/jpeg"))  
         ||(($ext == "gif") && ($imageUpload["type"] == "image/gif")) 
         && ($imageUpload["size"] < 1024))

That can be simplified to a failure scenario of:

if (((true) && (true)) ||((false) && (false)) && (false))

Let’s simplify that further, without changing the meaning:

if (true || (false && false))

Now you can see that the above condition is only checking the size on gif images.

Instead of that, you need the parenthesis around the two file-type conditions, like this:

if ((true || false) && false)

Can you work that back up to what you need to change in your code?

9

That is very clear, and I see exactly what you are saying thanks ill try it now :slight_smile:

10 

function gettype ( $image )
{
  if ( function_exists( 'exif_imagetype' ) )
    return exif_imagetype( $image )

  $tmp = getimagesize( $image );
  return $tmp[ 2 ];
}

$imgtype = gettype( $_FILE[ 'inputname' ][ 'tmp_name' ] );

if ( !in_array( $imgtype, array( IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG ) ) )
  exit( 'Incorrect Image Type!' );

if ( filesize( $_FILE[ 'inputname' ][ 'tmp_name' ] ) > 1048576 ) # This is 1 MB.
  exit( 'Must not be larger then 1 MB is size!' );
11

You’re welcome.

After updating it, you may want to move the complicated condition out to a separate function so that the condition is easier to understand.


function isAcceptableImage($imageUpload, $ext)
{
    return ...
}

if (isAcceptableImage($imageUpload, $ext)) {
    ...
}

Your future-self will thank you for making it easier to understand.

12

Thats a great idea Paul thankyou :), and thanks logic_earth for your alternative idea!

13

I’m going to disagree with logic_earth here that purely checking a file using getimagesize and exif_imagetype is sufficient for security. They can both be bypassed by a malicious file that has a malformed header that will pass verification but has active content that will be parsed and executed by php .

14

That is why you never execute or use the supplied file name by the client. However, using the type value from $_FILE is a no go. Or are you saying one should use that value for determining if the file is an image? If not then you agree with me.

By the way, I never said anything about this being “sufficient security”, please don’t put words in my mouth that I never spoken.

15

Yes using type from $_FILE is a no go. However, when your code is read by many less skilled php users on this forum, particularly when they see it posted by an experienced person such as yourself, many will take it as a finished and secure item, so it’s worth expanding on.