Skip to content

Fix GH-9618: drain __wakeup/__unserialize queue on unserialize failure - #21716

Open
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/gh9618-unserialize-wakeup-drain
Open

Fix GH-9618: drain __wakeup/__unserialize queue on unserialize failure#21716
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/gh9618-unserialize-wakeup-drain

Conversation

@iliaal

@iliaal iliaal commented Apr 10, 2026

Copy link
Copy Markdown
Member

Fixes #9618

unserialize() defers __wakeup() and __unserialize() calls until the entire payload has been parsed, then drains them inside PHP_VAR_UNSERIALIZE_DESTROY at the end of php_unserialize_with_options. On the failure path, zval_ptr_dtor(return_value) at ext/standard/var.c:1469 runs first and triggers destructors of the partially-built return value. A destructor that touches a sibling object sees that sibling in its pre-wakeup state, bypassing any invariant installed in __wakeup() or __unserialize().

The POC uses a malformed property key length to abort unserialization after constructing both A (with a destructor) and B (whose __wakeup() overwrites a field that A::__destruct later passes to eval()). Before this fix, A::__destruct runs first, reaches B with its pre-wakeup value, and eval() executes the attacker-controlled string.

Drain the deferred-call queue on the failure path, before the zval_ptr_dtor(return_value) that triggers destructors. __wakeup() and __unserialize() now run first, so sibling destructors see the post-wakeup state.

Implementation:

  • New static var_drain_entry() in var_unserializer.re factors the per-slot drain out of var_destroy(). Both var_destroy() and the new var_invoke_delayed_calls() share one implementation.
  • var_invoke_delayed_calls() walks the var dtor hash and calls var_drain_entry() on each slot, clearing the VAR_WAKEUP_FLAG / VAR_UNSERIALIZE_FLAG markers so a subsequent var_destroy() skips already-drained entries.
  • php_unserialize_with_options calls var_invoke_delayed_calls() on the failure branch, before zval_ptr_dtor(return_value).

Success path is unchanged. Tests cover both the __wakeup and __unserialize drain paths; both fail on unfixed master and pass after the fix.

@Sjord

Sjord commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I think there's a bug when unserialize calls are nested. I vibe-coded the following code to show it:

<?php
/*
 * Reproduces a bug in the GH-9618 fix (commit 6c5407a).
 *
 * The fix calls var_invoke_delayed_calls() on the failure path of
 * php_unserialize_with_options(). When a Serializable::unserialize()
 * method calls unserialize() internally, the nested call shares the
 * same php_unserialize_data_t with the outer call (BG(serialize_lock)==0,
 * BG(unserialize).level>1). If the nested call fails,
 * var_invoke_delayed_calls() drains ALL deferred __wakeup calls in the
 * shared hash — including those belonging to the outer call's objects.
 *
 * Correct output (A::__wakeup runs during var_destroy, after all parsing):
 *   B::unserialize complete
 *   A::__wakeup
 *   bool(true)
 *
 * Buggy output (A::__wakeup drained prematurely by inner failure):
 *   A::__wakeup
 *   B::unserialize complete
 *   bool(true)
 */

class A
{
    public function __wakeup()
    {
        echo "A::__wakeup\n";
    }
}

class B implements Serializable
{
    public $state;

    public function serialize(): string
    {
        return 'x';
    }

    public function unserialize($data): void
    {
        @unserialize('O:1:"X":{BAD');
        echo "B::unserialize complete\n";
        $this->state = 'ready';
    }
}

// Outer payload: array [A, B].
// A is parsed first (its __wakeup is queued in the dtor hash).
// B is parsed second (Serializable -> B::unserialize()).
// B::unserialize triggers a failing nested unserialize, which
// (incorrectly) drains A's __wakeup from the shared dtor hash.
$payload = 'a:2:{i:0;O:1:"A":0:{}i:1;C:1:"B":1:{x}}';

var_dump(@unserialize($payload));

@iliaal
iliaal force-pushed the fix/gh9618-unserialize-wakeup-drain branch from 6c5407a to 67111fe Compare September 3, 2026 12:03
iliaal added a commit to iliaal/php-src that referenced this pull request Sep 3, 2026
…lure

unserialize() defers __wakeup and __unserialize until parsing ends and
drains them in PHP_VAR_UNSERIALIZE_DESTROY, but the failure path runs
zval_ptr_dtor(return_value) first, so a destructor of the partially-built
value can reach a sibling object still in its pre-wakeup state and bypass
whatever invariant that wakeup installs. Drain the queue before that dtor,
scoped to the outermost call so a failing nested unserialize() leaves the
outer call's queue alone.

Fixes phpGH-9618
Closes phpGH-21716
…lure

unserialize() defers __wakeup and __unserialize until parsing ends and
drains them in PHP_VAR_UNSERIALIZE_DESTROY, but the failure path runs
zval_ptr_dtor(return_value) first, so a destructor of the partially-built
value can reach a sibling object still in its pre-wakeup state and bypass
whatever invariant that wakeup installs. Drain before that dtor. The var
dtor hash is shared with nested unserialize() calls and is append-only, so
the drain starts from the tail position recorded before parsing and covers
only the entries this call queued.

Fixes phpGH-9618
Closes phpGH-21716
@iliaal
iliaal force-pushed the fix/gh9618-unserialize-wakeup-drain branch from 67111fe to bd5ef5a Compare September 3, 2026 12:20
@iliaal

iliaal commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

Ah, indeed. Fixed, though not the way I first tried: scoping the drain to BG(unserialize).level <= 1 left the original bypass reachable one level down, since the nested root's destructor is deferred to the owner's var_destroy(), which drains and destructs per entry. The hash is append-only, so it now records the tail before parsing and drains from there. Your reproducer and the nested bypass added to tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unserialize __wakeup bypass

2 participants