Fix use-after-free on re-entrant ftp_close() during a transfer

[iliaal]

A user stream wrapper's stream_read/stream_write handler can call ftp_close() mid-transfer (during ftp_get/ftp_put/ftp_append or the ftp_nb_* variants), freeing the ftpbuf_t the engine still holds on the C stack, so the transfer resumes on freed memory (valgrind: invalid reads in the ftp_get receive loop). The fix guards each engine transfer with an in_use flag and makes ftp_close() throw while a transfer is in progress. Reported privately as GHSA-vr2x-x6ff-4586, rejected per the security policy because the trigger is PHP code, not the FTP server.

[ndossche]

Does this cover all possible functions? (e.g. non-blocking ones, they can close the connection too)

[iliaal]

Yes. Every non-blocking transfer funnels through ftp_nb_continue_read/ftp_nb_continue_write, both the initial ftp_nb_get/ftp_nb_put and each ftp_nb_continue step, and both are guarded. Together with the blocking ftp_get/ftp_put/ftp_append, those are the only engine functions that touch the user stream, so a wrapper can't re-enter ftp_close anywhere else.

[ndossche]

The following re-entrancy bug still triggers UAF:

--TEST--
Re-entrant ftp_nb_get() from a stream wrapper during a blocking ftp_get() must not free the active data connection
--EXTENSIONS--
ftp
pcntl
--FILE--
<?php
require 'server.inc';

class NbGetDuringGet {
    public $context;
    public static $ftp;
    public function stream_open($path, $mode, $options, &$opened_path) {
        return true;
    }
    public function stream_write($data) {
        @ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
        return strlen($data);
    }
    public function stream_close() {}
    public function stream_eof() {
        return true;
    }
}

stream_wrapper_register('reentrantget', NbGetDuringGet::class);

$ftp = ftp_connect('127.0.0.1', $port);
var_dump(ftp_login($ftp, 'user', 'pass'));
NbGetDuringGet::$ftp = $ftp;

var_dump(@ftp_get($ftp, 'reentrantget://sink', 'a story.txt', FTP_BINARY));

ftp_close($ftp);
echo "closed\n";
?>
--EXPECT--
bool(true)
bool(true)
closed

[iliaal]

Ah, you're right. The initiating calls reopen the data connection before reaching the guarded continue step, so in_use never fired: ftp_nb_get() and ftp_nb_put() close the active connection, and ftp_genlist() (nlist/rawlist/mlsd) reassigns ftp->data. Moved the guard to the entry of all three, so every engine function that opens a data connection now refuses while a transfer is in progress. Added your reproducer as ftp_nb_get_during_transfer.phpt.

[iliaal]

Pushed a follow-up: the C guard wasn't enough for the non-blocking path. The nb wrappers set ftp->direction and null ftp->stream around the C call, so a rejected re-entrant ftp_nb_get/ftp_nb_put still corrupted an outer nb transfer. Moved the busy check to the userland entry of all four nb functions, before that state mutation, and reordered the finished/failed cleanup to null ftp->stream before php_stream_close so a stream_close() calling ftp_close() can't double-close it. Added ftp_nb_get_during_nb_transfer.phpt for the nb-during-nb case.

[ndossche]

Final question: for ftp_close() you used an exception, but for the other cases you used warnings. Shouldn't all of these be exceptions?

[iliaal]

Deliberate. The transfer guards fire from wrapper callbacks inside an active transfer, so a thrown Error stays pending through the failed stream write and unwinds the outer transfer too unless the wrapper catches it; your nb reproducer would abort the outer get instead of returning true, since @ doesn't suppress an Error. Warning+false keeps the refusal contained to the re-entrant call, matching ext/ftp's other transfer failures. ftp_close() throws because it can't proceed at all while the transfer owns the connection.