Fix GH-22063: stream filter chain UAF on self-removal during callback

[iliaal]

A php_user_filter that calls stream_filter_remove on its own resource from inside filter() frees the php_stream_filter struct while userfilter_filter still dereferences &thisfilter->abstract and while the chain walks in _php_stream_filter_flush, _php_stream_write_filtered, and _php_stream_fill_read_buffer still need current->next. Two new fields on php_stream_filter, in_callback and deferred_dtor, carry the deferral: php_stream_filter_remove(filter, true) unlinks and runs zend_list_delete immediately but defers the pefree.

[arnaud-lb]

This looks right, but I'm wondering if it is possible to fix this in a way that doesn't force all filter iterators to pay attention to removal.

This affects only user filters, and only when removing themselves.

A related issue with user filters is that they were able to close the stream. This has been fixed by flagging the stream in userfilter_filter().

[iliaal]

The iterators' current = current->next advance happens after userfilter_filter returns, so a flag scoped to userfilter_filter alone misses the second UAF site. Same issue with the closing-flush in zif_stream_filter_remove, where the outer flush returns and the caller re-derefs the freed pointer. The iterator boundary has to participate.

Closest alternative: stream-level deferred-free flag + list, each iterator brackets its loop with set/clear/drain. That moves per-iteration logic to per-loop boundaries, I can try that approach if you want?

[arnaud-lb]

Another approach would be to leverage refcounting: php_stream_filter.res is refcounted and should have the same lifetime as the php_stream_filter. It may be possible to free the php_stream_filter and .abstract only in the resource's dtor callback (removing the filter from the list before that should be valid?). userfilter_filter should increase the resource's refcount temporarily.

[iliaal]

Tried the refcount version. Resource dtor on le_stream_filter does the free, userfilter_filter brackets the userland call with GC_ADDREF(thisfilter->res) / zend_list_delete, zif_stream_filter_remove does the same around the closing flush. gh22063.phpt passes under ASAN, upside, doesn't require new struct fields.

The blocker is request shutdown. zend_close_rsrc_list runs through EG(regular_list) in hash order, and zend_resource_dtor ignores refcount: the moment a filter resource's slot is visited, the new dtor frees the filter struct. If the owning stream resource is visited next, its stream_resource_regular_dtor → _php_stream_free → _php_stream_flush → _php_stream_write_filtered walks stream->writefilters.head into freed memory. ASAN flags 17 new heap-use-after-free regressions across ext/standard/tests/filters/*.

Making refcount-only work without that means either the dtor stops being the owner, or filter-resource vs stream-resource teardown order needs coordination, which gives back the simplicity.

My failed attempt is here alt/gh-22063-refcount in iliaal/php-src if you want to look.

[arnaud-lb]

Thank you for trying! I've tried to fetch your branch, but it looks like that you didn't push it :D

I see the issue with shutdown. zend_close_rsrc_list() ensures that resources are destroyed in reverse creation order, which is usually the reverse dependency order, but not in this case. We would need the filters to be destroyed after the stream.

What we could do is to still remove the filter from the list in stream_filter_remove() as before, so that the dtor's only responsibility is to free the filter. Dtor can ignore filters that are still in a filter chain. This would fix the shutdown issue.

However we still get UAFs if the next filter is also removed:

class SelfRemovingFilter extends php_user_filter {
    public $stream;
    public static ?string $key = null;
    public static bool $on_closing_only = false;

    public function filter($in, $out, &$consumed, $closing): int
    {
        while ($bucket = stream_bucket_make_writeable($in)) {
            $consumed += $bucket->datalen;
            stream_bucket_append($out, $bucket);
        }
        if (self::$key !== null && (!self::$on_closing_only || $closing)) {
            $res = $GLOBALS[self::$key];
            self::$key = null;
            stream_filter_remove($res);
            stream_filter_remove($GLOBALS['filter2']);
        }
        return PSFS_PASS_ON;
    }
}

stream_filter_register('self-removing', SelfRemovingFilter::class);

echo "write side: ";
$f = fopen('php://memory', 'r+');
SelfRemovingFilter::$key = 'write_res';
SelfRemovingFilter::$on_closing_only = false;
$GLOBALS['write_res'] = stream_filter_append($f, 'self-removing', STREAM_FILTER_WRITE);
$GLOBALS['filter2'] = stream_filter_append($f, 'string.rot13', STREAM_FILTER_WRITE);
fwrite($f, 'hello');
fwrite($f, ' world');
rewind($f);
echo stream_get_contents($f), "\n";

Maybe we should just deny the removal of filter during filtering.

[iliaal]

It helps to actually push branches, who would've guessed 🤦 https://github.com/iliaal/php-src/tree/alt/gh-22063-refcount

I think the idea of denying the removal of filter during filtering is the right path, I am re-working the PR to do that.

[arnaud-lb]

If we don't want to deny removal of filters during filtering, your approach is probably the best one. To avoid the ->next problem, we could do something like this:

• stream_filter_remove() sets ->deferred_dtor, but doesn't call php_stream_filter_remove(.., 1)
• iterators skip filters with deferred_dtor but follow ->next
• observers of filter->deferred_dtor && filter->in_callback == 0 call php_stream_filter_remove(.., 1)

[arnaud-lb]

This looks good to me for master, but please wait for an other approval before merging.

cc @bukka