Hi internals,
https://bugs.php.net/bug.php?id=76774 has been open since 2018-08-21.
That ticket proposes the following:
I propose that we disable the ability to have concrete types included in the serialized metadata by
providing an empty classlist to the unserialize call in the PHAR package.
This will support the real cases we see in the wild of metadata usage which is only array key values.
A major change such as PHP 8.0 seems like a good time to disable this.
(but it seems safe enough for any minor version)
Various blog posts have been written explaining the resulting vulnerabilities,
such as https://www.ixiacom.com/company/blog/exploiting-php-phar-deserialization-vulnerabilities-part-1
This change was previously proposed in https://externals.io/message/105271#105303
Bishop Bettini wrote,
I agree that $allowed_classes is a partial fix.
But is it not better to incrementally add defensive layers?I'll get to the immediate mitigation after I finish my phar fuzzing work,
unless somebody beats me to it.
I'm in favor of adding the defensive layer, and could probably implement the immediate mitigation if needed.
Thoughts on whether this needs an RFC? Has anything changed since that email thread? There seemed to be some debate over implementation details, but most responses considered the existing unserialization behavior problematic.
- If it did, this may need to start less than two weeks after finishing an RFC, due to the feature freeze in august 8th.
I assume the limitation of not allowing any objects (i.e. $allowed_classes=[]) for metadata would consistently affect getMetadata() and anything using the phar stream wrapper for a phar file.
Emitting an E_WARNING may be helpful but not absolutely necessary if an object is seen anywhere in the data passed to Phar->setMetadata().
Thanks,
- Tyson
Hi!
https://bugs.php.net/bug.php?id=76774 has been open since 2018-08-21.
That ticket proposes the following:
I propose that we disable the ability to have concrete types included in the serialized metadata by
providing an empty classlist to the unserialize call in the PHAR package.
This will support the real cases we see in the wild of metadata usage which is only array key values.
I think it's a good idea. I am a little bit worried that somebody
somewhere may be using phars with object data, and it'd be nice to
create some way for such people to still use these phars, but by default
I think no complex objects is a good idea.
--
Stas Malyshev
smalyshev@gmail.com
On Mon, Jul 6, 2020 at 8:58 PM tyson andre tysonandre775@hotmail.com
wrote:
Hi internals,
https://bugs.php.net/bug.php?id=76774 has been open since 2018-08-21.
That ticket proposes the following:
I propose that we disable the ability to have concrete types included in
the serialized metadata by
providing an empty classlist to the unserialize call in the PHAR package.
This will support the real cases we see in the wild of metadata usage
which is only array key values.A major change such as PHP 8.0 seems like a good time to disable this.
(but it seems safe enough for any minor version)Various blog posts have been written explaining the resulting
vulnerabilities,
such as
https://www.ixiacom.com/company/blog/exploiting-php-phar-deserialization-vulnerabilities-part-1This change was previously proposed in
https://externals.io/message/105271#105303Bishop Bettini wrote,
I agree that $allowed_classes is a partial fix.
But is it not better to incrementally add defensive layers?I'll get to the immediate mitigation after I finish my phar fuzzing work,
unless somebody beats me to it.I'm in favor of adding the defensive layer, and could probably implement
the immediate mitigation if needed.Thoughts on whether this needs an RFC? Has anything changed since that
email thread? There seemed to be some debate over implementation details,
but most responses considered the existing unserialization behavior
problematic.
- If it did, this may need to start less than two weeks after finishing an
RFC, due to the feature freeze in august 8th.
Thanks for raising this.
I agree that 8.0 is a great time to fix it. While I see this as a design
oversight rather than a new feature - and don't think it requires an RFC -
the RFC provides welcome visibility into the pros and cons. So, +1.
I've been working on an IMAP issue for some time now1, and Phar work has
been sidelined as a result. Fortunately, the fix is straightforward and I'm
happy to work together to land this before feature freeze.