Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security tightening: verify a stream file name is a string before unlinking #48

Merged

Conversation

weierophinney
Copy link
Member

@weierophinney weierophinney commented Jan 5, 2021

Per CVE-2021-3007 (and as reported on Bleeping Computer there is a possibility IF A USER HAS USED UNSERIALIZE() ON UNTRUSTED DATA of the stream response destructor potentially invoking a class __toString() implementation, and thus triggering a vulnerability.

This patch ensures that given that scenario, the stream response destructor does not use an object as a string for purposes of unlinking a potential stream filename.


Additional information

The Laminas security team was contacted on 2020-11-27 about a potential vulnerability in the MVC skeleton. After analysis, we responded on 2020-11-30 with the following:

On review, we feel this is not a vulnerability specific to the framework, but rather
more generally to the language. The un/serialize() functions have a long history
of vulnerabilities (please see https://www.google.com/search?q=php+unserialize+RCE
for examples), and developers should NEVER use it on untrusted input. If this is
impossible, they should at the very least pass the second `$options` argument,
and provide a list of allowed classes, or use the argument to disallow all
unserialization of objects (see https://www.php.net/unserialize for details).

We also received the report you provided against Zend Framework. That project
is no longer active, and any security issues are now resolved in the Laminas
Project (which will require users migrate to Laminas from ZF). Our findings remain
the same for that project, however; this is a PHP language issue, and not specific
to our project.

The Open Web Application Security Project (OWASP) has a classification for this sort of vulnerability: PHP Object Injection. It is not specific to any given framework, and presents itself when an application blindly unserializes user input that includes classes with __destruct() methods and/or methods that might get called within the application context (including other magic methods such as __toString()). The same vulnerability could have been achieved even easier by providing a serialized class with a __destruct() method defined, as the method would be called as soon as the object was out of scope.

Regardless, we are providing this patch to help further protect our users from these scenarios. The patch provides type checking of the $streamName property before performing a cleanup operation (which results in an unlink() operation, which, previously, could have resulted in an implied call to an an object's __toString() method) in the Laminas\Http\Response\Stream destructor.

Per https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3007 there is a possibility IF A USER HAS USED UNSERIALIZE() ON UNTRUSTED DATA of the stream response destructor potentially invoking a class `__toString()` implementation, and thus triggering a vulnerability.

This patch ensures that given that scenario, the stream response destructor does not use an object as a string for purposes of unlinking a potential stream filename.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Copy link
Member

@Ocramius Ocramius left a comment

LGTM. Also, since the upstream issue is not really a security issue with the library, I'd be fine with merging this for the next minor, rather than backporting.

boesing
boesing approved these changes Jan 5, 2021
@weierophinney weierophinney merged commit 019f31f into laminas:2.14.x Jan 5, 2021
2 checks passed
@weierophinney weierophinney deleted the security/stream-destruction branch Jan 5, 2021
xmorave2 added a commit to moravianlibrary/CPK that referenced this issue Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants