Hi
On 3/28/23 09:27, Christian Schneider wrote:
>>>> Personally, I'd like the unserialize to throw an exception if trailing bytes are detected.
>>>> If not by default then with the use of the option passed to unserialize function.
>>>
>>> If that's the desired direction, it makes more sense to emit a
>>> deprecation notice now and throw an exception starting in 9.0.
>>
>> I would also favor throwing an exception. This is a security vector being closed, and that should be closed *hard*. Warnings tend to show up where they're not useful (dev) and get not noticed where they are (prod). Go all the way to an exception here.
>
> I'm not sure why you say this because our set up is the opposite: On dev the warnings are on screen (and could potentially be missed), on production they generate an alert so they are much harder to miss. That's a set up I would recommend, it has worked well for us in maintaining quite an old code base while migrating it to the current PHP version.
>
>> I'm flexible on if that happens in 8.3 or 9. Maybe warning now, with exception in 9? I don't know if that's better from a BC POV, but it should end up as an exception.
>
> I'm generally in favor of going through a warning phase before switching to an exception but if the people here consider this a real security issue I wouldn't rally against an exception.
I do not consider "unserialize ignores trailing bytes" to be any more
security sensitive than any other case of "bytes are passed to
unserialize that are not originally created by serialize by the same
application".
At least in the former case the behavior is well-defined: Trailing bytes
are ignored and not touched at all. The worst that can happen is that
some data is left in a storage location, because the storage is not
properly truncated when rewriting. For that to be a security issue, the
application would need to use a storage that needs to be truncated (i.e.
a file system, not a database), an attacker needs to obtain the
serialized data and a former version of the data needed to be both
longer and contain sensitive data. Quite a few "ifs" and "whens".
In the latter case the security issue is: If an attacker is able to pass
untrusted data to unserialize, a remote code execution might happen.
That said: I agree that ideally unserialize() would use exceptions for
error reporting. However I strongly disagree that this should happen as
part of this RFC to make "trailing bytes" non-silent.
(1) It would be inconsistent.
unserialize() does not currently emit Exceptions (except when an
unserialization callback throws). In fact until PHP 8.3 it would just
emit a E_NOTICE + return false for completely malformed inputs. This
changed as part of this RFC:
https://wiki.php.net/rfc/improve_unserialize_error_handling
It does not appear appropriate to throw an Exception for this reasonably
well-defined case of "trailing bytes", but not throw an Exception in
case the input data is completely broken.
(2) It would not bring any benefit.
Specifically "no benefit compared to 'return false' + E_WARNING".
This effectively ties into (1) - the primary benefit of using Exceptions
is "improved error handling". Adding an Exception for one case, but not
the others would not make error handling any easier, on the contrary it
just adds to the possible error cases.
We also don't have a good exception to use. The
'UnserializationFailedException' of the RFC linked in (1) was declined
and using a plain 'Exception' would just make it harder to introduce a
dedicated Exception in the future. The reason that the
'UnserializationFailedException' was declined, was that folks expect
'unserialize()' to throw specific types of Exception (even if the
behavior is already not guaranteed).
------------
Referring back to this email from the voting thread of said RFC:
https://externals.io/message/118813#118903. My offer still stands: Some
other contributor is free to pick up and repropose my "unserialize +
Exception" proposal and I would be happy to assist with the technical
implementation. However I'm not going to spend any more time discussing
"Exceptions in unserialize", because this is not going to be good for my
mental health. If the discussion for the new "warning for trailing
bytes" RFC shifts to discussing the use of Exceptions in unserialize,
then I'm going to withdraw the RFC.
------------
That said: I would be receptive to adjust the RFC to "return false +
E_WARNING" if that is what folks want. Possibly as part of a secondary vote.
Personally I do not think that it is necessary, though. The warning is
likely sufficiently visible to make folks aware of an issue. However by
not returning 'false', folks would be able to ignore the specific
warning and then gracefully re-serialize their existing data without
including trailing bytes before enabling the warning. By returning false
we would unnecessarily introduce data loss for them even if the behavior
is reasonably well-specified.
Best regards
Tim Düsterhus
