Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119755
Message-ID: <a127051f-5ef2-f4be-6ddd-08a6d07dcc17@bastelstu.be>
Date: Tue, 28 Mar 2023 18:03:07 +0200
MIME-Version: 1.0
Content-Language: en-US
To: internals@lists.php.net
References: <f37f233b-6645-d618-0868-78cbadb649a2@bastelstu.be>
 <CABdc3WovbsYH2BTsTbeD71bSfM44C1nocXtoxQpPo=E2Fq4iLQ@mail.gmail.com>
 <1B77BC20-4EA6-453B-A39B-2406A4E53436@dafert.at>
 <e0a3a96c-63f3-4503-bb98-d997af3d316f@app.fastmail.com>
 <8E30718D-80AB-4631-9B5A-47499AF21CB0@cschneid.com>
In-Reply-To: <8E30718D-80AB-4631-9B5A-47499AF21CB0@cschneid.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] RFC [Discussion]: Make unserialize() emit a warning for
 trailing bytes
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

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