This code allows people to upload images to a site. It comes from here. The script works perfectly. Can anyone see any security issues with it? <?php //define a maxim size for the uploaded images in Kb define ("MAX_SIZE","100"); //This function reads the extension of the file. It is used to determine if the file is an image by checking the extension. function getExtension($str) { $i = strrpos($str,"."); if (!$i) { return ""; } $l = strlen($str) - $i; $ext = substr($str,$i+1,$l); return $ext; } //This variable is used as a flag. The value is initialized with 0 (meaning no error found) //and it will be changed to 1 if an errro occures. //If the error occures the file will not be uploaded. $errors=0; //checks if the form has been submitted if(isset($_POST['Submit'])) { //reads the name of the file the user submitted for uploading $image=$_FILES['image']['name']; //if it is not empty if ($image) { //get the original name of the file from the clients machine $filename = stripslashes($_FILES['image']['name']); //get the extension of the file in a lower case format $extension = getExtension($filename); $extension = strtolower($extension); //if it is not a known extension, we will suppose it is an error and will not upload the file, //otherwise we will do more tests if (($extension != "jpg") && ($extension != "jpeg") && ($extension != "png") && ($extension != "gif")) { //print error message echo '<h1>Unknown extension!</h1>'; $errors=1; } else { //get the size of the image in bytes //$_FILES['image']['tmp_name'] is the temporary filename of the file //in which the uploaded file was stored on the server $size=filesize($_FILES['image']['tmp_name']); //compare the size with the maxim size we defined and print error if bigger if ($size > MAX_SIZE*1024) { echo '<h1>You have exceeded the size limit!</h1>'; $errors=1; } //we will give an unique name, for example the time in unix time format $image_name=time().'.'.$extension; //the new name will be containing the full path where will be stored (images folder) $newname="images/".$image_name; //we verify if the image has been uploaded, and print error instead $copied = copy($_FILES['image']['tmp_name'], $newname); if (!$copied) { echo '<h1>Copy unsuccessfull!</h1>'; $errors=1; }}}} //If no errors registred, print the success message if(isset($_POST['Submit']) && !$errors) { echo "<h1>File Uploaded Successfully! Try again!</h1>"; } ?> <!--next comes the form, you must set the enctype to "multipart/frm-data" and use an input type "file" --> <form name="newad" method="post" enctype="multipart/form-data" action=""> <table> <tr><td><input type="file" name="image"></td></tr> <tr><td><input name="Submit" type="submit" value="Upload image"></td></tr> </table> </form> Code (php):
I added a fix. Check it out...First line: $allowedexts and the in_array where you checked extensions. <?php $AllowedExts=array('jpg','png','jpeg','.gif'); //define a maxim size for the uploaded images in Kb define ("MAX_SIZE","100"); //This function reads the extension of the file. It is used to determine if the file is an image by checking the extension. function getExtension($str) { $i = strrpos($str,"."); if (!$i) { return ""; } $l = strlen($str) - $i; $ext = substr($str,$i+1,$l); return $ext; } //This variable is used as a flag. The value is initialized with 0 (meaning no error found) //and it will be changed to 1 if an errro occures. //If the error occures the file will not be uploaded. $errors=0; //checks if the form has been submitted if(isset($_POST['Submit'])) { //reads the name of the file the user submitted for uploading $image=$_FILES['image']['name']; //if it is not empty if ($image) { //get the original name of the file from the clients machine $filename = stripslashes($_FILES['image']['name']); //get the extension of the file in a lower case format $extension = getExtension($filename); $extension = strtolower($extension); //if it is not a known extension, we will suppose it is an error and will not upload the file, //otherwise we will do more tests // Added by Thomas - thomas.p.andrews[at]gmail.com if (!in_array($extension,$AllowedExts)) { //print error message echo '<h1>Unknown extension!</h1>'; $errors=1; } else { //get the size of the image in bytes //$_FILES['image']['tmp_name'] is the temporary filename of the file //in which the uploaded file was stored on the server $size=filesize($_FILES['image']['tmp_name']); //compare the size with the maxim size we defined and print error if bigger if ($size > MAX_SIZE*1024) { echo '<h1>You have exceeded the size limit!</h1>'; $errors=1; } //we will give an unique name, for example the time in unix time format $image_name=time().'.'.$extension; //the new name will be containing the full path where will be stored (images folder) $newname="images/".$image_name; //we verify if the image has been uploaded, and print error instead $copied = copy($_FILES['image']['tmp_name'], $newname); if (!$copied) { echo '<h1>Copy unsuccessfull!</h1>'; $errors=1; }}}} //If no errors registred, print the success message if(isset($_POST['Submit']) && !$errors) { echo "<h1>File Uploaded Successfully! Try again!</h1>"; } ?> <!--next comes the form, you must set the enctype to "multipart/frm-data" and use an input type "file" --> <form name="newad" method="post" enctype="multipart/form-data" action=""> <table> <tr><td><input type="file" name="image"></td></tr> <tr><td><input name="Submit" type="submit" value="Upload image"></td></tr> </table> </form> PHP:
Thanks for that NatalicWolf. +Rep. Do you (or does anyone else) think there's any prospect of someone uploading something damaging using this script?
its not about the uploading file, but the question is do you have your upload folder writable? if so anybody can write on the folder and could compromise the server. see this: http://www.webmasterpals.com/showthread.php?t=232
Thanks olddocks, If I set the upload folder 777, what problems could really result? A hacker could read the contents - that's not a problem. He could write jpg, png, jpeg, and gif files to the folder. But, since none of these are executable, he's then stuck and no damage could be done. Am I missing something obvious?
So many problems. They can write a PHP script to read the contents of the other files, hack into the server, etc.. You never need to chmod folders 777, just don't allow write for the public. Peace,
Thanks for that azizy. Yet, I've read elsewhere (Post: Why chmod 777 is NOT a security risk) that the security risks assosiated with chmod 777 are exaggerated. Only if a hacker has already got into your server (I have a dedicated server) could chmod 777 cause a problem:
I didnt test it but I think it could be possible to upload with fake extensions. Better to check both image-header and extension. Look at this uploader: cafewebmaster.com/smart-multi-uploader-and-thumbnail-creator-gif-jpeg-png-php (no urls allowed here)
i do not think that the 777 will cause any problem if u r checking the file type before uploading that means u r not allowing any executable file to upload and i think in ur case u r only allowing the images to upload and any kind of image can not be execute . so u r ok with 777
a malicious code can be embedded into a photo to exectute. Headers or extensions can be faked easily. The best measure would be dont allow any executions inside 777 folder. see this: http://mysql-apache-php.com/fileupload-security.htm Another way around is give ownership to apache to write on the folder. It would work with 775 instead of 777.
plz tell me how one can execute this http://example.com/test.jpg or http://example.com/test.png or http://example.com/test.gif if on the server these file types are not set as executable?
Thanks for your thoughts everybody. Looking at the link olddocks provided, I followed Method 2: chown -R apache uploadsfolder chmod -R 770 uploadsfolder Also, following a little of Method 1, in the uploadsfolder .htaccess I placed as additional security to prevent other files appearing in the folder: <Files ^(*.jpeg|*.jpg|*.png|*.gif)> order deny,allow deny from all </Files> And, to prevent anyone seeing the contents of uploadsfolder and further prevention of script executions: Options -Indexes Options -ExecCGI AddHandler cgi-script .php .php3 .php4 .phtml .pl .py .jsp .asp .htm .shtml .sh .cgi Doing all this has made access to that folder a lot more difficult. In fact, I need to use puTTY now myself to get into it and see what's there! Again, I'd be grateful to hear everyone's thoughts on these security measures.
True! Just use getimagesize() function. If it fails dont allow to upload. You can get the width, height, picture type as well with this function.