Closed Firebasky closed 3 years ago
Correct me if I'm wrong, but of course, if you simply use the php unserialize
and decode
methods on the raw $_GET
data, this will work. That does not look like a framework issue but a design error (directly accessing superglobals).
You can easily filter the GET
paramaters with the get...()
methods of the $request
to avoid this:
https://codeigniter4.github.io/userguide/incoming/incomingrequest.html#retrieving-input
However, the utilization chain of deserialization is provided by the framework,$_GET['exp'] Just test the success of the utilization chain。
Where is this utilization chain provided? In the User Guide?
No,The chain is a framework belt. It is implemented through specific attack code. You can debug the code I provide above.
Sorry but I cannot follow here. The test code works because of this line in the controller:
unserialize(base64_decode($_GET['exp']));
As far as I see, this line is the only problem here, because the user data ($_GET
) do not get escaped/filtered.
But lets wait for the maintainers 😄
OK, as I said above, the vulnerability is only the execution chain of PHP deserialization. It must be combined with the unserialize function to achieve the harm of arbitrary code execution。You can see cve-2020-15148 https://www.cvedetails.com/cve/CVE-2020-15148/?q=cve-2020-15148++
Okay, I think I understand what you mean now.
In the issue you linked the topic is about prohibiting an object (BatchQueryObject
) to be unserialized.
What I think you are proposing would require us to disallow unserialize
completely, which is not really feasible. Instead, you should make sure that the $_GET
data are correctly filtered.
Yes, but I think users don't know the need to control unserialize () when using the framework, but they can fix it for the high security and robustness of the framework. I think the fix is just filtering malicious classes and adding __wakeup
method.You can see https://github.com/yiisoft/yii2/commit/9abccb96d7c5ddb569f92d1a748f50ee9b3e2b99 thank you😄
The wakeup() / unserialize() methods are executed in the class in which they are defined. You cannot add wakeup() / unserialize() methods to a serialized class instance.
Sorry, I don't understand what you mean. I mean it can only be added in key classes (such as validgenerator) __wakeup
method to repair, similar to Yii framework.
reference resources:https://github.com/yiisoft/yii2/commit/9abccb96d7c5ddb569f92d1a748f50ee9b3e2b99
Can you give an example of exploiting this "vulnerability"? I will say right away that, in my opinion, the implementation in Yii is meaningless and does not solve anything.
The content I submitted above is the vulnerability I demonstrated. I thought it was very detailed.It is a deserialization vulnerability.The vulnerability is only the execution chain of PHP deserialization. It must cooperate with the serialize function to achieve the harm of arbitrary command execution.
Yes this is a vulnerability, but not of the framework, rather of PHP. The implementation in Yii will not solve oder prevent the issue you demonstrated.
If some methods are used in the framework, such as:
Use in key classes__wakeup
so that key classes cannot be serialized,
And this operation is under the condition of perfect function. This can avoid vulnerabilities.
demo:
public function __wakeup()
{
throw new \BadMethodCallException('Cannot unserialize ' . __CLASS__);
}
😄
Still, if we do this to every class of the framework, your example will be working and open the calculator.
This is not the case. To execute the command, we need to make full use of the chain. We just need to add it at the key points __wakeup
is OK. It's not executing orders.This problem is caused only because a complete utilization chain can be found in the framework.
Adding code to the redishandler class can avoid vulnerabilities, which will be very safe.
public function __wakeup()
{
throw new \BadMethodCallException('Cannot unserialize ' . __CLASS__);
}
Why such difficulties? A simple example that shows how useless "protection" from Yii.
class A
{
public function __wakeup()
{
// some malicious code
}
}
$exp = serialize(new A);
//example.com?exp=$exp
unserialize($_GET['exp']);
No, the deserialization vulnerability is triggered after an object is destroyed__destruct()
then constructs a complete utilization chain to cause rce,If you repair where __destruct()
can be triggered so that this class cannot be deserialized, you can avoid the problem.
The correct fix for this is to search the framework where unserialize
is called in which argument is user-supplied input. If there is none, then this is not a framework-level vulnerability but rather from PHP itself, as it allows object serialization.
Your POC works because you designed it specifically to do so. I can make myself one without using the framework because I am exploiting PHP's vulnerability itself. So I say you search within the framework for unserialize()
calls with user controllable input. If none, then there's nothing to discuss here.
Yes, it is safer to add __wakeup () methods to key classes without affecting functionality.I'm just a suggestion.😄
I'm with @paulbalandan. It is not our job to prevent developers from creating exploitable code just because they happen to be using CodeIgniter. The example in Yii was an issue only because they were handling user input directly - RedisHandler
does not do this, but rather works on classes created by the developers. @Firebasky if you have discovered an actual case of this in the framework please email it to lonnieje@gmail.com, I am closing this thread.
Describe the bug If the framework is used and unserialize () is called at user controllable input and special characters are allowed, it will be subject to deserialization remote command execution vulnerability.
CodeIgniter 4 version Less than or equal to Version v4.1.3
Expected behavior, and steps to reproduce if appropriate Poc:
Test code
The vulnerability is only the execution chain of PHP deserialization. It must cooperate with the serialize function to achieve the harm of arbitrary command execution.