Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105300
MIME-Version: 1.0
References: <CAJRMtAG41c7tSDRO0H-pz+R=qxDva5h+7Q7mdkAUObN6fC+TPg@mail.gmail.com>
 <2e70065b-160b-7134-03ee-1a6025a6467c@gmail.com> <CAJRMtAEZugqtpSRE9nKUd9=QtS8wt9RzxBsnj00aHW9DxXD2gQ@mail.gmail.com>
 <92b1b774-7c4b-555d-671e-d58c1054bd14@gmail.com> <CAGa2bXZkLTkTRnBCL+0eesv_WJ+TtzGpXMB5mc+jLH661U+WWg@mail.gmail.com>
 <CAEYWF=5K1WWAqAwPR0sS_ge2bj_o=8dhhnN6WApYvsQDfNW3vw@mail.gmail.com>
In-Reply-To: <CAEYWF=5K1WWAqAwPR0sS_ge2bj_o=8dhhnN6WApYvsQDfNW3vw@mail.gmail.com>
Date: Wed, 17 Apr 2019 10:14:24 +0900
Message-ID: <CAGa2bXZ6etdbFWbzJxCFakiDpDYO9Nxh=Pbm85OxY4mhUxQ3zw@mail.gmail.com>
To: Bishop Bettini <bishop@php.net>
Cc: Stanislav Malyshev <smalyshev@gmail.com>, Raymond Irving <xwisdom@gmail.com>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000004bc6200586af9d1f"
Subject: Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for
 security researchers
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--0000000000004bc6200586af9d1f
Content-Type: text/plain; charset="UTF-8"

On Tue, Apr 16, 2019 at 10:55 PM Bishop Bettini <bishop@php.net> wrote:

> On Tue, Apr 16, 2019 at 6:38 AM Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>
>> On Mon, Apr 15, 2019 at 3:28 PM Stanislav Malyshev <smalyshev@gmail.com>
>> wrote:
>>
>> > Hi!
>> >
>> > > Thanks for responding to this issue.
>> > >
>> > > Will calling getMetaData still parse and
>> > > execute malicious code?
>> >
>> > If it's contained in phar and serialized data and the surrounding code
>> > (I understand that most techniques mentioned in the article rely on
>> > certain vulnerable code being present) then yes.
>> >
>>
>> This issue was discussed in this list before.
>> As long as PHP calls unserialize for phar metadata, object injection is
>> possible
>> which may allow malicious code execution.
>>
>> https://github.com/php/php-src/blob/master/ext/phar/phar.c#L607
>>
>> I'm not sure if Phar metadata requires object or not.
>> If not, Phar may use JSON. Or we may add safer unserialize that ignores
>> object
>> and reference for maximum compatibility.
>>
>> Something has to be done, since we wouldn't fix memory issue(s) in
>> unserialization.
>>
>
> Phar itself does not require object metadata, but users may have objects
> in their phars' metadata. Using the argument from BC, we can't remove
> object support. That said, I'd suggest we go about this in two phases:
>
> 1. Immediate mitigation. Invoke phar_parse_metadata only when explicitly
> called for, via getMetadata. I believe hasMetadata and delMetadata do not
> need to unserialize to answer their functions. This is, as I understand it,
> Stas' suggestion.
>
> 2. Improve caller control on unserialization. Change the signature to
> public Phar::getMetadata ( mixed $allowed_classes = true ) : mixed, and
> invoke the behavior similar to how unserialize itself works. Since all of
> this problem stems from the use of untrusted content on the phar:// stream,
> we should put into the caller's hands as much control as possible.
>
> If the above is reasonable, I'll create tickets for the work and put it up
> at the top of my queue (right behind finishing the phar fuzzing PR).
>

Sounds good to me.

Currently, it seems phar unserialize metadata always
by phar_parse_pharfile()
https://github.com/php/php-src/blob/master/ext/phar/phar.c#L664

This behavior is not nice.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--0000000000004bc6200586af9d1f--