Skip to content

Commit e0a9afb

Browse files
iliaaliliaal
committed
Fix use-after-free on re-entrant ftp_close() during a transfer
A user stream wrapper passed to ftp_get(), ftp_put(), ftp_append() or the ftp_nb_* variants can re-enter the FTP engine from its stream_read or stream_write handler while the engine still holds the freed ftpbuf_t and databuf_t on the C stack, so the transfer resumes on freed memory. The re-entrant call is ftp_close(), a second transfer, or a directory listing: ftp_nb_get(), ftp_nb_put() and ftp_genlist() reopen the data connection while the outer transfer still uses it. Guard every engine operation that opens a data connection with an in_use flag; ftp_close() throws while it is set and the others refuse to run. Closes GH-22400
1 parent e121cb2 commit e0a9afb

6 files changed

Lines changed: 186 additions & 0 deletions

File tree

‎ext/ftp/ftp.c‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -892,6 +892,11 @@ ftp_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_t pat
892892
if (ftp == NULL) {
893893
return 0;
894894
}
895+
if (ftp->in_use) {
896+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
897+
return 0;
898+
}
899+
ftp->in_use = true;
895900
if (!ftp_type(ftp, type)) {
896901
goto bail;
897902
}
@@ -967,9 +972,11 @@ ftp_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_t pat
967972
goto bail;
968973
}
969974

975+
ftp->in_use = false;
970976
return 1;
971977
bail:
972978
data_close(ftp);
979+
ftp->in_use = false;
973980
return 0;
974981
}
975982
/* }}} */
@@ -1057,6 +1064,11 @@ ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *inst
10571064
if (ftp == NULL) {
10581065
return 0;
10591066
}
1067+
if (ftp->in_use) {
1068+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
1069+
return 0;
1070+
}
1071+
ftp->in_use = true;
10601072
if (!ftp_type(ftp, type)) {
10611073
goto bail;
10621074
}
@@ -1097,9 +1109,11 @@ ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *inst
10971109
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
10981110
goto bail;
10991111
}
1112+
ftp->in_use = false;
11001113
return 1;
11011114
bail:
11021115
data_close(ftp);
1116+
ftp->in_use = false;
11031117
return 0;
11041118
}
11051119
/* }}} */
@@ -1114,6 +1128,11 @@ ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
11141128
if (ftp == NULL) {
11151129
return 0;
11161130
}
1131+
if (ftp->in_use) {
1132+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
1133+
return 0;
1134+
}
1135+
ftp->in_use = true;
11171136
if (!ftp_type(ftp, type)) {
11181137
goto bail;
11191138
}
@@ -1141,9 +1160,11 @@ ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
11411160
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
11421161
goto bail;
11431162
}
1163+
ftp->in_use = false;
11441164
return 1;
11451165
bail:
11461166
data_close(ftp);
1167+
ftp->in_use = false;
11471168
return 0;
11481169
}
11491170
/* }}} */
@@ -2055,6 +2076,10 @@ ftp_genlist(ftpbuf_t *ftp, const char *cmd, const size_t cmd_len, const char *pa
20552076
char **entry;
20562077
char *text;
20572078

2079+
if (ftp->in_use) {
2080+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
2081+
return NULL;
2082+
}
20582083

20592084
if ((tmpstream = php_stream_fopen_tmpfile()) == NULL) {
20602085
php_error_docref(NULL, E_WARNING, "Unable to create temporary file. Check permissions in temporary files directory.");
@@ -2156,6 +2181,11 @@ ftp_nb_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_t
21562181
return PHP_FTP_FAILED;
21572182
}
21582183

2184+
if (ftp->in_use) {
2185+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
2186+
return PHP_FTP_FAILED;
2187+
}
2188+
21592189
if (ftp->data != NULL) {
21602190
/* If there is a transfer in action, abort it.
21612191
* If we don't, we get an invalid state and memory leaks when the new connection gets opened. */
@@ -2223,11 +2253,17 @@ ftp_nb_continue_read(ftpbuf_t *ftp)
22232253

22242254
data = ftp->data;
22252255

2256+
if (ftp->in_use) {
2257+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
2258+
return PHP_FTP_FAILED;
2259+
}
2260+
22262261
/* check if there is already more data */
22272262
if (!data_available(ftp, data->fd, false)) {
22282263
return PHP_FTP_MOREDATA;
22292264
}
22302265

2266+
ftp->in_use = true;
22312267
type = ftp->type;
22322268

22332269
lastch = ftp->lastch;
@@ -2251,6 +2287,7 @@ ftp_nb_continue_read(ftpbuf_t *ftp)
22512287
}
22522288

22532289
ftp->lastch = lastch;
2290+
ftp->in_use = false;
22542291
return PHP_FTP_MOREDATA;
22552292
}
22562293

@@ -2265,9 +2302,11 @@ ftp_nb_continue_read(ftpbuf_t *ftp)
22652302
}
22662303

22672304
ftp->nb = 0;
2305+
ftp->in_use = false;
22682306
return PHP_FTP_FINISHED;
22692307
bail:
22702308
ftp->nb = 0;
2309+
ftp->in_use = false;
22712310
data_close(ftp);
22722311
return PHP_FTP_FAILED;
22732312
}
@@ -2283,6 +2322,10 @@ ftp_nb_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
22832322
if (ftp == NULL) {
22842323
return 0;
22852324
}
2325+
if (ftp->in_use) {
2326+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
2327+
return PHP_FTP_FAILED;
2328+
}
22862329
if (!ftp_type(ftp, type)) {
22872330
goto bail;
22882331
}
@@ -2330,16 +2373,24 @@ ftp_nb_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
23302373
int
23312374
ftp_nb_continue_write(ftpbuf_t *ftp)
23322375
{
2376+
if (ftp->in_use) {
2377+
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
2378+
return PHP_FTP_FAILED;
2379+
}
2380+
23332381
/* check if we can write more data */
23342382
if (!data_writeable(ftp, ftp->data->fd)) {
23352383
return PHP_FTP_MOREDATA;
23362384
}
23372385

2386+
ftp->in_use = true;
2387+
23382388
if (ftp_send_stream_to_data_socket(ftp, ftp->data, ftp->stream, ftp->type, true) != SUCCESS) {
23392389
goto bail;
23402390
}
23412391

23422392
if (!php_stream_eof(ftp->stream)) {
2393+
ftp->in_use = false;
23432394
return PHP_FTP_MOREDATA;
23442395
}
23452396

@@ -2349,10 +2400,12 @@ ftp_nb_continue_write(ftpbuf_t *ftp)
23492400
goto bail;
23502401
}
23512402
ftp->nb = 0;
2403+
ftp->in_use = false;
23522404
return PHP_FTP_FINISHED;
23532405
bail:
23542406
data_close(ftp);
23552407
ftp->nb = 0;
2408+
ftp->in_use = false;
23562409
return PHP_FTP_FAILED;
23572410
}
23582411
/* }}} */

‎ext/ftp/ftp.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ typedef struct ftpbuf
7373
databuf_t *data; /* Data connection for "nonblocking" transfers */
7474
php_stream *stream; /* output stream for "nonblocking" transfers */
7575
bool nb; /* "nonblocking" transfer in progress */
76+
bool in_use; /* engine transfer in progress; blocks re-entrant ftp_close */
7677
char lastch; /* last char of previous call */
7778
bool direction; /* recv = 0 / send = 1 */
7879
bool closestream;/* close or not close stream */

‎ext/ftp/php_ftp.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,6 +1220,10 @@ PHP_FUNCTION(ftp_close)
12201220

12211221
obj = ftp_object_from_zend_object(Z_OBJ_P(z_ftp));
12221222
if (obj->ftp) {
1223+
if (obj->ftp->in_use) {
1224+
zend_throw_error(NULL, "Cannot close FTP\\Connection while a transfer is in progress");
1225+
RETURN_THROWS();
1226+
}
12231227
success = ftp_quit(obj->ftp);
12241228
ftp_close(obj->ftp);
12251229
obj->ftp = NULL;

‎ext/ftp/tests/ftp_close_during_transfer.phpt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
--TEST--
2+
ftp_close() from a stream wrapper during a transfer throws instead of freeing the connection
3+
--EXTENSIONS--
4+
ftp
5+
pcntl
6+
--FILE--
7+
<?php
8+
require 'server.inc';
9+
10+
class CloseDuringWrite {
11+
public $context;
12+
public static $ftp;
13+
public function stream_open($path, $mode, $options, &$opened_path) {
14+
return true;
15+
}
16+
public function stream_write($data) {
17+
ftp_close(self::$ftp);
18+
return strlen($data);
19+
}
20+
public function stream_close() {}
21+
public function stream_eof() {
22+
return true;
23+
}
24+
}
25+
26+
stream_wrapper_register('reentrant', CloseDuringWrite::class);
27+
28+
$ftp = ftp_connect('127.0.0.1', $port);
29+
var_dump(ftp_login($ftp, 'user', 'pass'));
30+
CloseDuringWrite::$ftp = $ftp;
31+
32+
try {
33+
@ftp_get($ftp, 'reentrant://sink', 'a story.txt', FTP_BINARY);
34+
} catch (\Error $e) {
35+
echo $e->getMessage(), "\n";
36+
}
37+
38+
ftp_close($ftp);
39+
echo "closed\n";
40+
?>
41+
--EXPECT--
42+
bool(true)
43+
Cannot close FTP\Connection while a transfer is in progress
44+
closed

‎ext/ftp/tests/ftp_nb_close_during_transfer.phpt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
--TEST--
2+
ftp_close() from a stream wrapper during a non-blocking transfer throws instead of freeing the connection
3+
--EXTENSIONS--
4+
ftp
5+
pcntl
6+
--FILE--
7+
<?php
8+
require 'server.inc';
9+
10+
class CloseDuringNbWrite {
11+
public $context;
12+
public static $ftp;
13+
public function stream_open($path, $mode, $options, &$opened_path) {
14+
return true;
15+
}
16+
public function stream_write($data) {
17+
ftp_close(self::$ftp);
18+
return strlen($data);
19+
}
20+
public function stream_close() {}
21+
public function stream_eof() {
22+
return true;
23+
}
24+
}
25+
26+
stream_wrapper_register('reentrantnb', CloseDuringNbWrite::class);
27+
28+
$ftp = ftp_connect('127.0.0.1', $port);
29+
var_dump(ftp_login($ftp, 'user', 'pass'));
30+
CloseDuringNbWrite::$ftp = $ftp;
31+
32+
try {
33+
@ftp_nb_get($ftp, 'reentrantnb://sink', 'a story.txt', FTP_BINARY);
34+
} catch (\Error $e) {
35+
echo $e->getMessage(), "\n";
36+
}
37+
38+
ftp_close($ftp);
39+
echo "closed\n";
40+
?>
41+
--EXPECT--
42+
bool(true)
43+
Cannot close FTP\Connection while a transfer is in progress
44+
closed

‎ext/ftp/tests/ftp_nb_get_during_transfer.phpt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
--TEST--
2+
Re-entrant ftp_nb_get() from a stream wrapper during a blocking ftp_get() must not free the active data connection
3+
--EXTENSIONS--
4+
ftp
5+
pcntl
6+
--FILE--
7+
<?php
8+
require 'server.inc';
9+
10+
class NbGetDuringGet {
11+
public $context;
12+
public static $ftp;
13+
public function stream_open($path, $mode, $options, &$opened_path) {
14+
return true;
15+
}
16+
public function stream_write($data) {
17+
@ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
18+
return strlen($data);
19+
}
20+
public function stream_close() {}
21+
public function stream_eof() {
22+
return true;
23+
}
24+
}
25+
26+
stream_wrapper_register('reentrantget', NbGetDuringGet::class);
27+
28+
$ftp = ftp_connect('127.0.0.1', $port);
29+
var_dump(ftp_login($ftp, 'user', 'pass'));
30+
NbGetDuringGet::$ftp = $ftp;
31+
32+
var_dump(@ftp_get($ftp, 'reentrantget://sink', 'a story.txt', FTP_BINARY));
33+
34+
ftp_close($ftp);
35+
echo "closed\n";
36+
?>
37+
--EXPECT--
38+
bool(true)
39+
bool(true)
40+
closed

0 commit comments

Comments
 (0)