Stream error finalize UAF fixes

[bukka]

It fixes stream errors issues reported in GH-23259 , GH-23264 and GH-23262 . Those are mostly edge cases.

[LamentXU123]

As far as I can tell I think this is correct. But I dont know much about stream so I think I can't give useful reviews on this. Perhaps David can help :)

[edorian]

Given I've created the 3 fixed tickets, I did a purely mechanical/LLM review using the model that reported the issues:

All 👍

[edorian]

As briefly discussed privately: Evaluation of the fix unearthed two more issues. Attaching them here as @bukka will address them in the scope of this PR as well:

Model output below, collapsed to now blow up the discussion

Model output

---

php_stream_get_parent_operation() is off by one

Description

php_stream_error_operation_begin() stores an operation at index operation_depth and
then increments, so the operation at nesting level n lives at index n-1.
php_stream_get_parent_operation() returns index operation_depth - 2, but all four of
its callers (php_stream_error_operation_end(), the error_count == 0 fast path in
php_stream_error_operation_end_for_stream(), php_stream_error_operation_abort() and
php_stream_error_state_cleanup()) invoke it after they have already decremented
state->operation_depth. It therefore has to return operation_depth - 1.

The effect is that as soon as any nested stream operation finishes,
state->current_operation points one level too shallow, or is NULL. begin/end then
desynchronise, and an inner php_stream_error_operation_end() can finalise — and free the
error chain of — an operation that an outer php_stream_report_errors() is still walking.

The following code:

<?php
class ErrorStream
{
    public $context;
    public function stream_open($path, $mode, $options, &$openedPath): bool { return true; }
    public function stream_read(int $count): string { return str_repeat('A', $count + 1); }
    public function stream_eof(): bool { return true; }
    public function stream_stat(): array { return []; }
}

class NestingStream
{
    public $context;
    public function stream_open($path, $mode, $options, &$openedPath): bool { return true; }
    public function stream_read(int $count): string
    {
        $inner = fopen('php://memory', 'r');
        fread($inner, 1);
        fclose($inner);
        return 'x';
    }
    public function stream_eof(): bool { return true; }
    public function stream_stat(): array { return []; }
}

stream_wrapper_register('error-stream', ErrorStream::class);
stream_wrapper_register('nesting-stream', NestingStream::class);

$busy = false;
set_error_handler(static function (int $severity, string $message) use (&$busy): bool {
    if ($busy) { return true; }
    $busy = true;
    $s = fopen('nesting-stream://x', 'r');
    fread($s, 1);
    fclose($s);
    $busy = false;
    return true;
});

$stream = fopen('error-stream://x', 'r');
fread($stream, 1);
fclose($stream);
echo "done\n";

Resulted in this output:

==ERROR: AddressSanitizer: heap-use-after-free on address 0x… at pc 0x…
READ of size 8 at 0x… thread T0
    #0 php_stream_report_errors            main/streams/stream_errors.c:412   (entry = entry->next)
    #1 php_stream_error_operation_end      main/streams/stream_errors.c:463
    #2 php_stream_error_operation_end_for_stream stream_errors.c:555
    #3 zif_fread                           ext/standard/file.c:1623

freed by thread T0 here:
    #3 php_stream_error_entry_free         main/streams/stream_errors.c:226
    #4 php_stream_error_operation_end      main/streams/stream_errors.c:466
    #5 php_stream_error_operation_end_for_stream stream_errors.c:555
    #6 zif_fread                           ext/standard/file.c:1623        <- fread() inside the error handler
    #11 zend_error_zstr_at                 Zend/zend.c:1585
    #13 php_verror                         main/main.c:1237
    #15 php_stream_report_errors           main/streams/stream_errors.c:411 <- outer operation, still reporting
    #16 php_stream_error_operation_end     main/streams/stream_errors.c:463
    #18 zif_fread                          ext/standard/file.c:1623

previously allocated by thread T0 here:
    #3 php_stream_error_add                main/streams/stream_errors.c:342
    #5 php_stream_error                    main/streams/stream_errors.c:647
    #6 php_userstreamop_read               main/streams/userspace.c:664

The freed block is a php_stream_error_entry; after the free its next, docref and
message fields are still read, and on the storing paths they are passed to
zend_string_release()/efree().

Fix proposal:

 static inline php_stream_error_operation *php_stream_get_parent_operation(void)
 {
     const php_stream_error_state *state = &FG(stream_error_state);

-    if (state->operation_depth <= 1) {
+    if (state->operation_depth < 1) {
         return NULL;
     }

-    return php_stream_get_operation_at_depth(state->operation_depth - 2);
+    return php_stream_get_operation_at_depth(state->operation_depth - 1);
 }

---

php_stream_error_operation_end() keeps a pointer into overflow_operations across userland

Description

php_stream_error_operation_end() caches op = state->current_operation and keeps using
it after php_stream_report_errors() has run userland code. Operations deeper than
PHP_STREAM_ERROR_OPERATION_POOL_SIZE (8) do not live in the inline operation_pool but in
state->overflow_operations, which php_stream_error_operation_begin() grows with
erealloc(). If the userland handler nests enough further stream operations to cross a
capacity boundary, the array is reallocated, op dangles, and the rest of the function
reads op->first_error and then writes op->first_error/op->last_error/op->error_count
into freed memory.

The reproducer uses StreamErrorMode::Silent with a context error_handler:

<?php
const OUTER_DEPTH   = 9;   /* first operation that lives in overflow_operations */
const HANDLER_DEPTH = 8;   /* nesting performed from inside the error handler */

class DeepStream
{
    public $context;
    public static int $left = 0;
    public function stream_open($path, $mode, $options, &$openedPath): bool { return true; }
    public function stream_read(int $count): string
    {
        if (--self::$left > 0) {
            $f = fopen('deep-stream://x', 'r', false, $GLOBALS['ctx']);
            $s = fread($f, 1);
            fclose($f);
            return $s;
        }
        return str_repeat('A', $count + 1);
    }
    public function stream_eof(): bool { return true; }
    public function stream_stat(): array { return []; }
}

class WideStream
{
    public $context;
    public static int $left = 0;
    public function stream_open($path, $mode, $options, &$openedPath): bool { return true; }
    public function stream_read(int $count): string
    {
        if (--self::$left > 0) {
            $f = fopen('wide-stream://x', 'r');
            $s = fread($f, 1);
            fclose($f);
            return $s;
        }
        return 'x';
    }
    public function stream_eof(): bool { return true; }
    public function stream_stat(): array { return []; }
}

stream_wrapper_register('deep-stream', DeepStream::class);
stream_wrapper_register('wide-stream', WideStream::class);

$busy = false;
$ctx = stream_context_create(['stream' => [
    'error_mode'    => StreamErrorMode::Silent,
    'error_handler' => static function (array $errors) use (&$busy): void {
        if ($busy) { return; }
        $busy = true;
        WideStream::$left = HANDLER_DEPTH;
        $f = fopen('wide-stream://x', 'r');
        fread($f, 1);
        fclose($f);
        $busy = false;
    },
]]);

DeepStream::$left = OUTER_DEPTH;
$stream = fopen('deep-stream://x', 'r', false, $ctx);
fread($stream, 1);
fclose($stream);
echo "done\n";

Resulted in this output:

==ERROR: AddressSanitizer: heap-use-after-free on address 0x… at pc 0x…
READ of size 8 at 0x… thread T0
    #0 php_stream_error_operation_end      main/streams/stream_errors.c:469
    #1 php_stream_error_operation_end_for_stream stream_errors.c:555
    #2 zif_fread                           ext/standard/file.c:1623

0x… is located 0 bytes inside of 192-byte region     <- overflow_operations, capacity 8 * 24 bytes

freed by thread T0 here:
    #0 realloc
    #2 _erealloc                           Zend/zend_alloc.c:2899
    #3 php_stream_error_operation_begin    main/streams/stream_errors.c:317
    #4 zif_fopen                           ext/standard/file.c:750          <- nesting from the error handler

OUTER_DEPTH is the exact boundary: 8 or less is clean (those operations live in the
inline operation_pool, which never moves), 9 crashes.

Fix suggestion:

         php_stream_report_errors(context, op, error_mode, is_terminating);

+        /* report_errors ran userland, which may have erealloc()d
+         * state->overflow_operations and moved this operation. */
+        op = state->current_operation;
+
         if (store_mode == PHP_STREAM_ERROR_STORE_NONE) {

[bukka]

As briefly discussed privately: Evaluation of the fix unearthed two more issues.

Good find.

php_stream_get_parent_operation() is off by one

Fixed in 46fc222

php_stream_error_operation_end() keeps a pointer into overflow_operations across userland

Fixed in d26d1db . This one was quite tricky and required quite a bit of refactoring as that part wasn't thought through. Found couple of related problems with it.

Also found another recursion edge case that is fixed in 20ca2fe

[devnexen]

can a test be added along those lines ?

--TEST--
Stream errors: the error handler is not re-entered by its own stream errors
--FILE--
<?php
$calls = 0;
$stream = null;

$ctx = stream_context_create(['stream' => [
    'error_mode' => StreamErrorMode::Silent,
    'error_handler' => static function (array $errors) use (&$calls, &$stream): void {
        if (++$calls > 5) {
            return;
        }
        fwrite($stream, 'x');
    },
]]);

$stream = fopen(__FILE__, 'r', false, $ctx);
fwrite($stream, 'x');
fclose($stream);
var_dump($calls);
?>
--EXPECT--
int(1)

[devnexen]

this might be appropriate too

if (getenv('SKIP_ASAN')) {
    die('skip ASAN needs different stack limit setting due to more stack space usage');
}

[bukka]

The test is working on ASAN so why?

[devnexen]

true :) was just extra precaution in case we update clang and it suddenly fails.

[bukka]

are you saying there are some changes in clang so it's already failing in some version? I'm fine to skip it if there are some known cases where this can fail ofc

[devnexen]

just saying it can happen, but you know what ;we can always fix this test later after all :)

[bukka]

can a test be added along those lines ?

--TEST--
Stream errors: the error handler is not re-entered by its own stream errors
--FILE--
<?php
$calls = 0;
$stream = null;

$ctx = stream_context_create(['stream' => [
    'error_mode' => StreamErrorMode::Silent,
    'error_handler' => static function (array $errors) use (&$calls, &$stream): void {
        if (++$calls > 5) {
            return;
        }
        fwrite($stream, 'x');
    },
]]);

$stream = fopen(__FILE__, 'r', false, $ctx);
fwrite($stream, 'x');
fclose($stream);
var_dump($calls);
?>
--EXPECT--
int(1)

I don't think we should suppress magically stream setting if it's in error handler. This is really up to user to deal with it and there is recursion limit exactly because of this. Anyone who does this, must know what they are doing because they need to explicitly pass the same stream and then they use it. So if they do this, they will expect recursion to happen so I would prefer not to change this.

In other words I don't want to change the current behaviour to match your example so it's unrelated to this PR (master will recurse as well so it's not something this PR would change).

[devnexen]

alright, just one thing to confirm, since d26d1db the depth limit never kicks in here, end() pops the op before calling the handler so the depth never grows, right ?

[bukka]

Ah yeah, I was slightly imprecise in the previous comment. By recursion limit I meant that stack recursion protection that we have for some time (8.3 IIRC). There is another operation depth limit here which is mainly for user wrapper where the operation is not finished. That commit excluded handler call from it because I think it makes more sense as the operation is sort of done.

[devnexen]

in my side, nothing to add, now I see the pclose test it s all good.