Re: [PHP-DEV] [VOTE] Don't automatically unserialize Phar metadataoutside getMetadata()

This is only part of a thread. view whole thread
  111121
July 22, 2020 14:38 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Jul 21, 2020 at 3:37 PM tyson andre <tysonandre775@hotmail.com>
wrote:

> Hi internals, > > I've started the vote on > https://wiki.php.net/rfc/phar_stop_autoloading_metadata > as announced earlier in https://externals.io/message/110871 > ([RFC] Don't automatically unserialize Phar metadata outside getMetadata()) > > This adds the mitigations described in > https://externals.io/message/105271#105291 , > which seemed to be the most straightforward approach to avoiding > unexpected side effects of unserialization. > > - For a trusted phar, I wouldn't expect to need to unserialize metadata to > check for the file not being corrupt > (e.g. there's a checksum, and people would have tested the phar > manually). > - For an untrusted phar, I'd want php to avoid calling unserialize() when > reading it through stream wrappers. > > https://bugs.php.net/bug.php?id=76774 goes into more detail about the > security issues this aims to fix. > > Thanks, > - Tyson >
As a minor suggestion:
> Additionally, add an $allowed_classes parameter to both getMetadata() implementations, defaulting to the current behavior of allowing any classes
(true). This will be passed to the call to unserialize() performed internally. Rather than adding an $allowed_classes parameter, I'd add a general $unserialize_options parameter that just gets passed through to unserialize. E.g. we also have a "max_depth" option, which also seems potentially useful. This will ensure that any new limitations we implement for unserialize() will also be available in this context. Nikita
  111131
July 22, 2020 16:54 tysonandre775@hotmail.com (tyson andre)
Hi internals,

> As a minor suggestion: > > > Additionally, add an $allowed_classes parameter to both getMetadata() implementations, defaulting to the current behavior of allowing any classes (true). This will be passed to the call to unserialize() performed internally. > > Rather than adding an $allowed_classes parameter, I'd add a general $unserialize_options parameter that just gets passed through to unserialize. E.g.. we also have a "max_depth" option, which also seems potentially useful. This will ensure that any new limitations we implement for unserialize() will also be available in this context.
That sounds like a better idea than what I originally had - I'd forgotten about the max_depth option getting added in php 8.0. Thanks, - Tyson
  111134
July 22, 2020 17:25 tysonandre775@hotmail.com (tyson andre)
Hi internals,

> As a minor suggestion: > > > Additionally, add an $allowed_classes parameter to both getMetadata() implementations, defaulting to the current behavior of allowing any classes (true). This will be passed to the call to unserialize() performed internally. > > Rather than adding an $allowed_classes parameter, I'd add a general $unserialize_options parameter that just gets passed through to unserialize. E.g.. we also have a "max_depth" option, which also seems potentially useful. This will ensure that any new limitations we implement for unserialize() will also be available in this context.
I amended https://wiki.php.net/rfc/phar_stop_autoloading_metadata and changed from version 0.3 to 0.4, with the behavior I plan to implement. I'll aim to have the implementation updated by Friday.
> 0.4: Change from getMetadata($allowed_classes = …) to getMetadata(array $unserialize_options = []) in this document. > I forgot about max_depth being added in php 8.0 and the usefulness of being able to support future options added to unserialize() > without changing the signature of getMetadata. > Elaborate on implementation details $unserialize_options would lead to when setMetaData is called before > $pharFileOrEntry->getMetadata(['allowed_classes' => $classes])
Any other comments/concerns?