Make WordPress Core

Opened 8 months ago

Last modified 2 months ago

#59233 new task (blessed)

Improve error handling for unserialize()

Reported by: jrf's profile jrf Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: General Keywords: php83 2nd-opinion
Focuses: php-compatibility Cc:

Description

From https://core.trac.wordpress.org/ticket/59231:

Make unserialize() emit a warning for trailing bytes

While based on the current test suite, WP is not directly affected by this, the `maybe_unserialize()` function could still be confronted by data with trailing bytes.

However, the call to the PHP native unserialize() within maybe_unserialize() silences all (PHP 8.0+: non-fatal) errors, so this new warning will not affect WP or its ecosystem as long as the maybe_unserialize() function is used.

Having said that, a critical look at maybe_unserialize() may be warranted as the new warning in PHP is related to security issues discovered in other projects, so WP may want to consider rejecting unserialization for data throwing this warning.

Also note that there are 7 uses of unserialize() in total within WP Core, one within maybe_unserialize(), but the function is also used in 6 other places and 5 of those do not use error silencing.

Improve unserialize() error handling

This, again, affects the `maybe_unserialize()` function and this time, the code should probably be adjusted to handle the new errors which unserialize() can now throw.

The change does not affect unserializing valid data, but in the case of invalid data, the type of and severity of the notices/warnings/catchable exceptions have been changed.

All 7 uses of unserialize() in WP Core should be reviewed and for the 6 uses outside of the maybe_unserialize() function, it should be reviewed whether they can/should switch to using maybe_unserialize() and/or whether they should get their own (improved) error handling.

Change History (6)

#1 @jrf
8 months ago

  • Keywords 2nd-opinion added

#2 @hellofromTonya
6 months ago

  • Milestone changed from 6.4 to 6.5

6.4 RC1 is happening a few hours. Moving this to 6.5.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 months ago

#4 @chaion07
2 months ago

Thanks @jrf for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are making the following changes:

  1. Considering the lack of progress with this ticket we are in n favor of updating the milestone to 6.6
  2. However this is a PHP Compatibility ticket (and a Task) which often only get attention late in a cycle, punting too early may delay the ticket getting necessary discussion. That said, Tonya is active and may pull it back into 6.5 if that's appropriate

Cheers!

Props to @costdev & @mukesh27

#5 @swissspidy
2 months ago

  • Milestone changed from 6.5 to 6.6

#6 @azaozz
2 months ago

a critical look at maybe_unserialize() may be warranted as the new warning in PHP is related to security issues discovered in other projects

+1. As a minimum thinking that the warnings from unserialize() should not be silenced when WP is in development mode (see https://developer.wordpress.org/reference/functions/wp_is_development_mode/).

Also thinking it makes sense to use maybe_unserialize() instead of unserialize() in more places/as appropriate as an attempt to maintain backwards compatibility (no warnings) in production in PHP 8.0+.

Note: See TracTickets for help on using tickets.