froala / wysiwyg-editor-php-sdk

PHP SDK to ease the integration of Froala WYSIWYG Editor on server side.
https://www.froala.com/wysiwyg-editor
MIT License
42 stars 26 forks source link

Even examples shouldn't have massive security holes. #14

Open Danack opened 7 years ago

Danack commented 7 years ago

The example code given for the PHP documentation has a massive security hole.

$response = FroalaEditor_File::delete($_POST['src']);

public static function delete($src) {

    $filePath = $_SERVER['DOCUMENT_ROOT'] . $src;
    // Check if file exists.
    if (file_exists($filePath)) {
      // Delete file.
      return unlink($filePath);
    }

    return true;
  }

That code allows anyone who knows what the delete URL is, to delete any file off the server, that PHP has permissions to delete.

This is sub-optimal.

Even though it is just example code, there should be some example lines that check that the file being deleted is under the appropriate directory, and probably also a note that there should be a permissions check to ensure the user is allowed to delete images.

NunoLopesPT commented 7 years ago

Hi @Danack , I don't think the verifications should happen on the DiskManagement Class, so I updated the examples as you can see:

<?php

require __DIR__ . '/vendor/froala/wysiwyg-editor-php-sdk/lib/FroalaEditor.php';

try {
    $src = $_POST['src'];
    $upload_folder = "/uploads/";

    // If the file is inside the uploads folder
    if (substr($src, 0, strlen($upload_folder)) === $upload_folder)
    {
        $response = FroalaEditor_File::delete($src);
        echo stripslashes(json_encode('Success'));
    }
    else
    {
        echo stripslashes(json_encode('Error'));
    }
} catch (Exception $e) {
    echo $e->getMessage();
    http_response_code(404);
}

Regarding the permissions any idea to approach this? It may depend on the developers choice to do this

shreypasari-accolite commented 5 years ago

Hi @stefanneculai This is an example and this security hole can be taken care by a programmer because it depends where he wants to upload his files/images, so can he only put a check on it that the user is not trying to delete anything from somewhere else. Thank You

stefanneculai commented 5 years ago

@shreypasari-accolite can we update the examples with some code to avoid that? Thanks in advance.

shreypasari-accolite commented 5 years ago

@stefanneculai Raised PR for the mentioned thing in the examples. Thank You

stefanneculai commented 5 years ago

It's updated now on https://github.com/froala/editor-php-sdk-example, we'll update shortly on the website too.