Very secure upload form

I have an upload form on my website (I allow up to 6 files to be uploaded)
I want to protect myself and make sure only images are uploaded
Is this a good way?

//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 to make sure its an image
       $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','bnp']) && $images["size"][$key] < $size_per_upload && $images["size"][$key] > 0) {

        $source = $images['tmp_name'][$key];   
        $target = $path_to_image_directory . $name;
	 
        move_uploaded_file($source, $target);
		}
	}
}

What do I do about the .jpeg extension (do I just add it in the array?)

Yeah there is a way to make it secure. The first thing you should do is DO NOT LOOK FOR THE FILE EXTENSION when “trying” to make it secure. File extensions are the very least of your worries. What if someone made a malicious file using say Adobe Dreamweaver, saved it as malicous_file.php, went into their file explorer, and then changed the file extension to .jpg, .gif, or .png? What will you do then because the file is marked as a “legitimate” jpg file, but isn’t actually a “legitimate” image file.

just to provide an alternative: the better way would be using a tool to determine the file format, like finfo

http://php.net/manual/de/function.finfo-file.php

especially for images it would be better to try to open it with imagepng() (or similar, corresponsding to the mime type you got)

and then changed the file extension to .jpg

And?.. What harm it would do?

There are 2 rules to follow:

  1. One is simple: do not allow executable file extensions. .php for example. Or, better, whitelist allowed extensions just like you do.
  2. But the other one is a headache: you should make sure that your code does not allow including an arbitrary file based on user input. As long as your code does not have this vulnerability, you’re safe.But if it does, then you’re busted, no matter how deep you inspect uploaded files. Though, in such a case you’re busted even without any upload at all.

I’ll take this matter into a PM. OP, you should take the appropriate precautions.

Honestly, it’s no use to post the possible attack scenario through PM - I am sure everyone is interested to see one.

It is more appropriate to take the rest of the conversation into a PM because it is “off-topic”. Maybe Stack Overflow doesn’t adhere to their own rules, but here at Sitepoint, if the matter is off-topic, it is appropriate to take it some where else.

Would like to continue this in the PM or do you really want me to discuss this publicly where everyone might understand the credibility of you and maybe we’ll end up having to use the PM anyways?

Honestly, I don’t understand what are you talking about.
I asked you a simple question about the attack scenario, which you surely have in mind, so we can discuss it here.
Why do you want to discuss my humble person instead I don’t understand.

1 Like

We can agree that an actual file extension can be changed to one that it is not.
What determines whether or not code in a file will be executed is whether or not whatever is working with the file uses the extension or differentiates only between it being a text or binary file.
I have not tested all possible scenarios and indeed have not tested recently so things may have improved.
But I have in the past proven to myself that testing based on the extension was woefully inadequate and that script in a file with a harmless looking extension could under a certain condition execute.

I will not provide details on how I was able to do it. I will only say that testing for file extension alone was at least at that time a security risk and that testing for mime type is much more secure.

2 Likes

I wasn’t taking the attack scenario into the PM. I took something else into the PM that was actually off-topic. Obviously you didn’t read and just assumed like you usually do with everyone’s code. You don’t have the slightest idea of anything apparently and everyone sucks up to you, but I am not going to because you are a troll. So if we want to talk about scenarios, we can. Since people who come to this forum are usually beginners, they don’t know what is secure and what is not. So say they have a script that allows uploading. If they “whitelist” the file exentions. The users could simply manipulate the file extension and try to break the uploading system. Since people are careless of how their codes are written, I wouldn’t doubt people are giving the uploaded file an executable permission or even fail to validate if the file is a legitimate image file or not.

But since you really “like” theories. I’d like to hear your spin because you are obviously telling the OP that it’s “ok” to upload any arbitrary file so long as they keep the file extension in a whitelist which is what normally BEGINNERS who know very little about security do. So can I ask you a question. Are you really a programmer or are you a beginner? Because your credibility seems to dig itself a deeper hole every time I see you post something that is obviously pro-legacy.

Maybe doing a check with PHPs getimagesize helps. I’m not sure it would be able to come up with a size if the image is invalid

@Mittineague You are slightly misinterpreting the connection between the content type and the the file contents. In fact, whatever content type that is worth talking about in the security context is strictly based on the filename. And the case you mentioned is not an exception.

Sorry, but you already wrote that, no need to repeat the same. The question is, what is that breaking scenario.
You are claiming that simply renaming, say, malicious.php to malicious.jpg will allow an attacker to break the system.
So the question is, how would they do it?

You see, one’s statements shouldn’t be just unfounded claims. Once you said there is a possible danger, you should be able to prove it with a plausible scenario. Otherwise one can make a thousand statements like “always replace all vowels in the filenames! Imagine if there will be one!!!” The problem is, I cannot imagine what is the problem with vowels. Security is not something that could be just a cargo cult-based approach, like “just do what you’ve been told!”. Quite contrary, it should be always a conscious approach, when a developer always realize both the attack and defense.

So, for the last time, care to explain how it’s possible to break a site by renaming malicious.php to malicious.jpg.

From the PHP Online Manual:

2 Likes

It’s just irrelevant here. That’s a mind trick, an extremely common one, that is misleading you here.
You are assuming here that a valid image automatically means “a harmless file” while it is not the same.
Your goal (in this topic) is not a valid image but a harmless file, so image validity is off topic.

1 Like

But a genuine image would be a harmless file no? I made my statement thinking there might be a way of checking that by performing some operation with the image that would return an exception if we did not have a genuine harmless image at hand

That’s again the question what are we talking about - the file name or the file contents? Given you were suggesting getimagesize, you had the file contents in mind. So of course a file that contains a valid image could contain a valid PHP code as well. In the EXIF headers for example.

1 Like

Okay and maybe even if there isn’t a way of 100% ensuring we have a valid harmless image wouldn’t a combination of all the suggested methods be the best approach?

I don’t think so.
Of course, for the business logic, it is mandatory to verify whether the image is valid. But speaking of security, such a verification will give you nothing.

In security, I prefer one but robust solution. You know, there us a saying, too many cooks will spoil the broth. That’s indeed the truth: having half a dozen different measures it’s too easy to confuse something

1 Like

OK, for some reason this topic has died away, but I feel there should be some conclusion.

Like I said above, if a file looks like a valid image, it doesn’t mean that this same file couldn’t serve as a valid PHP file as well. And therefore whatever content based mime-type sniffing won’t help at all. So I hope that everyone agrees that it’s impossible to prevent “malicious” content from uploading based on the file content (unless we would search every uploaded file for the <? symbols and reject a lot of innocent image files under such a ridiculous pretext).

Therefore our concern should be not the file contents but the possible execution. And for this reason it’s the filename should be our concern, as, assuming the only interface our site has is HTTP, it’s the idea that HTTP server has of the executable files. And it is judging them by extension. So, speaking of the mime type, the only acceptable is one provided by the HTTP server. As long as it counts a file executable - it is so.
Therefore, if you have means of getting the mime type from the server - it’s ok.

But if you can’t, you have to prevent the execution manually, by means of testing the file extension.
The sad news is that you cannot rely on the proper extension, as by some strange whim Apache HTTP server is configured by default to sniff the file type based on one of file’s “extensions”, for some obscure reason treating file.php.jpg as PHP executable. So, for the better security it’s better to rename a file after some meaningless hash, keeping only an extension, filtered out against a white list.

3 Likes