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). bishop
