Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
On Wed, Apr 17, 2019 at 12:44 AM Stanislav Malyshev wrote: > Hi! > > > 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. > > This, unfortunately, is only a partial solution. As long as > serialization format supports references - and they are likely not going > anywhere - it won't be security, it's virtually impossible to secure > code that can modify references while object is being unserialized. At > least without rewriting whole unserialization code. If somebody > undertakes this task, then yes, maybe it can be made secure (not sure > even then). For now, unserializing insecure data is insecure, regardless > of allowed_classes. Allowed_classes is just a barrier to block most > obvious attacks in the wild now, but it does not guarantee safety. > This issue had an extant report, bug # 76774 [1]. To that report, I've added some detail. I agree that $allowed_classes is a partial fix. But is it not better to incrementally add defensive layers? I'll get to the immediate mitigation after I finish my phar fuzzing work, unless somebody beats me to it. bishop [1]: https://bugs.php.net/bug.php?id=76774
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
Hi! > 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. This, unfortunately, is only a partial solution. As long as serialization format supports references - and they are likely not going anywhere - it won't be security, it's virtually impossible to secure code that can modify references while object is being unserialized. At least without rewriting whole unserialization code. If somebody undertakes this task, then yes, maybe it can be made secure (not sure even then). For now, unserializing insecure data is insecure, regardless of allowed_classes. Allowed_classes is just a barrier to block most obvious attacks in the wild now, but it does not guarantee safety. -- Stas Malyshev smalys...@gmail.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
Hi! > 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. Right. That's why I want to make it not unserialize this data unless it's explicitly being requested. > 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. That would break BC with all existing phars that use metadata. -- Stas Malyshev smalys...@gmail.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
On Tue, Apr 16, 2019 at 10:55 PM Bishop Bettini wrote: > On Tue, Apr 16, 2019 at 6:38 AM Yasuo Ohgaki wrote: > >> On Mon, Apr 15, 2019 at 3:28 PM Stanislav Malyshev >> 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
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
On Tue, Apr 16, 2019 at 6:38 AM Yasuo Ohgaki wrote: > On Mon, Apr 15, 2019 at 3:28 PM Stanislav Malyshev > 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
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
On Mon, Apr 15, 2019 at 3:28 PM Stanislav Malyshev 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. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
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. -- Stas Malyshev smalys...@gmail.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
Hi, Thanks for responding to this issue. Will calling getMetaData still parse and execute malicious code? ;__ Raymond On Sun, 14 Apr 2019, 4:47 PM Stanislav Malyshev, wrote: > Hi! > > > I came across this article which highlights a few issues with PHP > > deserialization techniques: > > > > > https://portswigger.net/daily-swig/phar-out-php-deserialization-techniques-offer-rich-pickings-for-security-researchers > > PHP serialization is not meant to be used with external or > user-modifyable data. Looks like the crux of the issue is that phar uses > unserialize() to read metadata, which is an insecure scenario since it > is common to deal with external phar files and it's not obvious there > can be serialized data. Particular Drupal exploit seems to be caused by > insecure coding (one should not be able to give phar streams to system > paths) but the general issue of reading phars being insecure stays. > > It is a bit problematic since there are no limitations in what can be > stored in Phar metadata, so we can't really prohibit anything there > without breaking BC. I would start with banning objects there (at least > by default) but that again would be BC break if somebody did use objects > there. More workable idea would be to not parse metadata unless > getMetadata() is explicitly called. The chance of code that did not > intend to use metadata to use this call is nil, thus eliminating the > deserialization vector in most cases. OTOH, BC is kept since the > metadata is still available for the code that needs it. I'll try to > implement this soon. > -- > Stas Malyshev > smalys...@gmail.com >
Re: [PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
Hi! > I came across this article which highlights a few issues with PHP > deserialization techniques: > > https://portswigger.net/daily-swig/phar-out-php-deserialization-techniques-offer-rich-pickings-for-security-researchers PHP serialization is not meant to be used with external or user-modifyable data. Looks like the crux of the issue is that phar uses unserialize() to read metadata, which is an insecure scenario since it is common to deal with external phar files and it's not obvious there can be serialized data. Particular Drupal exploit seems to be caused by insecure coding (one should not be able to give phar streams to system paths) but the general issue of reading phars being insecure stays. It is a bit problematic since there are no limitations in what can be stored in Phar metadata, so we can't really prohibit anything there without breaking BC. I would start with banning objects there (at least by default) but that again would be BC break if somebody did use objects there. More workable idea would be to not parse metadata unless getMetadata() is explicitly called. The chance of code that did not intend to use metadata to use this call is nil, thus eliminating the deserialization vector in most cases. OTOH, BC is kept since the metadata is still available for the code that needs it. I'll try to implement this soon. -- Stas Malyshev smalys...@gmail.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] PHP deserialization techniques offer rich pickings for security researchers
Hello Team, I came across this article which highlights a few issues with PHP deserialization techniques: https://portswigger.net/daily-swig/phar-out-php-deserialization-techniques-offer-rich-pickings-for-security-researchers
