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
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 tounserialize()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
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
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 tounserialize()
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?
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.
The vote has passed with 25 yes votes and 0 no votes.
Thanks,
- Tyson