Shardj / zf1-future

PHP 8.1 compatible version of ZF1
BSD 3-Clause "New" or "Revised" License
440 stars 190 forks source link

Dangerous usage of unserialize (CVE-2020-29312) #341

Open Judx opened 1 year ago

Judx commented 1 year ago

What are the security implications of CVE-2020-29312 for ZF1-Future and are there any potential security updates or patches that have been released to address the issue?

Jimbolino commented 1 year ago

I've looked for the usage of unserialize in the /library/ folder

There are a couple usages, mostly internal for caching:

Function
    unserialize
            library/Zend/Cache  (1 usage found)
            library/Zend/Cache/Backend  (4 usages found)
            library/Zend/Date  (3 usages found)
            library/Zend/Feed  (1 usage found)
            library/Zend/Http  (2 usages found)
            library/Zend/Http/UserAgent  (1 usage found)
            library/Zend/Ldap  (1 usage found)
            library/Zend/Locale  (2 usages found)
            library/Zend/OpenId/Consumer/Storage  (4 usages found)
            library/Zend/OpenId/Provider/Storage  (5 usages found)
            library/Zend/Paginator  (1 usage found)
            library/Zend/Search/Lucene/Index  (1 usage found)
            library/Zend/Serializer/Adapter  (1 usage found)
            library/Zend/Server  (1 usage found)
            library/Zend/Service/WindowsAzure  (2 usages found)
            library/Zend/Stdlib  (2 usages found)

Unless more details are released, the only advise i can give is to not use these classes

Also the exploit appears to be for Zend Framework v.3.1.3 afaik there was never a release like that: https://github.com/zendframework/zendframework/tags

Couldn't find anything about this CVE in https://github.com/laminas either (the successor for Zend Framework v2)

Judx commented 1 year ago

I have not found any information related to the potential impact on ZF1-Future. It is possible that the initial concern was a false alarm, but further investigation may be necessary to confirm this as the base score is over 9.

develart-projects commented 1 year ago

Anyone to take a deeper look at this?

develart-projects commented 1 year ago

Some more details what is this about: https://cwe.mitre.org/data/definitions/502.html

Basically we have to sanitize data after unserialization. Recommendations:

Phases: Architecture and Design; ImplementationIf available, use the signing/sealing features of the programming language to assure that deserialized data has not been tainted. For example, a hash-based message authentication code (HMAC) could be used to ensure that data has not been modified.

Phase: ImplementationWhen deserializing data, populate a new object rather than just deserializing. The result is that the data flows through safe input validation and that the functions are safe.

Phase: ImplementationExplicitly define a final object() to prevent deserialization.

Phases: Architecture and Design; Implementation Make fields transient to protect them from deserialization. An attempt to serialize and then deserialize a class containing transient fields will result in NULLs where the transient data should be. This is an excellent way to prevent time, environment-based, or sensitive variables from being carried over and used improperly.

Phase: ImplementationAvoid having unnecessary types or gadgets available that can be leveraged for malicious ends. This limits the potential for unintended or unauthorized types and gadgets to be leveraged by the attacker. Add only acceptable classes to an allowlist. Note: new gadgets are constantly being discovered, so this alone is not a sufficient mitigation.

develart-projects commented 1 year ago

After brief check, there is shitload of direct usage of unserialize. so have to do this in 2 steps:

1) create internal sanitized unserialize function 2) replace direct userialize PHP function

Jimbolino commented 1 year ago

sanitisation/wrapper function does not solve your issue.

from the manual:

Warning Do not pass untrusted user input to unserialize() regardless of the options value of allowed_classes. Unserialization can result in code being loaded and executed due to object instantiation and autoloading, and a malicious user may be able to exploit this. Use a safe, standard data interchange format such as JSON (via json_decode() and json_encode()) if you need to pass serialized data to the user.

If you need to unserialize externally-stored serialized data, consider using hash_hmac() for data validation. Make sure data is not modified by anyone but you.

If the variable being unserialized is an object, after successfully reconstructing the object PHP will automatically attempt to call the __unserialize() or __wakeup() methods (if one exists).

so best to use json_encode and json_decode, instead of serialize and unserialize

develart-projects commented 1 year ago

@Jimbolino I don't think anyone will ever rewrite all the classes to use JSON, that's simply too much. We can rather think about to use internal function as I wrote and if not sanitizing, then including hash as stated in the manual.

Jimbolino commented 1 year ago

i only counted +- 30 usages of unserialize, it shouldn't be too much effort to replace them

rruchte commented 1 year ago

We can't just blindly replace serialize/unserialize with json_encode/json_decode, people actually do make use of the magic methods like sleep and wakeup in sessions, etc. Lots of things would break. It may make sense to do this in places where it would definitely not be a breaking change, but we should step very carefully here.

develart-projects commented 1 year ago

@rruchte I fully agree. And serializing is not the same as storing JSON representation as well. If someone has idea ho to use that recommended _hashhmac method to avoid any data change, I'm voting for this solution.

rruchte commented 1 year ago

There is a commit in the Laminas project that fixes a different CVE that I think is a duplicate (or at least a subset) of this one. Let's cherry-pick that, and possibly call this closed.

rruchte commented 1 year ago

Actually it looks like someone already did this - excellent.

develart-projects commented 1 year ago

@rruchte how is this connected to unserialize vulnerability? I'm kind of missing the point completely tbh :)

rruchte commented 1 year ago

It's sort of an oblique attack:

Laminas Project laminas-http before 2.14.2, and Zend Framework 3.0.0, has a deserialization vulnerability that can lead to remote code execution if the content is controllable, related to the __destruct method of the Zend\Http\Response\Stream class in Stream.php. NOTE: Zend Framework is no longer supported by the maintainer. NOTE: the laminas-http vendor considers this a "vulnerability in the PHP language itself" but has added certain type checking as a way to prevent exploitation in (unrecommended) use cases where attacker-supplied data can be deserialized.

develart-projects commented 1 year ago

@rruchte then this is just the first step, as we have like 30 unserialization occurrences.

rruchte commented 1 year ago

Well, not all of them are going to be exploitable. I don't think it's a reasonable goal to try to remove deserialization from the framework. Really the problem is where you have untrusted content being deserialized. If someone pops your box and can write to your filesystem, you've got bigger problems. My guess is that this is the vuln that CVE-2020-29312 refers to.

develart-projects commented 1 year ago

@rruchte are you competent to tell which is exploitable and which is not? :) I don't feel I'm. As you said, most probably not all of them are, just who can take responsibility and sort them.

rruchte commented 1 year ago

Not really, I mostly build stuff, I don't break stuff. I did see some things in Feed, Mail/Storage/Mbox, and Http/UserAgent.php that looked suspect.

I'm not sure how to get the specifics about the vulnerability that prompted this CVE. Does anyone here have more of an infosec background, and could find out more?

colinmollenhour commented 1 year ago

Anywhere that it is not necessary to support arbitrary object serialization it is best to use the second parameter of unserialize with the allowed_classes option. E.g. passing ['allowed_classes' => false] ensures that no classes can be deserialized. Otherwise, pass a list of allowed classes so all others are excluded. Only if the API allows a user to serialize something that is expected to contain objects of arbitrary types can this option not be used effectively so if it is internal APIs then it should be possible to mitigate this attack with this method.