HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Image, file, text uploader and URL shortener

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fileimageshortenertextuploaderandurl

Problem

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:

 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]

Solution

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.

Code Snippets

mysqli_query($conn,"INSERT INTO ".$db_table." (shortened,type,value) VALUES ('".$random."','".$_POST['type']."','".mysqli_real_escape_string($conn,$value)."');");
if ($_POST['pass'] != $upload_pass) {

Context

StackExchange Code Review Q#157487, answer score: 4

Revisions (0)

No revisions yet.