Hi internals,
I've created https://wiki.php.net/rfc/phar_stop_autoloading_metadata as mentioned earlier in https://externals.io/message/110856
This aims to add 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.
https://bugs.php.net/bug.php?id=76774 goes into more detail about the security issues this aims to fix.
Thanks,
- Tyson
Hi internals,
I plan to start the vote for https://wiki.php.net/rfc/phar_stop_autoloading_metadata on 2020-07-21, in 6 days.
I've created an implementation of this RFC that passes existing phar test cases ( https://github.com/php/php-src/pull/5855 ).
If anyone has additional test cases, patches, fixes, or improvements (or bug reports for this PR), I'd love to see them.
This RFC proposes to not unserialize the metadata automatically when a phar is opened by php (Previously, it did). It will make PHP unserialize the metadata only if Phar->getMetadata() or PharFile->getMetadata() is called directly. (as described in https://bugs.php.net/bug.php?id=76774)
- I plan to add more
ZEND_ASSERT
assertions that persistent phars added inphar.cache_list
don't have temporary zvals (e.g. objects) created in a place where permanent zvals were expected. (probably by avoiding storing any zvals)
(and/or stop storing the results ofunserialize()
) - I plan to look into early returns if
serialize()
/unserialize() calls throw a Throwable. This should not affect stream wrappers, only explicit uses of metadata from Phar or PharFile objects. - If any unexpected issues do get introduced here, I'd anticipate they'd be limited to explicit calls from PHP to Phar or PharFile's setMetadata/getMetadata/delMetadata,
which should have less security impact than prior to this RFC, wherefile_exists("phar://$untrusted")
can lead to a call tounserialize()
. (see the RFC for security concerns of phar stream wrappers ) - I'd expect that any unanticipated issues could be solved by the first release candidate is released
I've created https://wiki.php.net/rfc/phar_stop_autoloading_metadata as mentioned earlier in https://externals.io/message/110856
This aims to add 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.https://bugs.php.net/bug.php?id=76774 goes into more detail about the security issues this aims to fix.
Thanks,
- Tyson