16

I've got HTML form for editing images. All data is stored in JSON. When I change current image, I want to save changes, through PHP script, to a text file. If I return to previous image, this configuration will be send again from this file to the form.

My question is:

How to write/read this kind of data safely. Where and how effectively check data to prevent some JS/PHP code injections?

I have attached some concept code below:

JavaScript (using jQuery):

// Writing
$.ajax({
    global: false,
    type: "POST",
    cache: false,
    dataType: "json",
    data: ({
        action: 'write',
        config: JavaScriptJSON_Obj
    }),
    url: 'read-write.php'
});

// Reading
$.ajax({
    global: false,
    type: "POST",
    cache: false,
    dataType: "json",
    data: ({
        action: 'read'
    }),
    url: 'read-write.php',
    success: function(data){
        JavaScriptJSON_Obj = data;
    }
});

PHP example (read-write.php):

switch ($_REQUEST['action']) {
    case 'write':
        file_put_contents('config.txt', $_REQUEST['config']);
        break;
    case 'read':
        $s = file_get_contents('config.txt');
        echo json_encode($s);
        break;
}
Max Barnas
  • 309
  • 1
  • 2
  • 13
  • Depends on what is being done with the **JavascriptJSON_Obj** I would say. It could open the door for potential remote file includes. – inquam May 31 '11 at 11:57
  • 1
    Just save it as textual data and then load it as textual data. There's nothing unsafe. The only thing you should consider - access rights. You should store this information in a directory that is not directly accessible via browser (that's why you have `read-write.php` for *reading* `config.txt`). You might also consider storing this data in DB. – binaryLV May 31 '11 at 11:59

3 Answers3

7

The problem with your code is that it won't work, security issues aside. You must either serialize the data, or encode it to json BEFORE storing it in a file, ie. like this:

switch ($_REQUEST['action']) {
    case 'write':
        file_put_contents('config.txt', json_encode($_REQUEST['config']));
        break;
    case 'read':
        readfile('config.txt');
        break;
}

Serialising works like this:

switch ($_REQUEST['action']) {
    case 'write':
        file_put_contents('config.txt', serialize($_REQUEST['config']));
        break;
    case 'read':
        $data = unserialize(file_get_contents('config.txt'));
        echo json_encode($data);
        break;
}

As long as you make sure that the path you read/write to is correct, there are no code injection problems with this code. The only potential problem is if you can choose what file to use (rather than hardcode "config.txt" into the code). Then you'd have to validate to make sure the file is in a given directory etc.

Jakob Egger
  • 11,981
  • 4
  • 38
  • 48
  • 1
    @Jacob, thank you for pointing this. Serialization is, in my case, not needed. I will store only simple data like text and numbers. – Max Barnas May 31 '11 at 12:19
  • Oh, then I misunderstood. I thought `JavaScriptJSON_Obj` was a placeholder for an arbitrary javascript object or array. – Jakob Egger May 31 '11 at 12:22
  • Well, in fact, under my JS script, `JavaScriptJSON_Obj` is an object, but only for convenience. – Max Barnas May 31 '11 at 12:25
6

First of all: JSON is not JavaScript and vice versa. And JSON is even not a proper subset of JavaScript.

Besides that, since you neither interpret some user input as PHP nor some output as JavaScript, there is no need to worry. But don’t forget to specify your output properly:

header('Content-Type: application/json;charset=utf-8');
$s = file_get_contents('config.txt');
echo json_encode($s);
Gumbo
  • 643,351
  • 109
  • 780
  • 844
  • 4
    I've never said that JSON is JavaScript. I know the difference. It's almost like saying that PHP is an array :) – Max Barnas May 31 '11 at 12:04
  • 1
    Well, in some cases it might be potentially unsafe. E.g., data might be stored into a file named `$_SERVER['DOCUMENT_ROOT'] . '/data.php'`, which might be executed by accessing `http://example.com/data.php`. Though, user would have to know the name of a file and the file would have to be *executable by server* (.txt files usually are not *executable*). – binaryLV May 31 '11 at 12:07
0

I would always check the data returned to see if it is in a format I expect. Say you are saving an image... Check it using MIME checks etc. to make sure that it is an image. If you just save data as is on the server you could open the door for some potential security issues.

If you mean that you just save data about which images was viewed it could still pose a problem depending on how and where that data is accessed and used. So if you except an integer and nothing more, make sure that the data you receive and save is an integer and nothing more.

inquam
  • 12,664
  • 15
  • 61
  • 101
  • As far as I know, checking MIME is as unsafe as not checking it at all. If you want to ensure that "image" really IS an image, you should check data's structure, not some textual information that represents filename's extension. – binaryLV May 31 '11 at 12:11
  • No, of course... The MIME is just a first step check. If that is off then it's OBVIOUSLY not an image. But then you could try to actually load the image to make sure it's a valid image. – inquam May 31 '11 at 12:14
  • The OP is trying to store JSON data, not images or integers. – Jakob Egger May 31 '11 at 12:16
  • `All data is stored in JSON`, the format of the actual data within the JSON container might still be important and regarded when it comes to safety. – inquam May 31 '11 at 12:17
  • 1
    Is it actually possible to store a binary image inside a JSON object? – Jakob Egger May 31 '11 at 12:24
  • Well yes, you could `BASE64` encode it and several other methods is mentioned here: http://stackoverflow.com/questions/1443158/binary-data-in-json-string-something-better-than-base64 – inquam May 31 '11 at 12:28
  • @Jakob Egger, any kind of binary data can be stored as a string. Reading image pixel-by-pixel (which would probably be necessary for getting image's data into a variable) *might* be possible, though I'm not 100% sure about it (should take a closer look at `` tag and other "new technologies" to give exact answer). – binaryLV May 31 '11 at 12:30