Closed rickmacgillis closed 6 years ago
I believe json_decode()
is safe against object injection attack. It can only create stdClass object without methods.
The note in docs seems to be not accurate.
I agree, it seems this can be removed.
No, it can not. This got there because the issue was proven and it can not be prevented.
The problem is not the json itself, but the fact that it can instantiate existing classes. Misuse requires access to these classes, and knowledge of their internals. The example used one of image classes to delete files (due to an issue in the constructor of that class that has since been fixed).
I agree not to remove the note. But it is not accurate.
JSON and serialized arrays could contain objects.
"serialized arrays", why array? We should say "serialized string"?
Since their constructor will execute upon instantiation during auto formatting
As I said, json_decode()
only instantiate stdClass object which has no constructor or other methods. So JSON is safe.
And unserialize()
never execute object constructor, but execute __wakeup()
. And if the object has magic method like __destruct()
or __string()
, then the method would execute automatically. At least __destruct()
will definitely execute when php shutdown. And not only magic methods but also other methods might be misused, if could.
Although your reasoning sounds ok, it's not.
I have seen and tested the exploit code myself. It did instantiate the class who's name was passed in the response, and the response was crafted so that it could delete any arbitary file (due to specific code in the class constructor that was exploited).
As to your remark, it's specifically about a serialzed array containing a serialized object of a class that is loadable by the target app, and the request specifies conversion of that serialized array to json, which triggers the load and instantiation of the class.
No, not directly.
The issue is about a Fuel app doing a Request_Curl to a malicious REST service, and you have enabled auto formatting of the response (which since detection of this exploit has been off by default).
I had originally written that Controller_Rest should be able to disable auto-formatting, but what I had forgotten was that we still use \Input to catch the data flying our way, so the conversion is application specific.
However, my message was a bit premature, and what I was really looking at was when the data we have gets posted to a third-party, we perform the conversions at that time. Most applications that won't matter for since they're relying on known data getting posted off somewhere, but with Bit API Hub, we act as an API proxy, so the data has to get checked before it winds up in a body getting sent off to the API provider's system.
For those who come across this post, remember to check that the data you're sending to the remote server via the body of your call, is not serialized data. The conversions could leave you just as vulnerable as when you receive auto-formatted data from the remote server.
Check out: https://gist.github.com/cs278/217091
How the exploit works
Alright, so I looked a bit more into the situation just now and I can tell how it's pulled off. (Thanks to @WanWizard for a bit of help in understanding it a bit better.) These steps specify the workflow as it pertains to the attack from the remote "API."
Howerver, I still couldn't get anything to run the constructor. @WanWizard what was the serialized code used in the exploit? I want to make sure that the manual formatting I'm using is sufficient.
My test data The code below is what I'm calling with cURL to make the exploit work. I haven't been able to get it working.
<?PHP
header('Content-Type: application/vnd.php.serialized');
echo 'a:1:{i:0;O:4:"test":0:{}}';
Class Test: (Resides on the client)
<?php
class test
{
public function __construct()
{
\Debug::dump('Wreak havoc');
}
}
Code used to make the call:
$curltest = \Request::forge('http://example.com/text.php', 'curl');
$curltest->set_auto_format(true);
$response = $curltest->execute()->response()->body();
$serialized = \Format::forge($response)->to_serialized();
$json = \Format::forge($response)->to_json();
The docs state that this flaw pertains to both JSON, and serialize(), but it seems to only effect the serialize() function when generating the serialized data for the exploit. (new \Test) Hopefully you still have the code from the original security report.
@WanWizard
As to your remark, it's specifically about a serialzed array containing a serialized object of a class that is loadable by the target app, and the request specifies conversion of that serialized array to json
The above procedure that @CozyLife wrote is exaclty what you say?
I don't want to go into it too much, as it is an existing vulnerability, and there might be pre-1.7 applications out there that are unpatched or unfixed. This issue was reported by Masaki Chida (GREE, Inc.), including code to reproduce it.
It makes use of a combination of constructor (to setup the de-serialized object with the correct information, in case of the example with a filename), and a destructor (that deletes the file).
So the vulnerability is a combination of the ability to craft a serialized response that would instantiate objects of classes defined in the app using crafted data, and using the destructor of that class to manipulate data in the app (in this case delete any arbitrary file that was passed). It requires detailed knowledge of the class source in your application.
Something like this will do it:
class SomeClass
{
protected $file;
public function __construct($file)
{
$this->$file = $file;
}
public function __destruct()
{
unlink ($this->file);
}
}
unserialize()
doesn't call constructor.
In your example SomeClass
, __construct()
is not needed for the attack.
Attacker could inject object (serialize response) with arbitrary $file
property, and __destruct()
will remove arbitrary file.
We go back the first question.
I can't understand why the docs state that the auto_format issue pertains to JSON at all.
You are correct, the serialized object should have the property already set.
It has to do with any auto formatting that will instantiate the object, so I think it applies to both JSON and array's, so the format types 'json' and 'serialize'. I don't think the 'xml', 'php' and 'csv' formats will instantiate the object, but that has to be tested.
To all who come
Alright, I have a few comments. First, JSON can only store StdObj objects, so if the remote server speaks in JSON, then you decode it, the only object that it can possibly decode to is a StdObj. Therefore, there can't possibly be a danger when converting from a real JSON string. That's not to say that someone won't be sneaky and try to spit out a serialized string with a non-serialized Content-Type header.
XML can store objects, but can it store a PHP class object? I saw that \Format checks if an entry is an object, but not an instance of \SimpleXMLElement. That's the only part that makes me nervous in the conversion process.
CSV doesn't seem to have any semblance of a way to set off PHP classes, and neither does YAML including Spyc. (However, someone might add a bunch of _ YAML_LiteralBlock entries to the YAML just to mess things up.)
TL;DR
\Format::forge($serialized_data, 'json')->to_array();
) Trying to convert from a bad format will either throw an exception, return empty data, or it'll give you back your serialized string in one format or another.Test Case
Be sure to test your script's manual conversion method. Set up a dummy script to access via cURL as usual. Add the following class to your APPPATH/classes directory.
someclass.php
<?php
class SomeClass
{
protected $file = 'test';
public function __destruct()
{
echo $this->file;
}
}
Create a file and store it in a location that's web accessible to your script so that you can call it with Request_Curl.
malicious.php
<?PHP
header('Content-Type: application/vnd.php.serialized');
echo 'O:9:"SomeClass":1:{s:5:"*file";s:4:"test";}';
When you run the call, if you see the word "test" on your screen, then you know you're vulnerable. If you don't see it on your screen, try \Debug::dump('test');
or exit;
in the destructor.
I've been looking for some kind of exploit to test CVE-2014-1999 with JSON objects. I don't think it's possible for JSON to even store class methods. Numerous sources including the PHP manual state that json_encode() and json_decode() are safe for untrusted data. Therefore, I can't understand why the docs state that the auto_format issue pertains to JSON at all.