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