Everything

php

SensitiveParameterValue serialization behavior

php.internals

Tim Düsterhus, WoltLab GmbH

4 years ago

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 well.

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

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Guilliam Xavier

4 years ago

On Thu, Feb 24, 2022 at 3:11 PM Tim Düsterhus, 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 well.
>
> 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,

--
Guilliam Xavier

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Guilliam,

On 2/25/22 13:11, Guilliam Xavier wrote:

> I would prefer option 2 (if possible), to avoid potentially breaking
> existing code.

Sure, that's possible. Otherwise I wouldn't have proposed it :-)

The solution for this is simply an additional private property
$isPoisoned that is set to true when unserializing. If it is true,
->getValue() will throw an exception.

> Calls to ->getValue() will be in new code written specifically for
> SensitiveParameterValue anyway, and can be wrapped into try-catch, I think?
>

Yes, try-catch works. I theoretically could add an additional public
function isPoisoned(): bool as well. The user should likely already know
whether the SensitiveParameterValue came from unserialized data or not,
though.

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P

Dan Ackroyd

4 years ago

On Thu, 24 Feb 2022 at 14:11, Tim Düsterhus, WoltLab GmbH
<duesterhus@woltlab.com> wrote:

>
> 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.

That sounds the best option. Yes, people who serialize/unserialize
stack traces will need to take into account the new behaviour, but
they will need to do that anyway, and it's better for something to
fail early, when the data is serialized rather than it happen later
when someone goes to use that data. Programs continuing when they have
already failed is Bad™.

> Allow unserialization, but poison the unserialized object and
> disallow calling ->getValue() on it.

That sounds like something to be instantly added to the list of PHP Sadness.

> I theoretically could add an additional public function isPoisoned(): bool as well.

To me, that sounds like a workaround needed by a bad design aka a hack.

Guilliam Xavier wrote:

> to avoid potentially breaking existing code.

Technically, all existing code will continue to run, as no-one is
currently using SensitiveParameter and so that code will continue to
run. When people start using that feature, then yes they will need to
make sure that any stack serializer is aware of the new feature. But
that doesn't sound like a huge problem to me.

In general, I think we should only add surprising and awkward apis
when there is a really strong reason for doing so, not because there
might be a problem. If it's left as unserializable for now, people
would have the opportunity for saying why it needs to be relaxed
later, aka "no is temporary, yes is forever" for features.

cheers
Dan
Ack

Guilliam Xavier

4 years ago

Hi again,

FWIW, Dan's and Claude's explanations (thanks!) and arguments made me
change my preference to option 1 (i.e. make SensitiveParameterValue not
serializable, period).

Best regards,

--
Guilliam Xavier

Benjamin Eberlei

4 years ago

> Am 26.02.2022 um 12:49 schrieb Dan Ackroyd <Danack@basereality.com>:
>
> ﻿On Thu, 24 Feb 2022 at 14:11, Tim Düsterhus, WoltLab GmbH
> <duesterhus@woltlab.com> wrote:
>>
>> 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.
>
> That sounds the best option. Yes, people who serialize/unserialize
> stack traces will need to take into account the new behaviour, but
> they will need to do that anyway, and it's better for something to
> fail early, when the data is serialized rather than it happen later
> when someone goes to use that data. Programs continuing when they have
> already failed is Bad™.
>
>> Allow unserialization, but poison the unserialized object and
>> disallow calling ->getValue() on it.
>
> That sounds like something to be instantly added to the list of PHP Sadness.
>
>> I theoretically could add an additional public function isPoisoned(): bool as well.
>
> To me, that sounds like a workaround needed by a bad design aka a hack.
>
> Guilliam Xavier wrote:
>> to avoid potentially breaking existing code.
>
> Technically, all existing code will continue to run, as no-one is
> currently using SensitiveParameter and so that code will continue to
> run. When people start using that feature, then yes they will need to
> make sure that any stack serializer is aware of the new feature. But
> that doesn't sound like a huge problem to me.

That is not technically true from what I understand the SensitiveParameter will be added to core functions in a second step, so this will make them appear in existing code immediately.

It would also require some iteration across the whole stack trace and all its parameters to sanitize. Code that would always look the same and have no extra purpose.

However i cant seem to wrap my head around serialising stack traces with parameters. Does someone have an open source example of this kind of code?

Still I am for the second option, because SensitiveParameter without a value is still enough information in an unserialized state.

Claude Pache

4 years ago

>
> 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.

Hi,

Note that exception handlers that serialise stack traces without taking into account that the operation may fail, are already broken as of today, because common unserialisable objects, such as Closure instances (anonymous functions), may appear in stack traces.

https://3v4l.org/tv1s1 <https://3v4l.org/tv1s1>

https://3v4l.org/PGKnl <https://3v4l.org/PGKnl>

Making SensitiveParameterValue fail on serialisation won’t break those handlers, but make their existing brokenness apparent in more cases (which is a good thing).

—Claude

Tim Düsterhus, WoltLab GmbH

4 years ago

Hi Internals!

On 2/24/22 15:11, Tim Düsterhus, WoltLab GmbH wrote:

> Please find the thread in the GitHub PR at:
>
> https://github.com/php/php-src/pull/7921#discussion_r813743903
>
> […]
>
> 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.

Thank you for voicing your opinion.

I've adjusted the implementation to "Disallow both serialization and
unserialization":

https://github.com/php/php-src/pull/7921#discussion_r815976815

As per Dan's response disallowing serialization is the "safe" choice
with regard to future changes and it neatly avoids the bikeshedding with
regard to the exception class to use [1].

I'll make sure to update the RFC with an errata section later.

[1] https://externals.io/message/117022#117120

Best regards
Tim Düsterhus
Developer WoltLab GmbH

--

WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam

Tel.: +49 331 96784338

duesterhus@woltlab.com
www.woltlab.com

Managing director:
Marcel Werk

AG Potsdam HRB 26795 P