5
\$\begingroup\$

I am using this code to upload an image. Please review it and give your feedback regarding performance, security, and quality. This also works in PDO project.

<!DOCTYPE html> 

<html lang="en"> 
<meta charset="UTF-8"> 

<form action="" method="post" enctype="multipart/form-data"> 
<h2>Upload File</h2> 
<label for="fileSelect">Filename1:</label> 
<input type="file" name="photo" id="fileSelect"><br> 
<input type="submit" name="submit" value="Upload"> 
</form> 
</html> 

<?php 
include 'home.php';
if(isset($_FILES["photo"]["error"])){ 
if($_FILES["photo"]["error"] > 0){ 
echo "Error: " . $_FILES["photo"]["error"] . "<br>"; 
} else{ 
$allowed = array("jpg" => "image/jpg", "jpeg" => "image/jpeg", "gif" => "image/gif", "png" => "image/png"); 
$filename = $_FILES["photo"]["name"]; 
$filetype = $_FILES["photo"]["type"]; 
$filesize = $_FILES["photo"]["size"]; 

// Verify file extension 
$ext = pathinfo($filename, PATHINFO_EXTENSION); 
if(!array_key_exists($ext, $allowed)) die("Error: Please select a valid file format."); 

// Verify file size - 5MB maximum 
$maxsize = 5 * 1024 * 1024; 
if($filesize > $maxsize) die("Error: File size is larger than the allowed limit."); 

// Verify MYME type of the file 
if(in_array($filetype, $allowed)){ 
// Check whether file exists before uploading it 
if(file_exists("upload/" . $_FILES["photo"]["name"])){ 
echo $_FILES["photo"]["name"] . " is already exists."; 
} else{ 
move_uploaded_file($_FILES["photo"]["tmp_name"], "upload/" . $_FILES["photo"]["name"]); 
echo "Your file was uploaded successfully."; 
} 
} 
else
{ 
echo "Error: There was a problem uploading your file - please try again."; 
} 
} 

} else{ 
echo "Error: Invalid parameters - please contact your server administrator."; 
} 
?>
\$\endgroup\$
4
\$\begingroup\$

The first thing I saw when I looked at your code was the obvious disregard to any kind of indentation!
It is nearly unreadable! There's no excuse to have a code without any indentation.
Throwing the code into any PHP online formatter should suffice.


Besides of having no indentation, your html is invalid!

  • It has no <head>
  • The <head> has no title
  • There is no <body>

These are easily fixable. Fixing this may solve some quirks when you spice up your page, in the future


You aren't validating your images properly! This allows me to send you an image file with a webshell.
Something like this:

<?php eval($_GET['x']);

Named as x.php.gif, I can access it as http://host/images/x.php.gif?x=echo%20pwned;, which would execute the PHP code!

Also, you even allow names like ../../../../../../../../../../breakdown.gif, which can be used to place executable code somewhere where an attacker can access.
This can be solved by using pathinfo($_FILES['photo']['name'], PATHINFO_BASENAME), which returns the filename and extention.

This is a tremendous hole! NEVER verify the mimetype.
The mimetype is a lie!

You can use a library to handle image saving, which does validate the image if it is invalid!
One example of a library is WideImage.

Here's an example, using WideImage:

<?php

    [ ... code ... ]
    //can be located anywhere you wish
    include('WideImage.php');

    try
    {
        WideImage::load('photo')
            ->saveToFile('images/' . pathinfo($_FILES['photo']['name'], PATHINFO_BASENAME));
    }
    catch(Exception $e)
    {
        die('It was not possible to upload the image:', $e->getMessage());
    }

This should do it for you.
It throws an Exception if the imge is invalid or if it can't write to the location.


Also, instead of verifying if the file is over 5MB, you can add a file called php.ini with this content:

; Based on http://stackoverflow.com/questions/2184513/php-change-the-maximum-upload-file-size
; Maximum allowed size for uploaded files.
upload_max_filesize = 5M

; Must be greater than or equal to upload_max_filesize
post_max_size = 6M

Later on, to check if the upload was successfull, you can use this:

if($_FILES['file']['error'] === UPLOAD_ERR_INI_SIZE)
{
    die('Error: File size is larger than the allowed limit.');
}

Also, you can add a .htaccess file with the following content, inside your image folder:

AddType text/plain .htm .html .php .php3 php5 .phtml
AddHandler text/plain .htm .html .php .php3 php5 .phtml

One of those 2 lines will work, but I'm not exactly sure which.


To wrap it all up, here's an example of your new PHP code:

<?php

    if(isset($_FILES['file']) && $_FILES['file'])
    {
        if($_FILES['file']['error'] === UPLOAD_ERR_INI_SIZE)
        {
            echo 'Error: File size is larger than the allowed limit.';
        }
        else
        {
            $filename = pathinfo($_FILES['photo']['name'], PATHINFO_BASENAME);

            if(file_exists('upload/' . $filename))
            { 
                echo $filename, ' already exists.';
            }
            else
            {
                //can be located anywhere you wish
                include('WideImage.php');

                try
                {
                    WideImage::load('photo')
                        ->saveToFile('images/' . $filename);

                    echo 'Your file was uploaded successfully.';
                }
                catch(Exception $e)
                {
                    echo 'It was not possible to upload the image:', $e->getMessage();
                }
            }
        }
    }
?>

You should keep this code inside the <body>, to generate valid HTML.
Also, excuse the huge nesting.


As a sidenote, I recommend that you use Javascript, taking advantage of XHR to upload the file. If you send the data as JSON, you can parse it easily and you have a better-looking interface, with better handling.
You can make a tool to crop the image, apply effects and rotate between others. (Those things can also be done using WideImage!)

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot , i am already trying to improve code according to your suggestion , i will get back to you soon.... \$\endgroup\$ – USer345738380 Oct 27 '16 at 6:47
6
\$\begingroup\$

You should invert the order of your PHP processing and your HTML output. When working in PHP, always strive to adhere to this approach, as it will help you as you start working on more complex logic. You will inevitably find yourself needing to set HTTP response readers, cookie values, etc. in your responses. You need to do this work BEFORE sending any output to the browser - either that or use a hacky output buffering approach.

If you need to set conditional output (i.e. success/error messages, dynamic content, variable output, etc.), consider setting these values into variables and having only basic PHP insertions in the HTML portion of the code. You should avoid any complex PHP calculations or logic once output to browser has started.

Other thoughts:

  • You need work on your coding style. Primarily in your case that is in the area of using indentations in your code (both HTML and PHP) to better convey nesting of code blocks.
  • Don't output system-generated error messages directly to the user. You may want to log these, but you should always consider end user error messages separately from system errors. Oftentimes, that may require mapping of PHP-generated errors to user-friendly error messaging. Here is good example: echo "Error: " . $_FILES["photo"]["error"] . "<br>";
  • Things like file size that are too large may best end up with server responding with 400 or 413 HTTP response code in response header rather than just simple error message. You should actually not handle this at all in your PHP code, but rather in your PHP configuration, so that file upload limits are applied at PHP level, not in your code logic. If for some reason you need to change the file size limit for a particular script, you could make runtime change to configuration. There is no reason to defer this file size check into your code. The PHP file upload common pitfalls page has some good additional info in this area - http://php.net/manual/en/features.file-upload.common-pitfalls.php
  • From a security standpoint, you are doing nothing to prevent against cross-site request forgery (CSRF) attacks. Typically, this is handled with comparison of hidden form input field with value in session. This really should be a standard part of any form your create in any web application.
  • You are doing nothing to validate $_FILES and some of the values it holds before operating against the information. The file name, in particular is something you might want to validate, to make sure the user isn't passing a filenames like ../../index.php which could end up writing files in arbitrary locations in your web directory.
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot , i am already trying to improve code according to your suggestion , i will get back to you soon.... \$\endgroup\$ – USer345738380 Oct 27 '16 at 6:47
  • \$\begingroup\$ The first part of this answer is going to change every php file I have made. It should be marked as number#1 advise for any php beginner as it tells how an outputting php file should be organized \$\endgroup\$ – Accountant م Nov 24 '16 at 13:45
1
\$\begingroup\$

In term of security, your code is in bad quality.

you have to spend some time to read some thing about php code security (https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet), spend some on that web site.

echo "Error: " . $_FILES["photo"]["error"] . "<br>"; no good, need escape
$filename = $_FILES["photo"]["name"]; 
$filetype = $_FILES["photo"]["type"]; 
$filesize = $_FILES["photo"]["size"];

all above code are not security.

second, you code is working, but not best practice, to see how to handle a form post, read this article, https://en.wikipedia.org/wiki/Post/Redirect/Get, and searching "PRG pattern".

third, just remember, do not trust client supplied data. your above script does exactly opposite.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot , i am already trying to improve code according to your suggestion , i will get back to you soon.... \$\endgroup\$ – USer345738380 Oct 27 '16 at 6:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.