On Fri, Aug 11, 2017 at 12:55 PM, Nikita Popov <nikita....@gmail.com> wrote:
> On Thu, Aug 10, 2017 at 10:49 AM, Nikita Popov <nikita....@gmail.com> > wrote: > >> On Sun, Aug 6, 2017 at 12:49 AM, Stanislav Malyshev <smalys...@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
