Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105291
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>
In-Reply-To: <CAGa2bXZkLTkTRnBCL+0eesv_WJ+TtzGpXMB5mc+jLH661U+WWg@mail.gmail.com>
Reply-To: bishop@php.net
Date: Tue, 16 Apr 2019 09:55:14 -0400
Message-ID: <CAEYWF=5K1WWAqAwPR0sS_ge2bj_o=8dhhnN6WApYvsQDfNW3vw@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>, Stanislav Malyshev <smalyshev@gmail.com>
Cc: Raymond Irving <xwisdom@gmail.com>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000bb1f180586a61f4d"
Subject: Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for
 security researchers
From: bishop@php.net (Bishop Bettini)

--000000000000bb1f180586a61f4d
Content-Type: text/plain; charset="UTF-8"

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).

bishop

--000000000000bb1f180586a61f4d--