Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:111121
MIME-Version: 1.0
References: <DM6PR07MB66182B9F1879B64DA95FCFA6F9780@DM6PR07MB6618.namprd07.prod.outlook.com>
In-Reply-To: <DM6PR07MB66182B9F1879B64DA95FCFA6F9780@DM6PR07MB6618.namprd07.prod.outlook.com>
Date: Wed, 22 Jul 2020 16:38:41 +0200
Message-ID: <CAF+90c9TSRSu=xkdsqfwzwXj6g=Z1tL_XOvijWV=62VYzqG4-Q@mail.gmail.com>
To: tyson andre <tysonandre775@hotmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003160df05ab08b37b"
Subject: Re: [PHP-DEV] [VOTE] Don't automatically unserialize Phar metadata
 outside getMetadata()
From: nikita.ppv@gmail.com (Nikita Popov)

--0000000000003160df05ab08b37b
Content-Type: text/plain; charset="UTF-8"

On Tue, Jul 21, 2020 at 3:37 PM tyson andre <tysonandre775@hotmail.com>
wrote:

> Hi internals,
>
> I've started the vote on
> https://wiki.php.net/rfc/phar_stop_autoloading_metadata
> as announced earlier in https://externals.io/message/110871
> ([RFC] Don't automatically unserialize Phar metadata outside getMetadata())
>
> This adds 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 through stream wrappers.
>
> https://bugs.php.net/bug.php?id=76774 goes into more detail about the
> security issues this aims to fix.
>
> Thanks,
> - Tyson
>

As a minor suggestion:

> 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.

Rather than adding an $allowed_classes parameter, I'd add a general
$unserialize_options parameter that just gets passed through to
unserialize. E.g. we also have a "max_depth" option, which also seems
potentially useful. This will ensure that any new limitations we implement
for unserialize() will also be available in this context.

Nikita

--0000000000003160df05ab08b37b--