Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119751
Content-Type: text/plain;
	charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Date: Tue, 28 Mar 2023 09:27:58 +0200
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>
To: php internals <internals@lists.php.net>
In-Reply-To: <e0a3a96c-63f3-4503-bb98-d997af3d316f@app.fastmail.com>
Message-ID: <8E30718D-80AB-4631-9B5A-47499AF21CB0@cschneid.com>
Subject: Re: [PHP-DEV] RFC [Discussion]: Make unserialize() emit a warning for
 trailing bytes
From: cschneid@cschneid.com (Christian Schneider)

Am 28.03.2023 um 00:22 schrieb Larry Garfield <larry@garfieldtech.com>:
> On Mon, Mar 27, 2023, at 2:12 PM, Mel Dafert wrote:
>> On 27 March 2023 20:20:58 CEST, "Micha=C5=82 Marcin Brzuchalski"=20
>> <michal.brzuchalski@gmail.com> 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.
>>=20
>> If that's the desired direction, it makes more sense to emit a=20
>> deprecation notice now and throw an exception starting in 9.0.
>=20
> 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.

Regards,
- Chris