Hi, On Mon, Sep 8, 2025 at 10:44 PM Tim Düsterhus <[email protected]> wrote:
> Hi > > On 9/8/25 17:32, Jakub Zelenka wrote: > > I think the point here was that it was close and the RFC itself was > > Again, a 2/3 majority is *not* close. Twice the number of folks were in > favor than against the RFC. > > I meant close to the actual threshold (not close vote). Again if single person changed their mind because of the facts below (which is quite likely), it might not have passed. This RFC is just about to see if people can see that there is significantly more effort needed for this migration. It just tries to make sure that people can vote on the right description. If they don't think, that's wort it to change the result, then so be it. But I think it's fair to provide an RFC with a correct description so it can be properly decided. > > misleading and omitted some important points that would like change the > > final result. > > I concede that “straight up improvement” might not be perfectly accurate > for `__wakeup()`, but for `__sleep()` it *is* accurate. `__sleep()` is > broken and there's a straight-forward migration to `__serialize()`. > > I appreciate that both of you communicated your concerns with the > proposed deprecation during the discussion, but you basically just said > “I believe the impact is too large”, which is not actionable feedback > for the RFC author and also doesn't help voters make an educated > decision. The point of the discussion phase is figuring out these > details together. > > Yeah as I mentioned it was also my fault not to point this out sooner but it's hard to verify every single thing in detail if there are like 50 deprecations proposed. > >> The serialization mechanism is also a security sensitive part of the > >> language, the fewer moving parts there are, the better. Security is part > >> of the motivation for me. > >> > >> > > Could you be more specific here? We do not consider issues (crashes and > > similar) resulting from unserializing of the serialized string as > security > > issues because it must not come from the untrusted source (see > > https://www.php.net/manual/en/function.unserialize.php ). I don't > remember > > any security issue in serialize / unserialize since this rule was set. > > Even when only dealing with trusted data, unserialize needs to parse > inputs (which is already sufficiently dangerous in C) and it also needs > to deal with partial(ly initialized) object graphs all while executing > arbitrary (userland) code that may or may not interact in weird ways > (e.g. when error handlers get involved due to a bug in a custom > unserialize hook). And of course there can also be bugs with the > serializer that result in “trusted” output that triggers unexpected and > untested code-paths, particularly when references or cycles get > involved. When running with mixed PHP versions during an incremental > upgrade it might also be possible for serialization data to be emitted > by newer PHP versions that may not be anticipated by older PHP versions > and that trigger unexpected code-paths. > > I understand your concern about the complexity but this can apply to many other parts in php-src. As I'm sure you know, this still wouldn't likely to be considered as a security issue. > And then there's also stuff like > https://wiki.php.net/rfc/unserialize_warn_on_trailing_data, which can > happen due to bugs in a userland implementation even when not doing > unserialize($_GET['stuff']). > > This could be closer but we wouldn't like find exploit either. At least this wasn't even raised as a security issue so I guess you were expecting that not to be a security problem. > Saying that “unserialize is not security-relevant because you must only > feed it safe inputs” as an excuse to avoid making unserialize safer is > not helping anyone and is downplaying the risks involved in > unserialization. > > I don't have problem with making unserialize safer as there might be users that use it improperly. But we shouldn't be saying that this is a security sensitive part when we don't consider those sort of issues as security issues because none of those issues will get fixed in our security support only releases. In that sense I don't think we can consider it as a security sensitive part. Kind regards Jakub
