Basic PHP Cloud script

Question

I would like to add basic Cloud system to my website project. I have my php login system already included. Every person must be logged in to view this site. There are several session variables like $_SESSION["id"].

I've got my files.php file - main file with list of uploaded files on server, form to upload new and delete. File data to remove/add is sent to PHP script by POST + hidden inputs keeping user id.

I've found some basic PHP functions that are successfully connected to my website and working.

uploadscript.php (w3schools)

<?php
$target_dir = "files/" . $_POST["id"] . "/";
$target_file = $target_dir . basename($_FILES["fileToUpload"]["name"]);
$uploadOk = 1;
$imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
// Check if image file is a actual image or fake image
if(isset($_POST["submit"])) {
    $check = getimagesize($_FILES["fileToUpload"]["tmp_name"]);
    if($check !== false) {
        echo "File is an image - " . $check["mime"] . ".";
        $uploadOk = 1;
    } else {
        $uploadOk = 0;
    }
}
// Check if file already exists
if (file_exists($target_file)) {
    header('Location: files.php?alert=4;');
    $uploadOk = 0;
}
// Check file size
if ($_FILES["fileToUpload"]["size"] > 500000) {
    header('Location: files.php?alert=5;');
    $uploadOk = 0;
}
// Allow certain file formats
if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg"
&& $imageFileType != "gif" ) {
    header('Location: files.php?alert=3;');
    $uploadOk = 0;
}
// Check if $uploadOk is set to 0 by an error
if ($uploadOk == 0) {
    echo "Sorry, your file was not uploaded.";
// if everything is ok, try to upload file
} else {
    if (move_uploaded_file($_FILES["fileToUpload"]["tmp_name"], $target_file)) {
        header('Location: files.php?alert=1;');
    } else {
        header('Location: files.php?alert=2;');
    }
}
?>

downloadscript.php (php.net/manual/en/function.readfile.php)

<?php
$file = "files/" . $_POST["id"] . "/" . $_POST["file"];

if (file_exists($file)) {
    header('Content-Description: File Transfer');
    header('Content-Type: application/octet-stream');
    header('Content-Disposition: attachment; filename="'.basename($file).'"');
    header('Expires: 0');
    header('Cache-Control: must-revalidate');
    header('Pragma: public');
    header('Content-Length: ' . filesize($file));
    readfile($file);
    exit;
}
?>

deletescript.php (php.net/manual/en/function.unlink.php)

<?php
$file = "files/" . $_POST["id"] . "/" . $_POST["file"];

if (file_exists($file)) {
    unlink($file);
    header('Location: files.php?alert=6;');
    exit;
}
?>

Are these scripts secure? Do you suggest any modifications to it to prevent some unexpected and unwanted actions by users?

Answer 1

Are these scripts secure?

They are not. All three scripts are vulnerable to directory traversal.

This is what an attacker can achieve with each script:

• uploadscript.php: upload files with certain extensions to arbitrary directories, for example to deface the main site. This isn't that serious, but should be fixed.
• downloadscript.php: download arbitrary files. This is serious, and needs to be fixed. An attacker can read any files the webserver has access to, thus revealing sensitive information.
• deletescript.php: delete arbitrary files in arbitrary directories. This may possibly be used to bypass authentication in your application (eg by deleting something like authentication.php), or to make your application unusable (DOS).

You need to sanitize your inputs (basename for filenames, .. filter for directories, although you could filter stricter), and you need to check if the normalized filename + path is inside the directory you want it to be in.

Note also that even if you fix this, as ids are passed from the user, users will still be able to upload files to other users directory, read files of other users, and delete files of other users.

Note also that getimagesize is nice as defense in depth, but can easily be bypassed. I would remove the comment as to not leave a false sense of security (it's mainly a usability check).

Misc

• Comments that just repeat what the code does are usually frowned upon, as they bloat your code and make it harder to notice relevant comments. If you want to structure your code, use functions (eg isValidFilesize, isValidFileExtension, etc).
• 500000 could actually either use an inline comment, or be put into a properly named variable.
• I would get rid of the uploadOk flag, as it isn't all that nice to read. You also perform a header redirect for many of the cases anyways, so normal users will not see the echo.
• don't mix snake_case and camelCase without a good reason.
• imageFileType doesn't hold the image file type, but the file extension.

Answer 2

To extend upon the nice review by @tim ...

All input received in:

$_POST, $_GET, $_REQUEST, $_COOKIE, $_FILES, $_SERVER (portions of it)

Should be considered as potentially harmful user input to be validated accordingly before doing ANYTHING with it in your application. These sorts of validations are simply missing from your code altogether, making your could extremely vulnerable to a number of attack vectors, the most glaring of which is

---

Consider enforcing file size upload limits through PHP INI settings either in server configuration (ideally), or at script runtime (if for some reason your limit needs to to change for a certain script). Ideally, you want to block a request with oversize payload as early as possible in the request handling process (this ideally to have it enforced at a web server level is ideal).

---

I do not like the approach of always redirecting the user to another page even when there are error conditions, giving 300 series HTTP redirection response instead of a more appropriate HTTP 400 or 500 series response. In either, case obfuscating the code read from an understanding of what the actual end user messaging mechanism is, also seems a little odd. How is one reading this page to know what different alert id values are?

---

// Check if image file is a actual image or fake image
if(isset($_POST["submit"])) {
    $check = getimagesize($_FILES["fileToUpload"]["tmp_name"]);
    if($check !== false) {
        echo "File is an image - " . $check["mime"] . ".";
        $uploadOk = 1;
    } else {
        $uploadOk = 0;
    }
}
// Check if file already exists
if (file_exists($target_file)) {
    header('Location: files.php?alert=4;');
    $uploadOk = 0;
}
// Check file size
if ($_FILES["fileToUpload"]["size"] > 500000) {
    header('Location: files.php?alert=5;');
    $uploadOk = 0;
}
// Allow certain file formats
if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg"
&& $imageFileType != "gif" ) {
    header('Location: files.php?alert=3;');
    $uploadOk = 0;
}
// Check if $uploadOk is set to 0 by an error
if ($uploadOk == 0) {
    echo "Sorry, your file was not uploaded.";
// if everything is ok, try to upload file

You need to explicitly exit execution once failure condition is met, and typically anytime after a redirect header is sent. This section of code is is actually VERY problematic. One could meet all of the failure criteria. One could actually fail every one of these tests, yet code execution would continue through all additional tests until the message is echoed out, with the location header being overwritten every time. So, the end effect is that if more than one condition fails, the end user will be redirected with only the last alert id value set.

Your code should be more like:

// Check if file already exists
if (file_exists($target_file)) {
    header('Location: files.php?alert=4;');
    exit();
}
// Check file size
if ($_FILES["fileToUpload"]["size"] > 500000) {
    header('Location: files.php?alert=5;');
    exit();
}
// Allow certain file formats
if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg"
&& $imageFileType != "gif" ) {
    header('Location: files.php?alert=3;');
    exit();
}

Of course this eliminates the echo altogether. But this makes sense if you are committing to the pattern of having all end user messaging delivered on files page based on alert value. Why would you need messaging here anyway?