Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117137
MIME-Version: 1.0
References: <b9d3740d-7664-44ea-318d-bcb516ad5c87@woltlab.com>
In-Reply-To: <b9d3740d-7664-44ea-318d-bcb516ad5c87@woltlab.com>
Date: Fri, 25 Feb 2022 13:11:50 +0100
Message-ID: <CAC5V6ghjiDN4ZOzJaikVrKoGoA3atLu4zL2zodvEHhZ2tW0ZAA@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus=2C_WoltLab_GmbH?= <duesterhus@woltlab.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001a1e7b05d8d69b87"
Subject: Re: [PHP-DEV] SensitiveParameterValue serialization behavior
From: guilliam.xavier@gmail.com (Guilliam Xavier)

--0000000000001a1e7b05d8d69b87
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 24, 2022 at 3:11 PM Tim D=C3=BCsterhus, WoltLab GmbH <
duesterhus@woltlab.com> wrote:

> Hi Internals!
>
> during code review of the "Redacting parameters in back traces" RFC [1]
> an issue with the proposed serialization behavior of
> SensitiveParameterValue objects became apparent that was not noticed
> before the RFC went into voting:
>
> The RFC proposed that serialization was allowed, but without including
> the inner value in the serialization data:
>
>      public function __serialize(): array { return []; }
>
> As this operation is lossy, it was proposed that unserialization fails
> and this is what was implemented in the PoC patch:
>
>      public function __unserialize(array $data): void {
>          throw new \Exception('...');
>      }
>
> The decision to allow serialization was to allow existing error handlers
> to work without needing to special case SensitiveParameterValue. However
> it is clearly not useful, if unserialization does not work after all.
> Any error during unserialization is not recoverable.
>
> Please find the thread in the GitHub PR at:
>
> https://github.com/php/php-src/pull/7921#discussion_r813743903
>
> As per Ilija Tovilo's suggestion I'm looping in the Internals list as wel=
l.
>
> I see two possible options to remediate this issue:
>
> -------
>
> 1. Disallow both serialization and unserialization.
>
> This will make the serialization issue very obvious, but will require
> adjustments to exception handlers that serialize the stack traces.
>
> 2. Allow unserialization, but poison the unserialized object and
> disallow calling ->getValue() on it.
>
> This would be closer to the original intent of the RFC, but moves the
> issue just somewhere else: The object would not be usable either way.
>
> -------
>
> What would be your preferred option? Feel free to either reply on the
> list or add to the discussion on GitHub.
>
> Thanks!
>
> [1] https://wiki.php.net/rfc/redact_parameters_in_back_traces
>

Hi Tim,

I would prefer option 2 (if possible), to avoid potentially breaking
existing code.
Calls to ->getValue() will be in new code written specifically for
SensitiveParameterValue anyway, and can be wrapped into try-catch, I think?

Best regards,

--=20
Guilliam Xavier

--0000000000001a1e7b05d8d69b87--