[iliaal]

A php_user_filter that calls stream_filter_remove() while its chain is being applied frees a filter that the chain walk, or userfilter_filter() after the callback returns, still uses. stream_filter_remove() now warns and returns false in that window. The state lives in two stream flags, one per chain, set around each chain walk and around the removal's own flush, so no struct changes and the fix can target PHP-8.4.

PHP-8.6 added filter seek(), so when merging up, php_stream_filters_seek() needs the same bracket plus gh22063_seek.phpt, both on https://github.com/iliaal/php-src/tree/fix/gh-22063-flags-master.

Fixes GH-22063

[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

[iliaal]

Pushed one more commit. php_stream_filters_seek walked the chain calling filter->fops->seek without raising in_iteration, so a user filter that removes itself from seek() hit the same UAF the filter() denial already closes. Bracketed the seek walk the same way, with a gh22063_seek.phpt covering it.

[bukka]

I think the prohibiting approach makes sense consider that we have a similar no fclose approach for the actual stream.

You should also change PR description.

[bukka]

Do we really need to break ABI here? I mean we could use a similar logic as for stream protection using PHP_STREAM_FLAG_NO_FCLOSE (in this case 2 bits as we have got two chains). Then we could consider 8.4 as a target. It's also a bit more consistent.

[iliaal]

Switched to two stream flags, saved and restored around each chain walk like NO_FCLOSE, and retargeted to PHP-8.4. stream_filter_remove() also holds the flag across its own flush, since a wrapper's stream_write() could otherwise remove the same filter mid-removal.

[bukka]

Also needs resolving NEWS conflict...