# [RFC] Don't automatically unserialize Phar metadata outside getMetadata()

- Thread: https://externals.io/message/110871
- Original archive: https://news-web.php.net/php.internals/110871
- Started by tyson andre on 2020-07-08T00:06:51+00:00
- Messages: 2
- RFC: Don't automatically unserialize Phar metadata outside getMetadata() (Implemented (PHP 8.0)) — https://externals.io/rfc/phar_stop_autoloading_metadata.md
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #110871 — tyson andre — 2020-07-08T00:06:51+00:00

URL: https://externals.io/message/110871#110871

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

---

## #111034 — tyson andre — 2020-07-16T04:13:50+00:00

URL: https://externals.io/message/110871#111034 · In reply to: #110871

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