| Both sides previous revisionPrevious revisionNext revision | Previous revisionLast revisionBoth sides next revision |
| rfc:phar_stop_autoloading_metadata [2020/07/07 23:58] – tandre | rfc:phar_stop_autoloading_metadata [2020/07/22 17:23] – tandre |
|---|
| ====== PHP RFC: Don't automatically unserialize Phar metadata outside getMetadata() ====== | ====== PHP RFC: Don't automatically unserialize Phar metadata outside getMetadata() ====== |
| * Version: 0.1 | * Version: 0.4 |
| * Date: 2020-07-07 (use today's date here) | * Date: 2020-07-07 |
| * Author: Tyson Andre <tandre@php.net> | * Author: Tyson Andre <tandre@php.net> |
| * Status: Draft | * Status: Voting |
| * First Published at: https://wiki.php.net/rfc/phar.sanitize_object_metadata | * First Published at: https://wiki.php.net/rfc/phar_stop_autoloading_metadata |
| | * Implementation: https://github.com/php/php-src/pull/5855 |
| |
| ===== Introduction ===== | ===== Introduction ===== |
| ===== Proposal ===== | ===== Proposal ===== |
| |
| Don't unserialize the metadata automatically when a phar is opened by php. Only unserialize the metadata if ''%%Phar->getMetadata()%%'' or ''%%PharFile->getMetadata()%%'' is called directly. | Don't unserialize the metadata automatically when a phar file is opened by php. Make PHP unserialize the metadata **only** if ''%%Phar->getMetadata()%%'' or ''%%PharFile->getMetadata()%%'' is called directly. |
| |
| 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. | Additionally, add an ''array $unserialize_options = []'' parameter to both getMetadata() implementations, defaulting to the current default ''unserialize()'' behavior such as allowing any classes. (As an implementation detail, if ''$unserialize_options'' is set to anything other than the default, the resulting metadata won't be cached and this won't return the value from the cache. E.g. ''%%getMetaData(['allowed_classes' => []])%%'' after ''setMetadata(new stdClass())'' will likely trigger a ''%%unserialize(['allowed_classes' => []])%%'' call internally.) |
| |
| This implements one possible solution for https://bugs.php.net/bug.php?id=76774 | This implements one possible solution for https://bugs.php.net/bug.php?id=76774 |
| ===== RFC Impact ===== | ===== RFC Impact ===== |
| ==== To SAPIs ==== | ==== To SAPIs ==== |
| This affects stream wrapper calls such as file_exists(), as well as direct calls to ''%%Phar->getMetadata()%%'' | This affects stream wrapper calls such as ''file_exists()'' (they will no longer call ''unserialize()'' if there is metadata), as well as direct calls to ''%%Phar->getMetadata()%%'' (they will call ''unserialize()'' if there is metadata, instead of using the data from the time the phar file was loaded) |
| Describe the impact to CLI, Development web server, embedded PHP etc. | |
| |
| ==== php.ini Defaults ==== | ===== Vote ===== |
| If there are any php.ini settings then list: | |
| * hardcoded default values: On | |
| * php.ini-development values: On | |
| * php.ini-production values: On | |
| | |
| ===== Proposed Voting Choices ===== | |
| |
| Yes/No, requiring 2/3 majority to stop automatically unserializing metadata. | Yes/No, requiring 2/3 majority to stop automatically unserializing metadata. |
| | Voting started on 2020-07-21 and ends 2020-08-04. |
| | |
| | <doodle title="Stop automatically unserializing Phar metadata outside direct getMetadata() calls" auth="tandre" voteType="single" closed="false"> |
| | * Yes |
| | * No |
| | </doodle> |
| |
| ===== References ===== | ===== References ===== |
| | |
| | https://externals.io/message/110871 '[RFC] Don't automatically unserialize Phar metadata outside getMetadata()' |
| |
| https://externals.io/message/110856 'Including "Disable the ability to use concrete types in PHAR metadata" in PHP 8.0?' | https://externals.io/message/110856 'Including "Disable the ability to use concrete types in PHAR metadata" in PHP 8.0?' |
| https://externals.io/message/105271 'PHP deserialization techniques offer rich pickings for security researchers' | https://externals.io/message/105271 'PHP deserialization techniques offer rich pickings for security researchers' |
| |
| https://bugs.php.net/bug.php?id=76774 | https://bugs.php.net/bug.php?id=76774 'Request #76774 Disable the ability to use concrete types in PHAR metadata' |
| |
| ===== Rejected Features ===== | ===== Rejected Features ===== |
| |
| I'd considered continuing to call unserialize() all of the time to validate phars earlier, but with ''allowed_classes=[]'' instead (controlled by an ini setting). This seemed like an unnecessary ini setting that could be avoided by just not automatically loading the metadata. | I'd considered continuing to call ''unserialize()'' all of the time to validate phars earlier, but with ''allowed_classes=[]'' instead (controlled by an ini setting). This seemed like an unnecessary ini setting that could be avoided by just not automatically loading the metadata. |
| | |
| | ===== Changelog ===== |
| | |
| | 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])%%'' |
| | |
| | 0.3: Clarify wording, add link to RFC announcement thread. Remove inapplicable ini defaults section. |
| | |
| | 0.2: Link to implementation. |