Unserialize security policy

On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov nikita.ppv@gmail.com
wrote:

On Sun, Aug 6, 2017 at 12:49 AM, Stanislav Malyshev smalyshev@gmail.com
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 security@ 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

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