[RFC] Don't automatically unserialize Phar metadata outside getMetadata() View as Markdown

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 in phar.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 of unserialize())
• 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, where file_exists("phar://$untrusted") can lead to a call to unserialize(). (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