PHP File Upload Script

Question

I've put together an example single file upload script that attempts to cover all the things PHP could check for prior to allowing a successful file upload. Is there anything else maybe now available in PHP 7.4+ I could use to make this more secure? For example, I use filter_input below even though I don't find it in many scripts out there.

Take a look

<?php # EVALUATE REQUEST METHOD $REQUEST_METHOD = filter_input(INPUT_SERVER, 'REQUEST_METHOD', FILTER_SANITIZE_ENCODED); switch ($REQUEST_METHOD) { # HTTP:POST - PAYLOAD:BLOB case 'POST': # POST IMAGE if(\in_array(@$_FILES["files"], $_FILES) && \count($_FILES) === 1) { upload(); } break; default: methodInvalid(); break; } /** * Function upload() uploads a single file. * * */ function upload() { // Establish the upload file directory $upload_dir = $_SERVER['DOCUMENT_ROOT'] . '/gui/v1/uploads/submittals/'; // Establish the upload file path $upload_file = $upload_dir . $_FILES['files']['name'][0]; // Derive the upload file extension $upload_file_extension = strtolower(pathinfo($upload_file, PATHINFO_EXTENSION)); // Allowed file types // $allowed_file_extensions = ['pdf', 'jpg', 'jpeg', 'png', 'gif']; $allowed_file_extensions = ['pdf']; /** * Does tmp file exist? * * */ if (!file_exists($_FILES['files']['tmp_name'][0])) { # ERROR object $errorObject = new stdClass(); $errorObject->apiVersion = '1.0'; $errorObject->context = 'upload.submittal'; # ABOUT ERROR object $aboutError = new stdClass(); $aboutError->code = 'ERR-000'; $aboutError->message = 'Select file to upload.'; # APPEND ABOUT ERROR object TO ERROR object $errorObject->error = $aboutError; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($errorObject)); } /** * Is file extension allowed? * * */ if (!in_array($upload_file_extension, $allowed_file_extensions)) { # ERROR object $errorObject = new stdClass(); $errorObject->apiVersion = '1.0'; $errorObject->context = 'upload.submittal'; # ABOUT ERROR object $aboutError = new stdClass(); $aboutError->code = 'ERR-000'; $aboutError->message = 'Allowed file formats .pdf'; # APPEND ABOUT ERROR object TO ERROR object $errorObject->error = $aboutError; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($errorObject)); } /** * Is file bigger than 20MB? * * */ if ($_FILES['files']['size'][0] > 20000000) { # ERROR object $errorObject = new stdClass(); $errorObject->apiVersion = '1.0'; $errorObject->context = 'upload.submittal'; # ABOUT ERROR object $aboutError = new stdClass(); $aboutError->code = 'ERR-000'; $aboutError->message = 'File is too large. File size should be less than 20 megabytes.'; # APPEND ABOUT ERROR object TO ERROR object $errorObject->error = $aboutError; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($errorObject)); } /** * Does file already exist? * * */ if (file_exists($upload_file)) { /** * File overwritten successfuly! * * */ move_uploaded_file($_FILES['files']['tmp_name'][0], $upload_file); # SUCCESS object $successObject = new stdClass(); $successObject->apiVersion = '1.0'; $successObject->context = 'upload.submittal'; $successObject->status = 'OK'; # UPLOAD SUBMITTAL object $data = new stdClass(); $data->submittalUploaded = true; # APPEND DATA object TO SUCCESS object $successObject->data = $data; # APPEND empty arrays to DATA object $successObject->data->arr1 = []; $successObject->data->arr2 = []; $successObject->data->arr3 = []; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($successObject)); } /** * Can file actually be uploaded? * * */ if (!move_uploaded_file($_FILES['files']['tmp_name'][0], $upload_file)) { /** * File upload error! * * */ # ERROR object $errorObject = new stdClass(); $errorObject->apiVersion = '1.0'; $errorObject->context = 'upload.submittal'; # ABOUT ERROR object $aboutError = new stdClass(); $aboutError->code = 'ERR-000'; $aboutError->message = 'File couldn\'t be uploaded.'; # APPEND ABOUT ERROR object TO ERROR object $errorObject->error = $aboutError; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($errorObject)); } else { /** * File uploaded successfuly! * * */ # SUCCESS object $successObject = new stdClass(); $successObject->apiVersion = '1.0'; $successObject->context = 'upload.submittal'; $successObject->status = 'OK'; # UPLOAD SUBMITTAL object $data = new stdClass(); $data->submittalUploaded = true; # APPEND DATA object TO SUCCESS object $successObject->data = $data; # APPEND empty arrays to DATA object $successObject->data->arr1 = []; $successObject->data->arr2 = []; $successObject->data->arr3 = []; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($successObject)); // We could insert URL file path to a database from here... } } /** * Function methodInvalid() warns about invalid method. * * */ function methodInvalid() { # ERROR object $errorObject = new stdClass(); $errorObject->apiVersion = '1.0'; $errorObject->context = 'uploads'; # ABOUT ERROR object $aboutError = new stdClass(); $aboutError->code = 'ERR-000'; $aboutError->message = 'Invalid Request. Allowed Methods are POST.'; # APPEND ABOUT ERROR object TO ERROR object $errorObject->error = $aboutError; # RETURN JSON RESPONSE header('Content-type:application/json;charset=utf-8'); return print(json_encode($errorObject)); } ?> 

Answer 1

Personally I am more interested in looking at the possible security issues. File uploads are notoriously dangerous if not done right. A vulnerable form could allow an attacker to upload a webshell and take over your server.

Let focus on this line:

// Establish the upload file path $upload_file = $upload_dir . $_FILES['files']['name'][0]; 

You are relying on the original file name, which could be malicious. By definition, it is untrusted third-party input. What would happen if the file name contains stuff like '../../'. The uploaded file could land outside the designated folder. This is a classic path traversal attack.

Or what happens if the file contains spaces, what about "shell.php test.pdf". This file name will match the allowed file extensions but what happens next ? What could possibly go wrong ? Maybe not an immediate vulnerability but your script could easily choke on a malformed file name.

You should never ever blindly reuse the original file name, which anyway can contain special characters or even be too long for your file system. At the very least sanitize it, discard or replace undesirable characters etc.

The absolute minimum you should have done is resolve the canonical path, you have a function like realpath for this purpose.

However the doc says:

move_uploaded_file() is open_basedir aware. However, restrictions are placed only on the to path as to allow the moving of uploaded files in which from may conflict with such restrictions. move_uploaded_file() ensures the safety of this operation by allowing only those files uploaded through PHP to be moved.

---

Regarding the request methods: I am afraid these are moot security considerations.

First of all, the allowed methods should be set in your webserver configuration or virtual host directives. I don't think your script should concern itself with the method (verb). It should simply expect GET or POST requests. If anything else is submitted, your script will not see it and will not process it.

So I would configure the webserver to accept GET, POST, and I would also add HEAD because search engines might use it.

The PHP doc for move_uploaded_file says (emphasis is mine):

move_uploaded_file(string $from, string $to): bool

This function checks to ensure that the file designated by from is a valid upload file (meaning that it was uploaded via PHP's HTTP POST upload mechanism). If the file is valid, it will be moved to the filename given by to.

So that means a file upload will only work if done using POST. The move_uploaded_file function in itself is sufficient to assert that the upload indeed took place using the POST method.

---

What is obvious in your code is the amount of unnecessary repetition. For instance you are testing the file extension, the file size, and also whether the file exists in the upload folder. Then you are repeating the $aboutError stuff in each block, when in fact the only thing that is different is the actual error message relevant to the condition you are testing:

$aboutError->message = 'File is too large. File size should be less than 20 megabytes.'; 

Even the error code is the same everywhere for the moment. The code is bloated, because there is lots of stuff that you could define just once. You can simply redefine the error message in each test with the appropriate message, and leave the rest of the variables unchanged since they have been set previously.

What is clear is that certain variables like API version don't need to be defined multiple times. They should be constants and defined just once at the top of your code.

---

At line 124: you check if the file already exists:

if (file_exists($upload_file)) { 

but you overwrite it anyway, by design it seems. So this block of code is useless, you could as well remove it and let the procedure continue to the next block, where move_uploaded_file is actually run.

And in fact the block starting at line 46 is pointless too:

if (!file_exists($_FILES['files']['tmp_name'][0])) { 

because as previously mentioned, move_uploaded_file can verify that an uploaded file actually exists. By the way a sibling function exists: is_uploaded_file

But you are not even checking that move_uploaded_file returns true - this function returns a boolean value. If it does not return true, something went wrong.

---

I don't understand the point of this code:

# APPEND empty arrays to DATA object $successObject->data->arr1 = []; $successObject->data->arr2 = []; $successObject->data->arr3 = []; 

If it is really useful or meaningful, this is where you should have added some comments to clarify the purpose. I am even wondering if your code actually works in its present form ?

---

The indentation should be streamlined to make the code outline more clear, especially control blocks (ifs or loops).

---

To sum up, I suggest to read the PHP docs and start over with simple examples that you can augment along the way. The bottom line is, don't do useless tests, and don't test for irrelevant conditions.

Answer 2

A few of my thoughts after quick top-to-bottom scan:

Filtering REQUEST_METHOD is absurdly future-proof (you'd have to anticipate an evolution of the code where the literal contents are stored in a database and end up being output without validation to somebodies browser), but somehow I like your mind-set.

The check count($_FILES) === 1 is sort of the opposite. If the front-end evolves into uploading a second file (in a different POST-variable) your code ignores both uploads. It doesn't call methodInvalid nor does it log an error-message.

Later in the code you double-check if PHP has indeed put the temporary file where it says it has put it (that's overkill, but it's better to be overly paranoid than overly relaxed). Yet you don't don't check if $_FILES['files']['name'] contains nasty stuff like "../" which could cause files to be uploaded to places you don't want them to go.

I noticed a lot of magic values which make their appearance in the guts of your code (the extensions you support, the maximum file size, the upload directory, error codes, api versions etc.). Pull them out.

The code has no structure (unless you count "one statement after the other" as a structure). Introducing functions to isolate/encapsulate functional aspects makes the codes much easier to maintain & evolve.

Invoking "new stdclass" and giving it a field "apiVersion" raised an eyebrow and a half. If you're serious about developing an API, introduce some dedicated classes for responses.

In summary: the code has many opportunities for improvements, but I really appreciate your goal of making it as secure as possible. As it stands, it looks pretty solid security-wise. But don't ignore the fact that the current low level of readability makes it hard/too tedious to truly verify if it really is secure.