Schedule deserialization hardening

[barryhughes]

Both of the default action stores persist schedule information as serialized PHP blobs. However, there are currently no protections during the deserialization process to prevent PHP object injection attacks.

• Fixes Insecure Deserialization #1318
• Builds on and potentially replaces Harden schedule deserialization against PHP object injection #1341 (but I've kept that alive in case we want to rework from that foundation)

The core concept here (originally introduced by #1341, thank you @vladolaru) is to handle deserialization in one or more passes. High-level (slightly simplified) summary of the algorithm in this PR:

• First pass: unserialize() with an allowed_classes list covering the library's own schedule classes and the small set of support classes they legitimately nest (the CronExpression family, and the built-in date/time value objects). Normal data hydrates fully in this pass — if the outermost object is a schedule and nothing decoded to __PHP_Incomplete_Class, we return it with no further work.
• If unexpected classes remain: the outermost object must itself be a schedule (a top-level non-schedule — e.g. an injected gadget — is rejected here, before we walk anything). Each remaining __PHP_Incomplete_Class is then vetted: acceptable if it's on the (filterable) support allow-list or implements ActionScheduler_Schedule. If all pass, we unserialize() once more restricted to the expanded set and return the result; if any fails, we return false (which the caller treats as corrupt).

The idea is to minimize any additional overhead when we are dealing with perfectly normal and expected schedule data, while still filtering out potentially dangerous class references.

---

Testing

The test suite should provide reasonable coverage, but we can also perform some manual testing and exploration. For instance, you can try creating conventional scheduled actions of various sorts via WP CLI:

wp eval "as_schedule_cron_action( time(), '* */2 * * *', 'testcronaction' );"

Then, visit Tools ‣ Scheduled Actions. You should see the created actions in the queue of pending actions and, if you run them manually, they should successfully complete or fail as expected.

Tip

When is it acceptable for a test action like testcronaction, created above, to fail? When we haven't registered a callback. You can workaround this by adding something like add_action( 'testcronaction', fn () => 0 ); to a mu-plugin, or just look at the failed action log and verify it says something like, 'Scheduled action for testcronaction will not be executed as no callbacks are registered.' This is fine and expected.

Second, we can simulate a perfectly valid custom schedule that happens to contain an instance of an unknown class:

class Problematic_Custom_Schedule implements ActionScheduler_Schedule {
	public $data;

	public function __construct() {
		$this->data = new Custom_Entity;
	}

	public function next(?DateTime $after = null) {
		return $after === null ? date_create( '+1 day' ) : $after->modify( '+1 day' );
	}

	public function is_recurring() {
		return false;
	}

	public function get_date() {
		return date_create();
	}
}

class Custom_Entity {
	public $data = 123;
}

ActionScheduler::store()->save_action( 
	new ActionScheduler_Action( 'testaction', [], new Problematic_Custom_Schedule )
);

Run the above with wp eval-file /path/to/script.php, then head over to Tools ‣ Scheduled Actions. Depending on how quickly you get there it will probably be sitting in either the past-due queue or else will have moved to cancelled (which is how these particularl cases are handled).

Feel free to explore other permutations 🙂

---

Notes/questions

• The above example of Problematic_Custom_Schedule illustrates how some third party or custom schedules, that until now were considered valid, will be handled. This is potentially problematic (by default, cancelled actions cannot be re-run and will be deleted after one month); so we need to decide if that risk is acceptable.
• Note here that I did not find any cases like this when scanning through a body of known WooCommerce extensions. Of course, that doesn't cover everything out there (still, it's a useful indicator).
• It may even be desirable to introduce a new status like suspended (right now I'm less interested in what we name it and more the concept as a whole) which would then give admins a chance to review and force run the actions. An existing status like failed might also work for this, but we would need to add the ability to forcibly re-run.
• Further, we provide a new action_scheduler_allowed_nested_schedule_classes filter to make it easier for custom schedule implementations to expand the allow-list.
• At site/project level, new hook action_scheduler_enforce_schedule_allowed_classes essentially allows for these new mechanics to be turned off (while still making it possible to observe any problems). Of course, this also requires the ability to add custom code. We again need to ask if that is acceptable by itself, or should be supported with some sort of settings UI.
• Last, it seems fine to ship this as part of a 4.1.0 release alongside other fixes already merged to trunk, but we could also separate it out.

[kalessil]

LGTM overall. Leaving some feedback in the comments below.

[kalessil]

There is potential to use the singleton pattern here - better for performance.

[barryhughes]

Here also replying to this earlier comment, I'm a little reluctant to implement these ideas (but may be overthinking it, so may as well talk it out) because of scenarios like this:

foreach ( $blog_ids as $blog_id ) {
	switch_to_blog( $blog_id );

	if ( ! as_next_scheduled_action( 'custom_job' ) ) {
		warn_job_not_scheduled();
	}
}

Obviously the above snippet is a bit contrived, but I just mean to illustrate that it will (potentially) lead to schedule deserialization taking place multiple times within the same request, but across different and changing contexts (for which the range of acceptable/unacceptable classes may also change).

For example, instances of Plugin_A_Schedule_Data may be allowed on the first iteration, when we are inside Blog 1, but not on the next when we are inside Blog 2 and Plugin A is not even active. And/or, there may be plugin classes that are acceptable within Blog 2 but which were unknown when this first runs, within Blog 1.

This probably isn't very common, and I can think of supporting mechanics to work around it, but am basically feeling a little wary.

[kalessil]

Given your prior work on A-S, I'd trust your gut feeling.

"Just" drop in a comment there clarifying that the new instance is by design to accommodate context switching (as multisite and more) - so future people have this context and don't challenge this decision because of a lack of context.

[barryhughes]

That's a good call. There is an existing comment alluding to this, specifically in the sense of multisite network context changes (in the docblock for the action_scheduler_allowed_nested_schedule_classes hook), but I think it can be promoted to class level and made clearer.

I'd trust your gut feeling.

I'm definitely up for this work being challenged. It's a tricky line we're having to follow between full-on correctness and keeping existing sites working.

[kalessil]

Weighing out complexity vs. maintainability aspects, $deserializer = new self( self::KNOWN_SCHEDULE_CLASSES, $allowed_nested ); is a clear winner.

What I read from this thread is that my proposal is actually low ROI: micro-optimization with potential risk to multi-site (and likely more hidden scenarios). Let's ignore this one.

[kalessil]

Nit: I'd add a second parameter as false:

• Line ~328 — is_schedule_implementation(): class_exists( $class_name ) - Line ~332 — is_schedule_implementation(): class_implements( $class_name )
• Line ~359 — is_allowed_nested_class(): class_exists( $class_name )
• Line ~360 — is_allowed_nested_class(): class_implements( $class_name )

Effectively, it's sealing the auto-loading attack channel once and for all (I know we have a prior array check, but extra hardening, if feasible, will not hurt).

This requires constructor patching to keep the hierarchy investigation working (it'll load the class definition):

$this->allowed_nested_classes = array_filter( $allowed_nested_classes, 'class_exists' );

One thing to investigate, though, is when a custom class is not directly extending A-S classes (adds multiple inheritance layers).

[barryhughes]

Re multiple inheritance layers, I think this is fine:

class Baz implements ActionScheduler_Schedule {
	public function next( ?DateTime $after = null ) {}
	public function is_recurring() {}
}

class Bar extends Baz {}
class Foo extends Bar {}

var_dump( class_implements( Foo::class ) );
# array(1) {
# 	["ActionScheduler_Schedule"]=>
#   string(24) "ActionScheduler_Schedule"
# }

(...but let me know if I've misunderstood.)

[barryhughes]

I'd add a second parameter as false:

The thing is that the current mechanism relies on this being true.

This does indeed mean there is potential for autoloader side-effects (ie, when a file contains both a class definition and other top-level code, then the top-level code will be executed), but that's one of the trade-offs we're reckoning with here.

If false, then we will fail to load legitimate (third party) schedule implementations that rely on an autoloader for discovery.

I'm not closed to the idea, but it increases the risk of breakages (in the sense that up-until-now perfectly valid scheduled actions would risk being cancelled) in production.

[kalessil]

I see, let's keep as it is. We can take this risk if security reports point to this attack vector again after this solution is merged. We'll have some options then if this scenario materializes.

[barryhughes]

✍🏼 This was discussed elsewhere, but it's possibly worth dropping a note in here, too, to make some of the background thinking a little clearer.

An obvious alternative or supplement to this approach would be to adopt JSON-based (de)serialization of scheduling information. The problem with that is that, unless we also continue to write serialized blobs in their current form and current location — and we could, but it raises bigger performance questions that the current approach alone — then a version rollback would potentially render a large number of scheduled actions invalid (because older versions could not read that information).

That's a particular concern for this library, because multiple plugins may ship different (and sometimes quite old) versions of Action Scheduler, so a rollback can happen indirectly simply by a plugin that is no longer required being deactivated.