Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:111034
To: "internals@lists.php.net" <internals@lists.php.net>
CC: Bishop Bettini <bishop@php.net>, Stanislav Malyshev <smalyshev@gmail.com>
Thread-Topic: [RFC] Don't automatically unserialize Phar metadata outside
 getMetadata()
Thread-Index: AQHWVLp90COYmNzkwki/UeUuwn48GKkJnjCq
Date: Thu, 16 Jul 2020 04:13:50 +0000
Message-ID:
 <DM6PR07MB661860DF93DCF017448762B3F97F0@DM6PR07MB6618.namprd07.prod.outlook.com>
References:
 <DM6PR07MB661806D69A310B38A3E999C4F9660@DM6PR07MB6618.namprd07.prod.outlook.com>
In-Reply-To:
 <DM6PR07MB661806D69A310B38A3E999C4F9660@DM6PR07MB6618.namprd07.prod.outlook.com>
Accept-Language: en-CA, en-US
Content-Language: en-CA
x-ms-exchange-messagesentrepresentingtype: 1
x-tmn: [YMedhRlYfnwB/vG8uTi/dpqdm6Wrjw6Y+cfXCFqPrUAFHT9YrvNVDSdDoYDwkdKG]
x-ms-publictraffictype: Email
x-incomingheadercount: 45
x-eopattributedmessage: 0
x-ms-office365-filtering-correlation-id: 90dddea1-6ec9-4505-e759-08d8293ea4e9
x-ms-traffictypediagnostic: CY1NAM02HT028:
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info:
 X5pZalgv8K0VYfZ8RT3HW8/peR9fOkdiJ9kwXRT/qOv6EILVDtJCF6+ZH+JCbqT40iZgIySUc9NEfKcualnebDEjnUnhO2bWpE6gqI2D6eIN8xx0Z9RE1I1bCqrHk6VLRbTnJhMhi1OFFgCQ+jqg6BGWBg6bqHTq0TRysZm83XQokK5LyD218zeLqdYKhTWuUgpIQwk/1C9i0Cnp0ke1OhgwznCW4OE3+sCLLzevpK42RU6b5M6+PtOQYIqMa2A4
x-forefront-antispam-report:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:0;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR07MB6618.namprd07.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:;DIR:OUT;SFP:1901;
x-ms-exchange-antispam-messagedata:
 G71qEndImSe+77sf650eB5R/6OVQBMA32H3MS6xPFBdqMi9vjh+XMOsBN10E53xfrhyJWBPBWYCQZr5L439SgB1CSnVZ03uv01LRrcnJWBkRFXVLk/YUzyDFRgQ6WjIs9rfEsoIVPmvd+zrpZLCO3wj0I548Pn7ZurK4zfVCV6t3sEr9haL7RglWVdxX6Ke691u2RZU5afT1QlfJ06fJYw==
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: hotmail.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-AuthSource: CY1NAM02FT022.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: 90dddea1-6ec9-4505-e759-08d8293ea4e9
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jul 2020 04:13:50.4109
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Internet
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1NAM02HT028
Subject: Re: [RFC] Don't automatically unserialize Phar metadata outside
 getMetadata()
From: tysonandre775@hotmail.com (tyson andre)

Hi internals,=0A=
=0A=
I plan to start the vote for https://wiki.php.net/rfc/phar_stop_autoloading=
_metadata on 2020-07-21, in 6 days.=0A=
=0A=
I've created an implementation of this RFC that passes existing phar test c=
ases ( https://github.com/php/php-src/pull/5855 ).=0A=
If anyone has additional test cases, patches, fixes, or improvements (or bu=
g reports for this PR), I'd love to see them.=0A=
=0A=
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 me=
tadata **only** if Phar->getMetadata() or PharFile->getMetadata() is called=
 directly. (as described in https://bugs.php.net/bug.php?id=3D76774)=0A=
=0A=
- I plan to add more `ZEND_ASSERT` assertions that persistent phars added i=
n `phar.cache_list` don't have temporary zvals (e.g. objects) created in a =
place where permanent zvals were expected. (probably by avoiding storing an=
y zvals)=0A=
  (and/or stop storing the results of unserialize())=0A=
- I plan to look into early returns if serialize()/unserialize() calls thro=
w a Throwable. This should not affect stream wrappers, only explicit uses o=
f metadata from Phar or PharFile objects.=0A=
- 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/getMe=
tadata/delMetadata,=0A=
  which should have less security impact than prior to this RFC, where `fil=
e_exists("phar://$untrusted")` can lead to a call to unserialize().  (see t=
he RFC for security concerns of phar stream wrappers )=0A=
- I'd expect that any unanticipated issues could be solved by the first rel=
ease candidate is released=0A=
=0A=
> I've created https://wiki.php.net/rfc/phar_stop_autoloading_metadata as m=
entioned earlier in https://externals.io/message/110856=0A=
> =0A=
> This aims to add the mitigations described in https://externals.io/messag=
e/105271#105291 , which seemed to be the most straightforward approach to a=
voiding unexpected side effects of unserialization.=0A=
> - For a trusted phar, I wouldn't expect to need to unserialize metadata t=
o check for the file not being corrupt (e.g. there's a checksum, and people=
 would have tested the phar manually).=0A=
> - For an untrusted phar, I'd want php to avoid calling unserialize() when=
 reading it.=0A=
> =0A=
> https://bugs.php.net/bug.php?id=3D76774 goes into more detail about the s=
ecurity issues this aims to fix. =0A=
=0A=
Thanks,=0A=
- Tyson=0A=