Unserialize security policy

  100147
August 2, 2017 20:02 nikita.ppv@gmail.com (Nikita Popov)
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
  100148
August 2, 2017 21:05 cmbecker69@gmx.de ("Christoph M. Becker")
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
  100149
August 2, 2017 21:24 zeev@zend.com (Zeev Suraski)
> On 2 Aug 2017, at 23:03, Nikita Popov ppv@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. >
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.
  100158
August 5, 2017 22:49 smalyshev@gmail.com (Stanislav Malyshev)
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). -- Stas Malyshev smalyshev@gmail.com
  100159
August 6, 2017 06:56 Remi Collet <remi@fedoraproject.org>
--kcLqjStBTWxxjuAOUFphwLWLWPJDGmqgu
Content-Type: text/plain; charset=utf-8
Content-Language: fr-FR
Content-Transfer-Encoding: quoted-printable

Le 06/08/2017 =C3=A0 00:49, Stanislav Malyshev a =C3=A9crit :
> Hi! >=20 >> https://bugs.php.net/bug.php?id=3D75006 has been marked as a non-secur= ity
>> bug, with the justification that unserialize() should not be fed untru= sted
>> 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.
>=20 > Not always, but sometimes we did. I think we should stop doing it, as t= o
> not validate the idea that unserialize can safely be used with untruste= d
> data=20
+1 --kcLqjStBTWxxjuAOUFphwLWLWPJDGmqgu--
  100171
August 10, 2017 08:49 nikita.ppv@gmail.com (Nikita Popov)
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
  100179
August 11, 2017 10:55 nikita.ppv@gmail.com (Nikita Popov)
On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov 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
  100213
August 15, 2017 09:29 nikita.ppv@gmail.com (Nikita Popov)
On Fri, Aug 11, 2017 at 12:55 PM, Nikita Popov ppv@gmail.com> wrote:

> On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov 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
  100225
August 16, 2017 02:02 smalyshev@gmail.com (Stanislav Malyshev)
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
  100247
August 17, 2017 16:39 ajf@ajf.me (Andrea Faulds)
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/
  100222
August 15, 2017 21:56 cmbecker69@gmx.de ("Christoph M. Becker")
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:
  100223
August 15, 2017 22:32 cmbecker69@gmx.de ("Christoph M. Becker")
On 15.08.2017 at 23:56, Christoph M. Becker wrote:

> What about references? Consider, for instance, the following code: > > > $_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