6
\$\begingroup\$

I've made a tiny PHP script that sits on my server and receives images/files and text/urls I send to it from a desktop application. I'm a bit worried about how secure the whole thing is because the upload endpoint is the same URL as users who view the content I've uploaded go to.

This script takes uploaded things and stores them in the database under a unique, random, 5-character alphanumeric string. Text and URLs are stored verbatim (albeit mysqli_escape_string'd) in the database while files (this includes images) are stored in a subfolder named "upload" that the webserver user has 0755 access to. URLs are location redirected, text is displayed raw, and files are fpassthrough()ed when a user requests an existing 5-character string.

The database credentials ($db_*) and upload control variables ($upload_*) are in uploader_settings.inc.php (which is not included here).

index.php:

<?php
/**
 * Make sure the upload directory is chmod'ed to 755
 *
 */
require("./uploader_settings.inc.php");

$conn = mysqli_connect($db_url,$db_user,$db_pass,$db_database);
if (mysqli_connect_errno()) {
    http_response_code(500); //server error
    die();
}
//handle content
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    //all required data?
    if (!isset($_POST['type']) || !isset($_POST['value']) || !isset($_POST['pass'])) {
        http_response_code(400); //bad request
        echo "Malformed request!";
        die();
    }
    //correct password?
    if ($_POST['pass'] != $upload_pass) {
        http_response_code(401); //not authorized
        echo "Missing auth!";
        die();
    }
    //actually do it
    $value =  $_POST['value'];
    $random = random_str(5);
    do {
        $query = mysqli_query($conn,"SELECT * FROM ".$db_table." WHERE shortened = '".$random."';");
        if (mysqli_num_rows($query) == 0) {
            break;
        } else {
            $random = random_str(5);
        }
    } while (true);
    switch ($_POST['type']) {
        case "url":
        case "text":
            mysqli_query($conn,"INSERT INTO ".$db_table." (shortened,type,value) VALUES ('".$random."','".$_POST['type']."','".mysqli_real_escape_string($conn,$value)."');");
            break;
        case "file":
            $file = $_FILES['fileToUpload'];
            $newfile = "./upload/".$random.".".mysqli_real_escape_string($conn,pathinfo($file['name'],PATHINFO_EXTENSION));
            if ($file['error'] == UPLOAD_ERR_OK) {
                if (!move_uploaded_file($file['tmp_name'], $newfile)) {
                    http_response_code(500);
                    echo "File error!";
                    die();
                }
                mysqli_query($conn, "INSERT INTO " . $db_table . " (shortened,type,value) VALUES ('" . $random . "','file','" . $newfile . "');");
            } else {
                http_response_code(500);
                echo "Upload error!";
                die();
            }
            break;
        default:
            http_response_code(500);
            echo "Invalid type";
            mysqli_close($conn);
            die();
    }
    http_response_code(200);
    echo $upload_returnURL.$random;
    mysqli_close($conn);
    die();
} else { //getting a URL
    $query = mysqli_query($conn,"SELECT * FROM ".$db_table." WHERE shortened = '".mysqli_real_escape_string($conn,$_GET['selector'])."';");
    mysqli_close($conn);
    if (mysqli_num_rows($query) > 0) {
        $array = mysqli_fetch_array($query);
        $value = $array['value'];
        switch ($array['type']) {
            case "url":
                header("Location: ".$value);
                die();
            case "text":
                echo $value;
                die();
            case "file":
                header("Content-Type: ".mime_content_type($value));
                fpassthru(fopen($value,"rb"));
                die();
        }
    } else {
        header("Location: ".$upload_failRedirect);
        die();
    }
}

function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    $str = '';
    $max = mb_strlen($keyspace, '8bit') - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}

And should it matter, .htaccess:

RewriteEngine on
#all files and directories
RewriteCond %{REQUEST_FILENAME} !-f [OR]
RewriteCond %{REQUEST_FILENAME} !-d
#pass to index as perameter
RewriteRule ^(.*)$ index.php?selector=$1 [NC,L,QSA]
\$\endgroup\$
4
\$\begingroup\$

Pay close attention to the advice from @janos regarding your SQL injection vulnerability. I agree with that assessment and so won't he-hash it here.


First, I would consider splitting the upload/record creation endpoint (POST use case) apart from the view/retrieval endpoint (GET use case). Why introduce the complexity of managing both of these endpoints within a single script? There is basically nothing to lose into splitting these endpoints, and a lot to gain in terms of simplifying your code.

A rule of thumb you can think about is that the more indented code you have in a script (via conditionals, loops, switches), the more likely you are to have bugs. So in this case why deal with two wholly different codepaths with different expected results within one script?


I think you have already introduced bad behavior into this script by having the whole GET use case executed within an unconditional else block. You just assume that if someone arrives at this script while not using POST action, that the request is fine and you go right into querying against the database. There could be a whole lot of things wrong about that request that you should validate before making the relatively expensive database call. What if the request is not GET at all? What if selector input parameter is not set, or is not within expected range of values?


In POST path, there should be no reason to try a select against the DB to see if the short code already exists. If you have this table properly implemented with a unique index on the short code field, there is no reason to pre-select. You could go directly to insert and check for duplicate index violation error and retry with new code if this fails. This makes it to where happy path use case no longer takes two queries against the DB, it would only take one.


I am concerned with the whole approach in comparing $_POST['pass'] against some stored password variable (I am assuming this is defined in the include file). Does this mean you have a single password that, if compromised, gives any user of the system the ability to modify any record? This seems like a poor security strategy, not to mention the apparent plain text nature of this as already noted. You should like have a separate user authentication/authorization mechanism rather having a single password passed at the time of especially since I am seeing nothing in your .htaccess or code (maybe this is in include) that is forcing HTTPS protocol to be used.


Though the code is not shown, here, you did comment that that your DB credentials a re set in the include file. You should strongly consider moving away from a paradigm where credentials of any kind are stored in your code. Ideally these are passed from environmental configuration. Additionally, why would these value be in variables that are able to be changed at run time anywhere in your code?


In POST path, I think you should validate "type" value early in code path like you have done with other validations. If an invalid value is set, you should use 400 error, not 500 as is currently given.


Why are you using mysqli escape on file name when forming file path?


You have a potential directory traversal vulnerability when creating your file path, as you are doing nothing to validate the path file name input. What happens if file name has ../../ or similar in name?


You have both CSRF and XSS vulnerabilities in your code. You should be using form tokens as mitigation against CSRF (this also means you need to be using sessions). And you need to consider cleansing text input before insert (for example failing if you detect <script> tags or other XSS vectors), and at point of rendering (where you are also not even rendering proper HTML entities.).


I am guessing the GET path doesn't actually work as you are closing your DB connection before retrieving rows!


In your type="test" GET use case, you do not explicitly set http response headers. This is actually probably not a problem, but this is not consistent with your usage in POST path.


I am not sure why you are using multibyte function in your random string generation function when you are not working with a multibyte "keyspace".


In general, you should get in the habit of using exact comparisons (===, !==) instead of loose comparisions, unless there is some specific reason you need to perform a loose comparison. This will make your code more resilient against unexpected truthy/falsey evaluations. That time you spend in typing the extra characters is negligible in comparison to the time you will spent debugging a codebase full of loose comparisons.


You should consider moving away from SELECT * queries. These are oftentimes wasteful of bandwidth and application memory as most practical cases don't require all columns to be retirieved (so why retrieve them). For example, you GET path has you sending back the short code in your query when that is the known value you are using for lookup.

One might also find that * selects can make their code fragile to database schema changes.

Finally, you code is much more readable when you see the exact fields that you are working with. One doesn't need to know the DB schema to understand what the result set looks like.


Why is your random string function defined in this script instead of in the include? It is good to get in the habit of separating your PHP code into files based on whether they represent application logic (i.e. classes, functions, interfaces, etc.) or whether they actually cause changes in the state of the application or render output. These leads to better code organization.


Your application should probobaly only have 600 permissions (or possibly 644 if other system users need to read to files) in the upload file directory. I assume none of these files are going to be executed (thus needing 7).


Your code is very happy path oriented. You just assume your code is going to work as expected. For example, you assume that all database queries are going to work and have no handling of potentially failing queries.


You have basically no validation of input parameters (outside of their mere existence). This combined with your happy path only code, means your whole script is very fragile to having unexpected behaviors and security vulnerabilities.

\$\endgroup\$
4
\$\begingroup\$

Use prepared statements

This, and other SQL statements in your code, is an SQL injection waiting to happen:

mysqli_query($conn,"INSERT INTO ".$db_table." (shortened,type,value) VALUES ('".$random."','".$_POST['type']."','".mysqli_real_escape_string($conn,$value)."');");

Don't construct SQL by embedding user input into a string. Use prepared statements.

Don't store clear passwords

This suggests that you have somewhere a password stored in clear text:

if ($_POST['pass'] != $upload_pass) {

It's better to store passwords in a hashed form, so that they cannot be stolen too easily.

\$\endgroup\$

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.