Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Unserialize security policy

Posted by Nikita Popov 
Nikita Popov
[PHP-DEV] Unserialize security policy
August 02, 2017 10:10PM
Hi,

https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
bug, with the justification that unserialize() should not be fed untrusted
input. While we do document that unserialize() shouldn't be used on
untrusted input, we have always treated these as security bugs in the past.

Could somebody please clarify our current security policy with regard to
unserialize?

Thanks,
Nikita
Christoph M. Becker
[PHP-DEV] Re: Unserialize security policy
August 02, 2017 11:10PM
On 02.08.2017 at 22:02, Nikita Popov wrote:

> https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
> bug, with the justification that unserialize() should not be fed untrusted
> input. While we do document that unserialize() shouldn't be used on
> untrusted input, we have always treated these as security bugs in the past.
>
> Could somebody please clarify our current security policy with regard to
> unserialize?

According to the security issue classification[1], it seems to me such
issues are correctly classified as "Not a security issue"[2] by virtue
of the clause:

"requires the use of code or settings known to be insecure"

[1] https://wiki.php.net/security
[2] <https://wiki.php.net/security#not_a_security_issue>;

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Zeev Suraski
Re: [PHP-DEV] Unserialize security policy
August 02, 2017 11:30PM
> On 2 Aug 2017, at 23:03, Nikita Popov <[email protected]> wrote:
>
> Hi,
>
> https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
> bug, with the justification that unserialize() should not be fed untrusted
> input. While we do document that unserialize() shouldn't be used on
> untrusted input, we have always treated these as security bugs in the past.
>

Correct, which was a mistake long overdue for fixing.
Treating unserialoze issues as security creates the false sense that we expect it to be secure, when we absolutely don't. We'll continue fixing these bugs of course, But after discussing it on the security mailing list, we decided to finally stop treating those as security issues. Unserialize is inherently insecure, people should know it and act accordingly.

It may be worth a note in the ChangeLog to make it a bit more prominent.


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Unserialize security policy
August 06, 2017 01:00AM
Hi!

> https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
> bug, with the justification that unserialize() should not be fed untrusted
> input. While we do document that unserialize() shouldn't be used on
> untrusted input, we have always treated these as security bugs in the past.

Not always, but sometimes we did. I think we should stop doing it, as to
not validate the idea that unserialize can safely be used with untrusted
data (it can't, and it doesn't look likely that it ever will be, at
least not without comprehensive rewrite and possibly removing references
support, which is not likely to happen).

If anybody strongly feels that this is wrong, we can make an RFC about
it, but given the current state of unserialize(), I can not say we can
support such usage scenario in the current state of unserialize() code,
and would like to hear arguments to the contrary.

If somebody wants to do something about it, please feel welcome, we have
a number of open unserialize bugs right now (if you want to work on them
and don't have access to private bugs and you believe you should -
please ask on [email protected] list).

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Remi Collet
Re: [PHP-DEV] Unserialize security policy
August 06, 2017 09:00AM
Le 06/08/2017 à 00:49, Stanislav Malyshev a écrit :
> Hi!
>
>> https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
>> bug, with the justification that unserialize() should not be fed untrusted
>> input. While we do document that unserialize() shouldn't be used on
>> untrusted input, we have always treated these as security bugs in the past.
>
> Not always, but sometimes we did. I think we should stop doing it, as to
> not validate the idea that unserialize can safely be used with untrusted
> data

+1
Nikita Popov
Re: [PHP-DEV] Unserialize security policy
August 10, 2017 11:00AM
On Sun, Aug 6, 2017 at 12:49 AM, Stanislav Malyshev <[email protected]>
wrote:

> Hi!
>
> > https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
> > bug, with the justification that unserialize() should not be fed
> untrusted
> > input. While we do document that unserialize() shouldn't be used on
> > untrusted input, we have always treated these as security bugs in the
> past.
>
> Not always, but sometimes we did. I think we should stop doing it, as to
> not validate the idea that unserialize can safely be used with untrusted
> data (it can't, and it doesn't look likely that it ever will be, at
> least not without comprehensive rewrite and possibly removing references
> support, which is not likely to happen).
>
> If anybody strongly feels that this is wrong, we can make an RFC about
> it, but given the current state of unserialize(), I can not say we can
> support such usage scenario in the current state of unserialize() code,
> and would like to hear arguments to the contrary.
>
> If somebody wants to do something about it, please feel welcome, we have
> a number of open unserialize bugs right now (if you want to work on them
> and don't have access to private bugs and you believe you should -
> please ask on [email protected] list).
>

Thanks everyone for the clarification. I agree that this is the right
decision. I think it would be good to update the security policy to
explicitly mention unserialize(), as this is probably our largest source of
security bug reports right now, so there's bound to be questions from
security researchers regarding this.

Nikita
Nikita Popov
Re: [PHP-DEV] Unserialize security policy
August 11, 2017 01:00PM
On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov <[email protected]> wrote:

> On Sun, Aug 6, 2017 at 12:49 AM, Stanislav Malyshev <[email protected]>
> wrote:
>
>> Hi!
>>
>> > https://bugs.php.net/bug.php?id=75006 has been marked as a non-security
>> > bug, with the justification that unserialize() should not be fed
>> untrusted
>> > input. While we do document that unserialize() shouldn't be used on
>> > untrusted input, we have always treated these as security bugs in the
>> past.
>>
>> Not always, but sometimes we did. I think we should stop doing it, as to
>> not validate the idea that unserialize can safely be used with untrusted
>> data (it can't, and it doesn't look likely that it ever will be, at
>> least not without comprehensive rewrite and possibly removing references
>> support, which is not likely to happen).
>>
>> If anybody strongly feels that this is wrong, we can make an RFC about
>> it, but given the current state of unserialize(), I can not say we can
>> support such usage scenario in the current state of unserialize() code,
>> and would like to hear arguments to the contrary.
>>
>> If somebody wants to do something about it, please feel welcome, we have
>> a number of open unserialize bugs right now (if you want to work on them
>> and don't have access to private bugs and you believe you should -
>> please ask on [email protected] list).
>>
>
> Thanks everyone for the clarification. I agree that this is the right
> decision. I think it would be good to update the security policy to
> explicitly mention unserialize(), as this is probably our largest source of
> security bug reports right now, so there's bound to be questions from
> security researchers regarding this.
>
> Nikita
>

I think it might also be useful to make a distinction based on
allowed_classes here. I think there is a reasonable expectation that if
allowed_classes is empty (and as such any object injection vectors are
excluded), unserialize() should be safe. The vast majority of unserialize()
bugs are variants on the theme of __wakeup() and
Serializable::unserialize(). But there are also bugs that apply with
allowed_classes=>[], for example https://bugs.php.net/bug.php?id=75054.

Nikita
Nikita Popov
Re: [PHP-DEV] Unserialize security policy
August 15, 2017 11:40AM
On Fri, Aug 11, 2017 at 12:55 PM, Nikita Popov <[email protected]> wrote:

> On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov <[email protected]>
> wrote:
>
>> On Sun, Aug 6, 2017 at 12:49 AM, Stanislav Malyshev <[email protected]>
>> wrote:
>>
>>> Hi!
>>>
>>> > https://bugs.php.net/bug.php?id=75006 has been marked as a
>>> non-security
>>> > bug, with the justification that unserialize() should not be fed
>>> untrusted
>>> > input. While we do document that unserialize() shouldn't be used on
>>> > untrusted input, we have always treated these as security bugs in the
>>> past.
>>>
>>> Not always, but sometimes we did. I think we should stop doing it, as to
>>> not validate the idea that unserialize can safely be used with untrusted
>>> data (it can't, and it doesn't look likely that it ever will be, at
>>> least not without comprehensive rewrite and possibly removing references
>>> support, which is not likely to happen).
>>>
>>> If anybody strongly feels that this is wrong, we can make an RFC about
>>> it, but given the current state of unserialize(), I can not say we can
>>> support such usage scenario in the current state of unserialize() code,
>>> and would like to hear arguments to the contrary.
>>>
>>> If somebody wants to do something about it, please feel welcome, we have
>>> a number of open unserialize bugs right now (if you want to work on them
>>> and don't have access to private bugs and you believe you should -
>>> please ask on [email protected] list).
>>>
>>
>> Thanks everyone for the clarification. I agree that this is the right
>> decision. I think it would be good to update the security policy to
>> explicitly mention unserialize(), as this is probably our largest source of
>> security bug reports right now, so there's bound to be questions from
>> security researchers regarding this.
>>
>> Nikita
>>
>
> I think it might also be useful to make a distinction based on
> allowed_classes here. I think there is a reasonable expectation that if
> allowed_classes is empty (and as such any object injection vectors are
> excluded), unserialize() should be safe. The vast majority of unserialize()
> bugs are variants on the theme of __wakeup() and
> Serializable::unserialize(). But there are also bugs that apply with
> allowed_classes=>[], for example https://bugs.php.net/bug.php?id=75054.
>
> Nikita
>

Here's some external perspective on unserialize() security by Sean Heelan:
https://sean.heelan.io/2017/08/12/fuzzing-phps-unserialize-function/

The two main points are:
1. While it's true that if you're using unserialize() on untrusted input
you are most likely going to be vulnerable due to object injection, it may
be quite hard for an attacker to exploit this for closed source
applications, because it requires knowledge about specific classes defined
by the application. On the other hand, bugs in unserialize() itself can be
exploited much more reliably, without knowing any specific details of the
application.
2. We should be able to precipitate most unserialize() bugs by regularly
fuzzing it ourselves. The setup for doing so is also provided.

Nikita
Christoph M. Becker
Re: [PHP-DEV] Unserialize security policy
August 16, 2017 12:00AM
On 11.08.2017 at 12:55, Nikita Popov wrote:

> I think it might also be useful to make a distinction based on
> allowed_classes here. I think there is a reasonable expectation that if
> allowed_classes is empty (and as such any object injection vectors are
> excluded), unserialize() should be safe. The vast majority of unserialize()
> bugs are variants on the theme of __wakeup() and
> Serializable::unserialize(). But there are also bugs that apply with
> allowed_classes=>[], for example https://bugs.php.net/bug.php?id=75054.

What about references? Consider, for instance, the following code:

<?php

$_POST['untrusted_input'] = 'a:1:{i:0;a:1:{i:0;R:2;}}';

function flatten($array)
{
if (is_array($array)) {
$result = [];
foreach ($array as $element) {
$result = array_merge($result, flatten($element));
}
return $result;
}
return [$array];
}

$unserializedInput = unserialize($_POST['untrusted_input'], []);
flatten($unserializedInput);

Of course, the `flatten()` function is naive, but it is fine for any
"normal" input. However, this very code has a DOS issue. Do we really
want to say that it is the developers responsibility to check for
infinite recursion for code that uses the result of `unserialize(…, [])`
in this way?

It appears to me that `unserialize()` cannot ever be safe, unless class
instantiation *and* references can be excluded. (Neither of these
"features" are available in JSON or (supposed to be) in WDDX, by the
way.) While the former is possible, the latter is not (yet), so in my
humble opinion we should not try to claim that `unserialize(…, [])` is
safe, at least unless there is a mechanism to disallow unserializing of
references, too.

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Christoph M. Becker
Re: [PHP-DEV] Unserialize security policy
August 16, 2017 12:40AM
On 15.08.2017 at 23:56, Christoph M. Becker wrote:

> What about references? Consider, for instance, the following code:
>
> <?php
>
> $_POST['untrusted_input'] = 'a:1:{i:0;a:1:{i:0;R:2;}}';
>
> function flatten($array)
> {
> if (is_array($array)) {
> $result = [];
> foreach ($array as $element) {
> $result = array_merge($result, flatten($element));
> }
> return $result;
> }
> return [$array];
> }
>
> $unserializedInput = unserialize($_POST['untrusted_input'], []);
> flatten($unserializedInput);
>
> Of course, the `flatten()` function is naive, but it is fine for any
> "normal" input. However, this very code has a DOS issue. Do we really
> want to say that it is the developers responsibility to check for
> infinite recursion for code that uses the result of `unserialize(…, [])`
> in this way?
>
> It appears to me that `unserialize()` cannot ever be safe, unless class
> instantiation *and* references can be excluded. (Neither of these
> "features" are available in JSON or (supposed to be) in WDDX, by the
> way.) While the former is possible, the latter is not (yet), so in my
> humble opinion we should not try to claim that `unserialize(…, [])` is
> safe, at least unless there is a mechanism to disallow unserializing of
> references, too.

My apologies for not having read the documentation! Actually, I meant

unserialize(…, ['allowed_classes' => false])

instead of

unserialize(…, [])

--
Christoph M. Becker


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Stanislav Malyshev
Re: [PHP-DEV] Unserialize security policy
August 16, 2017 04:10AM
Hi!

> The two main points are:
> 1. While it's true that if you're using unserialize() on untrusted input
> you are most likely going to be vulnerable due to object injection, it may
> be quite hard for an attacker to exploit this for closed source

Objects are not the problem (unless it's internal objects, in which case
the extension/code authors should have known better, but frequently
don't). References are huge problem, there are too many scenarios where
references can be made completely broken with bad serializing data.

> 2. We should be able to precipitate most unserialize() bugs by regularly
> fuzzing it ourselves. The setup for doing so is also provided.

That assumes that problems in unserialize() stem from some accidental
errors like off-by-one here and there. I don't think it's the case - I
think that given references support, it may not be possible to make it
robust against every hostile input without completely rewriting the
whole code, and even then I'm not sure it's possible. References can
create links between unrelated data items, which may lead to very subtle
problem with semantics inside objects, etc. which current code is just
not prepared to handle.

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Andrea Faulds
Re: [PHP-DEV] Unserialize security policy
August 17, 2017 06:50PM
Hi,

Stanislav Malyshev wrote:
> Hi!
>
>> The two main points are:
>> 1. While it's true that if you're using unserialize() on untrusted input
>> you are most likely going to be vulnerable due to object injection, it may
>> be quite hard for an attacker to exploit this for closed source
>
> Objects are not the problem (unless it's internal objects, in which case
> the extension/code authors should have known better, but frequently
> don't). References are huge problem, there are too many scenarios where
> references can be made completely broken with bad serializing data.
>
>> 2. We should be able to precipitate most unserialize() bugs by regularly
>> fuzzing it ourselves. The setup for doing so is also provided.
>
> That assumes that problems in unserialize() stem from some accidental
> errors like off-by-one here and there. I don't think it's the case - I
> think that given references support, it may not be possible to make it
> robust against every hostile input without completely rewriting the
> whole code, and even then I'm not sure it's possible. References can
> create links between unrelated data items, which may lead to very subtle
> problem with semantics inside objects, etc. which current code is just
> not prepared to handle.
>

This is roughly how I feel about the matter also.

I have wondered about whether it might be possible to rewrite
unserialize() to be somewhat more resilient to reference issues. For
example, making *every* value be unserialized as an IS_REFERENCE, rather
than retroactively replacing non-reference values with reference values,
could prevent use-after-free issues entirely. But it would also be
slower and potentially expose a lot of issues elsewhere…

--
Andrea Faulds
https://ajf.me/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login