Return to Revisions

Questions

Is this code really secure against uploading of malicious codes ? Anything i have to add to this code to make it more secure?

My initial thought is to ensure that the uploaded file is not an executable file or a file that may be run as a separate script- e.g. PHP or some other language the server may handle. The Mime type checking should handle that for the most part. You could also :

• check the permissions on the file after it is uploaded to ensure anything that shouldn’t be executed cannot be executed - perhaps use is_executable(), fileperms and chmod
• like you say: "move images folder out of public_html (root) folder."- Ensure that any uploaded files are not stored in a web accessible directory unless that is your desire. This may lead to needing a (PHP/other) script or web server module to read files.

Suggestions

avoid excess else

Whenever a conditional block contains a die statement then subsequent code does not need to be prefixed with else e.g.

//check if error is set
if(!isset($_FILES['image']['error']) ){
 echo 'Invalid upload.';
 die();
}
// check formultiple uploads and block
elseif(is_array($_FILES['image']['error'])){
 echo 'Only one file allowed.';
 die();
} 

When else is used it adds more complexity to the decision tree.

Returning early

Going along with the previous point, in some parts of the code when a precondition is not met then an error message is printed to the screen before die() is called. However this is not true in all cases - e.g. the first conditional:

if(isset($_POST['submit'])){

Flipping the logic on that so the error message that is displayed when that condition is false and adding a call to die would allow the indentation levels to be decreased dramatically, which would likely make the code more readable. Further improvement would come from moving some logic to functions - e.g. a function to determine if the file can be uploaded - if not then return early.

With such changes the decision tree of the logic could go from something like this:

enter image description here

To this:

enter image description here

storing max size

 $maximumsize=5000000000;

Since that value shouldn’t be reassigned during the runtime it is advisable to use a constant instead, and a common convention in many programming languages is to use all caps, which is in-line with the PHP Standards recommendation PSR-1 - e.g.

const MAXIMUM_SIZE = 5000000000;

And define those near the top of the code or in a configuration file to allow quick access should you ever need to change the value - that way you won’t need to go hunting through the code to find it.

Array syntax

There isn't anything wrong with using array() but as of the time of writing, PHP has Active support for versions 8.0 and 7.4, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4).

$allowed = array('jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif');

Can be shortened slightly to

$allowed = ['jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif'];

checking mimetype

$filemime could either be string or Boolean - check for Boolean, then if it isn’t then in_array() could be used to determine if it exists in the array instead of using array_search()

Appending to $newfilename

The extension is appended to the file name:

 $newfilename= $newfilename.'.'.array_search($filemime, $allowed);

The dot equals operator can be used to append to the variable:

 $newfilename .= '.' . array_search($filemime, $allowed);

Spaces added for readability.