Uploading files

I have an upload script to upload a few image types (.jpg, .gif, and .png) which are less than 500 KB
my form has 8 file types whose names are images
Is this right?

//is at least 1 file being uploaded?		 
if(isset($images)) {
   //do this for each file
  foreach($images['name'] AS $key => $name) {
	//get the extension of the file
       $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION));
            //if the file has the right extension and is less than 500 KB, do the upload... 
            if( (in_array($ext, ['jpg', 'gif', 'png']) && ($images["size"][$key] < 512000)) ){
		$target_dir = "providers/".$primaryKey;;
		$img_url = "/".$target_dir."/".$name;
		$target_file = $target_dir . basename($_FILES["images"][$key]["name"]);
					  
                move_uploaded_file($_FILES["images"][$key]["tmp_name"], $target_file);

		}
	}
}

did it work?

no, nothing gets uploaded but I dont get any error either though…

Have you tried echoing some of the variables such as $target_file to see if they are wht you expect them to be?

did some fiddling with the code…

//is at least 1 file being uploaded?		 
if(isset($images)) {

   //do this for each file
  foreach($images['name'] AS $key => $name) {

	//get the extension of the file
       $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION));

            //if the file has the right extension and is less than 500 KB, do the upload... 
            if( (in_array($ext, ['jpg', 'gif', 'png']) || ($images["size"][$key] < $_POST['MAX_FILE_SIZE'])) ){
		    $target_dir = "/home/luke69/public_html/providers/".$primaryKey;
		    $img_url = "/".$target_dir."/".$name;

            move_uploaded_file($images[$key]["tmp_name"], $img_url);

			echo $images[$key]["tmp_name"]." -> ". $img_url; 

			echo '<img style="height:100px;float:left" src="'.$img_url.'" class="img-responsive img-thumbnail">';
		}
	}
}

Heres the result

→ //home/luke69/public_html/providers/16/84611899.gif
so it looks like $images[$key][‘tmp_name’] doesnt return anything. How do I target the temporary location of the uploaded file?

How many files did you upload? The structure of $_FILES is actually different for when you upload one file vs more than one.
Also, did you put the correct enctype on the form? It should be
<form enctype="multipart/form-data" action="__URL__" method="POST">

Lastly, regarding your original question, do not ever rely on user input. That also includes extensions of filenames. Just because someone named a file .jpg doesn’t mean that it is. Might be a malicious php script with a jpg extension. To prevent problems like these use fileinfo to open the file and get the mime type and check that. Don’t check the mime type in $_FILES because I’m pretty sure that can be spoofed too. If you really want to go all out, open the file with GD and then save it to disk, discarding the original upload. That will take care of any extra bytes an attacker may have wanted to sneak in that don’t belong.

Doesn’t even have to be hacker, there are still people who believe you can “convert” a bmp file to a jpg file by changing the extension :confused:

I have a form which can upload up to 8 files like

<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">
<input type="file" name="images[]">

and I place that array into $images

$images = $_FILES['images'];

so, its better if I do something like…
http://www.php.net/manual/en/book.fileinfo.php

//is at least 1 file being uploaded?		 
if(isset($images)) {

   //do this for each file
  foreach($images['name'] AS $key => $name) {

	//new code here}
	}
}

ok, got the upload script to work but there are a few bugs…

$size_per_upload = $_POST["MAX_FILE_SIZE"];
echo "MAX SIZE: ".$size_per_upload;


//is at least 1 file being uploaded?		 
if(isset($images)) {

   //do this for each file
  foreach($images['name'] AS $key => $name) {

	//get the extension of the file
       $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION));
       echo $ext;
            //if the file has the right extension and is less than 500 KB, do the upload... 
            if( (in_array($ext, ['jpg', 'gif', 'png']) || ($images["size"][$key] < $size_per_upload)) ){
		    $target_dir = "/home/luke69/public_html/providers/".$primaryKey;
		    $img_url = $target_dir."/".$name;

            move_uploaded_file($images["tmp_name"][$key], $img_url);

			echo $images["tmp_name"][$key]." -> ". $img_url; 

			echo '<img style="height:100px;float:left" src="/providers/'.$primaryKey.'/'.$name.'" class="img-responsive img-thumbnail">';
			echo "<br>";

		}
	}
}

And heres the result

MAX SIZE: 500000
gif
→ /home/luke69/public_html/providers/26/547eac1788842e6e418c1e36d9abc2ec1.gif
jpg
/tmp/phpEbsYUv → /home/luke69/public_html/providers/26/81055375.jpg
jpeg
/tmp/phpleJibu → /home/luke69/public_html/providers/26/20a0d3565ecc598cbe306d3e23fa524a.jpeg
→ /home/luke69/public_html/providers/26/
png
/tmp/php2exHrs → /home/luke69/public_html/providers/26/84977606.png
→ /home/luke69/public_html/providers/26/
So it looks like everything is working…
The only snag is that when the extension is jpeg, the upload still works, and why does the last loop run when the 1st image (gif whose site is 2.8MB) is run through the loop?
Shouldn’t the gif (2.8MB), the image with a wrong extension, and when nothing is uploaded be totally ignored at that point/
(And once I get that way working, ill use the more secure fileinfo() method to get the files mime type)

If you have a look at the code, you’ll see the problem:

  if( (in_array($ext, ['jpg', 'gif', 'png']) || ($images["size"][$key] < $size_per_upload)) ){

Even though your comment directly above says you’re making sure that the file is the correct type and less than 500Kb, you’re actually checking for it being the correct type or less than 500Kb. Hence the jpeg loads because it’s small, and the gif loads because it’s a valid file extension, and the blank probably loads because the size of a non-existent file is always comfortably below 500Kb.

2 Likes

Also there are way too many parentheses in that statement that are in the way of readability. You can simplify to

if (in_array($ext, ['jpg', 'gif', 'png']) && $images["size"][$key] < $size_per_upload) {
1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.