On Tue, Apr 16, 2019 at 10:55 PM Bishop Bettini <bis...@php.net> wrote:
> On Tue, Apr 16, 2019 at 6:38 AM Yasuo Ohgaki <yohg...@ohgaki.net> wrote: > >> On Mon, Apr 15, 2019 at 3:28 PM Stanislav Malyshev <smalys...@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 yohg...@ohgaki.net
