Secure image upload

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Secure image upload

Post by julian_lp »

feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]


I'm gonna use Pseudo code in some part of the code, hopefully you'll be able tu uderstand

Code: Select all

$CONF_AVATAR_MAX_FILE_SIZE = whatever
    $arr_allowed = array (
         'image/x-png',
         'image/jpeg',
         'image/pjpeg'
      );

	 if(isset($_FILES['file_avatar'])){
		 if(in_array($_FILES['file_avatar']['type'],$arr_allowed)){
			$random_name = md5(uniqid);
			///EDITED, It was an important missing line
			$extension ='.BAD';
			
			switch (file_extension){
			case 'jpg':
				$extension ='.jpg';
				break;
			case 'png':
				$extension ='.png';
				break;
			}  

			$filenamefordatabase = $random_name.$extension;
			$uploadfile = UPLOAD_PATH.$random_name.$extension;

			 if ($_FILES['file_avatar']['size'] < $CONF_AVATAR_MAX_FILE_SIZE){
				if (move_uploaded_file($_FILES['file_avatar']['tmp_name'], $uploadfile)){
					///file uploaded


				}
			}
		}
	}

My main concern is not allowing some bad guy to execute his own php code on my server. Do you think it's enough just renaming the file? (after having checked its size and type of course).


feyd | Please use

Code: Select all

,

Code: Select all

and [syntax="..."] tags where appropriate when posting code. Your post has been edited to reflect how we'd like it posted. Please read:  [url=http://forums.devnetwork.net/viewtopic.php?t=21171]Posting Code in the Forums[/url] to learn how to do it too.[/color]
Last edited by julian_lp on Tue Oct 02, 2007 12:31 pm, edited 2 times in total.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

Well, this is definitely not secure. Don't rely on the ['type'] value to be accurate, as this can be changed by the end user. Instead you should use the getimagesize() function on the image. I believe that function analyzes the data of the image to make sure it's an image.

If you only wanted png and jpeg's to be uploaded, do something like this:

Code: Select all

if ($imageInfo = getimagesize($_FILES['whatever']['tmp_name']))
{
    if (!in_array($imageInfo[2], array(2, 3)))
    {
        echo 'Your image must be in JPG or PNG format.';
    } else
    {
        //rest of code in here
    }
} else
{
    echo 'You did not upload an image';
}
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

scottayy wrote:Well, this is definitely not secure. Don't rely on the ['type'] value to be accurate
Nice advice, but take into accont that i'm not strictly relying on ['type'], I mean, note that I already change the file extension either to .jpg or .png. ( I dont rely on original file name either)

In the worst scenario (the bad guy uploaded a php file after change its extension), images wont have the correct format and they are not going to be rendered properly by the browser, .... but why do you think it could be a risky code?
I'm interested in proofs about the potential risk I could be suffering (this code is already running :) )

I'll use your code though, many thanks again.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

But aren't you relying on ['type'] to get the extension? You don't have a default in your switch, so say a malicious .exe gets uploaded with no extension.. that could be bad. Even so, file extensions mean little. Just as a means to identify the type of file for ease of convenience for the operating system and end user. In any case, you don't want arbitrary code or malicious files to be able to be uploaded an on your web server. :)

Right now I could rename my hypothetical virus.exe to flowers.jpg and upload it to your server.

(I'm not trying to sound too anal here, but this IS the security forum :P)
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
xylex
Forum Newbie
Posts: 2
Joined: Thu Sep 06, 2007 5:59 am

Post by xylex »

I'm not sure of scottayy meant for you to leave it out, but I did want to specify that you still need your extension check/change after using getimagesize(). You don't want someone to be able to put code in an image header, but with a .php or some other executable extension, and execute it. It's a valid image, so getimagesize would return an accurate image size, but it would still be an executable script.
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

scottayy wrote: You don't have a default in your switch, so say a malicious .exe gets uploaded with no extension.. that could be bad.
Yes, I should've written a default value (as I said, it was pseudo code, in the original there is a default value already)
scottayy wrote:
Right now I could rename my hypothetical virus.exe to flowers.jpg and upload it to your server.
I understand that, but you wouldnt be able to execute it, cause apache wouldn't recognize it as a valid file. That's what I was trying to say.
I still would like a simple fact, step by step, about how anyone could exploit my upload code. (please consider that there is a default switch value, say ".bad", which I forgot to write in my example)
(I'm not trying to sound too anal here, but this IS the security forum :P)
I seriously appreciate your opinions, no problem :)
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

reformulating the question, I'd ask:

If every single file in a certain directory, is both ".jpg" or ".png" or ".gif", even if internally they dont have the correct format (they could be renamed .exe .php .js files), ....

Code: Select all

<img src="THE_DANGEROUS_FILE">
Are there any risks with regard to the security using those files in this way?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Yes. Whether it is dangerous for your site or the user is a different story. If it's possible to coherse PHP to execute one of these files, say via include() or directly, the files could do damage to your site. If the files are written in such a way to exploit a buffer overflow image processing, such as a browser, could cause the user's computer to be compromized as well.

I don't see a reason to even maintain a file extension at all.
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

feyd wrote:Yes. Whether it is dangerous for your site or the user is a different story. If it's possible to coherse PHP to execute one of these files, say via include() or directly, the files could do damage to your site. If the files are written in such a way to exploit a buffer overflow image processing, such as a browser, could cause the user's computer to be compromized as well.
You say "if it's posible", but.. Is it possible to "execute" a .jpg????
Including a file remotely (from another host) wouldn't be a problem to my site I guess
With regard to some buffer overflow exploit, you're right. But I think it's a common problem on every bbcode enabled forum software which allows linking any sort of image from any sort of location
feyd wrote: I don't see a reason to even maintain a file extension at all.
Well, the code is part of an "avatar" upload script, they should be able to upload their own image...
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

Yes it is possible.

I could write a script that outputs a jpg image and you'd never know that it was a script.

In my script I could have a malicious include, grab the contents of settings/passwords on your server, or other data.

Even if it were not possible to do such a thing, you're limiting the security to "the bad guys can get bad stuff on my server, but they can't execute it".

You need to get into the mindset that "the bad guys can't even get it on my server". If you're expecting an image, make sure it's an image. It's proper input validation, and security 101, in my opinion.

EDIT| Oh, and about the bbcode and other web sites linking to remote images.. that point is completely null because they are just that.. remote. Any code included, eval'd, gathered, etc is happening on a remote server. If the "images" are on your server, then any code ran on your server is going to directly effect your server (if that is the intent of the code, of course).
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

scottayy wrote:Yes it is possible.
How?
scottayy wrote: I could write a script that outputs a jpg image and you'd never know that it was a script.

Please remember that I rename every file as a .jpg or .png so it never gets executed (It wont render as an image either, but that's not big deal). You still didn't show me a concrete way to execute a .jpg file.

scottayy wrote: In my script I could have a malicious include, grab the contents of settings/passwords on your server, or other data.
Again, you wouldnt be able to run your script.
scottayy wrote: Even if it were not possible to do such a thing, you're limiting the security to "the bad guys can get bad stuff on my server, but they can't execute it".
That's true, I dont see any problem with regard to bad "users". There exists a risk if someone gain acces to the server though (but if there is someone with access to the server, every thing is going wrong and not only the upload script)

NOTE: As I said, I'm really interested in knowing concrete and showable risks with my script. I'll be the first one in recognize it and shout the alarm.

IMHO there are no facts yet, just "if it is possible" "it could" ,,,, etc
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Asking for concrete ways to basically execute malicious code on ones server is not a discussion we allow here, however what has been advocated thus far is absolutely correct.

Basically, if your code relies on server settings, which may be altered down the road, and without realizing, you may be opening a security vulnerability elsewhere on your server. For instance, there are several legitimate reasons to map an image extension to the php intepreter (dynamic sigs?).
User avatar
julian_lp
Forum Contributor
Posts: 121
Joined: Sun Jul 09, 2006 1:00 am
Location: la plata - argentina

Post by julian_lp »

Jcart wrote:Asking for concrete ways to basically execute malicious code on ones server is not a discussion we allow here, however what has been advocated thus far is absolutely correct.
So we are talking about security, we are on php forum, I wrote a piece of code asking for help and no one who could eventully see some *concrete* problem with it, is allowed to be clear and say where the problem is?
Very interesting...

Jcart wrote: Basically, if your code relies on server settings, which may be altered down the road, and without realizing, you may be opening a security vulnerability elsewhere on your server. For instance, there are several legitimate reasons to map an image extension to the php intepreter (dynamic sigs?).
I guess every single script you write rely on certain server settings, so, again, that's not to do with my original question.

I need to repeat that I appreciate every response you gave me, this in not a personal issue by no means.

To every other user in this forum:
If you think you know where could be the risk, (with a real example) just post your reply.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

You asked for security and we told you of potential exploits of your code -- making good use of the forum title and giving you enough incentive to change your code.

Being good willed volunteers, I doubt anyone on this forum is going to post a malicious exploit (and it's not allowed).
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

julian_lp wrote:
Jcart wrote:Asking for concrete ways to basically execute malicious code on ones server is not a discussion we allow here, however what has been advocated thus far is absolutely correct.
So we are talking about security, we are on php forum, I wrote a piece of code asking for help and no one who could eventully see some *concrete* problem with it, is allowed to be clear and say where the problem is?
Very interesting...
I would rather not take any chances of legal ramifications of providing you with software that could potentially do a lot of harm. I'll pass.

I guess every single script you write rely on certain server settings, so, again, that's not to do with my original question.
Some settings yes, but usually those kinds of settings are not as important. Magic quotes is an excellent example, reguardless of the server settings we should be prepared if the setting is active, although if it is off it should not affect the scripts behavior. AddHandler for instance is very important to know which file extensions are being processed with the php intepreter. Hint hint.
I need to repeat that I appreciate every response you gave me, this in not a personal issue by no means.
I never thought it was :D
To every other user in this forum:
If you think you know where could be the risk, (with a real example) just post your reply.
We've already told you of one potential exploit, have you fixed that yet?
Post Reply