unread
Hello Team,
I came across this article which highlights a few issues with PHP
deserialization techniques:
PHP deserialization techniques offer rich pickings for security researchers View as Markdown
unread
Hi!
I came across this article which highlights a few issues with PHP
deserialization techniques:
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
smalyshev@gmail.com
unread
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, smalyshev@gmail.com
wrote:
Hi!
I came across this article which highlights a few issues with PHP
deserialization techniques: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
smalyshev@gmail.com
unread
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
smalyshev@gmail.com
unread
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.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
unread
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:
-
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. -
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
unread
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:
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.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
unread
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
smalyshev@gmail.com
unread
Hi!
- 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
smalyshev@gmail.com
unread
On Wed, Apr 17, 2019 at 12:44 AM Stanislav Malyshev smalyshev@gmail.com
wrote:
Hi!
- 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