Export/Import from security perspective

Question

Hi have two working scripts, one to backup a db field and the other one to restore that same db field. What i would like to know if from security perspective they are safe or what can be improved. Working on a Joomla 3.x system plugin.

Export script

function export()
{
    // Get a database object.   
    $db = JFactory::getDbo();

    // Create a new query object.
    $query = $db->getQuery(true);

    // Select fields to get.
    $fields = array(
        $db->quoteName('params')
    );

    // Conditions for which records should be get.
    $conditions = array(
        $db->quoteName('element') . ' = ' . $db->quote('plugin_name'), 
        $db->quoteName('folder') . ' = ' . $db->quote('system')
    );

    // Set the query and load the result.
    $query->select($fields)->from($db->quoteName('#__extensions'))->where($conditions);
    $db->setQuery($query);   
    $results = $db->loadResult(); 

    // Namming the filename that will be generated.
    $name      = 'file_name';
    $date      = date("Ymd");
    $json_name = $name."-".$date;

    // Clean the output buffer.
    ob_start();
    echo $results;
    $fileContent = ob_get_contents();
    ob_end_clean();

    header('Content-disposition: attachment; filename='.$json_name.'.json');
    header('Content-type: application/json');

    echo $fileContent;
    exit();
}

Import script

function import($_FILES)
{   
    if ($_FILES['fileToUpload']['error'] > 0) {
        echo "<p class='alert alert-error'>An error occurred while uploading!</p>";     
    } else {
        $file_name = $_FILES['fileToUpload']['name'];
        $file_ext  = strtolower(end(explode(".", $file_name)));
        $file_size = $_FILES['fileToUpload']['size'];
        if (($file_ext == "json") && ($file_size < 50000)) {
            $options = file_get_contents($_FILES['fileToUpload']['tmp_name']);

            // Get a database object. 
            $db = JFactory::getDbo();

            // Create a new query object.
            $query = $db->getQuery(true);

            // Fields to update.
            $fields = array(
                $db->quoteName('params') . ' = ' . $db->quote($options)
            );

            // Conditions for which records should be updated.
            $conditions = array(
                $db->quoteName('element') . ' = ' . $db->quote('plugin_name'), 
                $db->quoteName('folder') . ' = ' . $db->quote('system')
            );

            // Set the query and execute it.
            $query->update($db->quoteName('#__extensions'))->set($fields)->where($conditions);
            $db->setQuery($query);
            $result = $db->execute();

            echo "<p class='alert alert-success'>Backup file successfully restored!</p>";
        } else {
            if ($file_ext != "json") {
                echo "<p class='alert alert-error'>Invalid file type!";
            } elseif ($file_size > 50000) {
                echo "<p class='alert alert-error'>File size error!</p>";
            }
        }
    }
    return;
}

Answer 1

First thing I would do is use JInput instead of using $_FILES, like so:

$jinput = JFactory::getApplication()->input;
$files  = $input->files->get('jform1');

function import($files)
{
     // rest of code here
}

So rather than becoming some long winded array, it will become much more simplified like so:

Array
(
    [test] => Array
        (
            [0] => Array
                (
                    [name] => file.png
                    [type] => image/png
                    [tmp_name] => /tmp/phpXoIpSD
                    [error] => 0
                    [size] => 34409
                )

            [1] => Array
                (
                    [name] => file2.jpg
                    [type] => image/jpeg
                    [tmp_name] => /tmp/phpWDE7ye
                    [error] => 0
                    [size] => 99529
                )

        )

)

There are some other things like using Joomla's API instead of header(), but I wouldn't say that's necessary.

One thing you could do for additional security is get the user object and check the permissions when calling the function. To properly check for a super user, have a look at the following answers by dev-m and Bakual

https://joomla.stackexchange.com/a/1154/168

Note: Don't use the code from my answer as the others are better methods. I've tried to delete mine but it won't let me.

Answer 2

Regarding your export script, I don't see any glaring security issues. However, I don't see any authorization checks either. It looks like you are just printing the json to screen. Perhaps you'd like to include a feature to save the json to a forced-download file.

You might gain some inspiration from this module with functional similarities:

How to create a custom script to force the download of a .csv file that is generated in real time?

As for the importation, I have stronger concern.

You are checking that the filename ends with .json or .jSoN or JSON, but you aren't actually sniffing to see if it is actually a .json file or actually contains json data. For the record, I find this file extension extracting technique to be elegant:

strtolower(pathinfo($filename, PATHINFO_EXTENSION))

But Joomla has some available techniques that will serve you very well:

https://docs.joomla.org/Secure_coding_guidelines#File_uploads

And here's something worth reading: Fastest way to check if a string is JSON in PHP?

There is a lot of debate on that page, but if you are expecting iterable json-encoded data, you should probably try to decode it and check what results.

Furthermore, if your project is expecting certain keys or values or key-value pairs in the incoming data, you should apply a whitelist check in your validation process. If you know that the expected values never contain certain characters, apply that restriction.

The importation step is the most dangerous process for your system. Evil genius hackers salivate at the chance to manipulate such an access point. You need to make the import security layer as hard as your "good users" can tolerate. Deny people who are not logged in. Use a token to prevent CSRF attacks.

Validate and sanitize the data to within an inch of its life ...because failing to do so can lead to loss of time, money, credibility, hair, sleep, etc. Best of luck.