Fix #72667. opendir() attempted to open datastream for non-existent directory. #2033
Conversation
…en the data stream. included test cases for opendir() with ftp:// and ftps:// wrappers. Test cases re-use ext/ftp/tests/server.inc
| datastream = php_stream_sock_open_host(hoststart, portno, SOCK_STREAM, 0, 0); | ||
| if (datastream == NULL) { | ||
| goto opendir_errexit; | ||
| } | ||
|
|
||
| result = GET_FTP_RESULT(stream); | ||
| if (result != 150 && result != 125) { | ||
| /* Could not retrieve or send the file | ||
| * this data will only be sent to us after connection on the data port was initiated. | ||
| */ |
There was a problem hiding this comment.
This comment makes it sounds like the data connection is opened before checking the result intentionally -- it seems to imply that the result will only be sent after the data connection is initiated.
There was a problem hiding this comment.
I suspect this code was copied from php_stream_url_wrap_ftp() where it might make a bit more sense.
My interpretation that data channel should not be opened is based on section 6 (state diagrams) of the RFC 959. Response with 4xx or 5xx would lead into F-state (failure) without attempting to open data channel. 1xx response (e.g. 125 or 150) should lead to opening the data channel, processing as required, and return to W-state.
This also matches what I see in real life. IIS 7.5 and 8.5 both respond with 550. ProFTPD responds with 450 but may be configured to respond with 226 as well. At this point server will not accept passive data stream.
It is OS and/or firewall dependent whether to RST or silently ignore the attempts to open data channel without server listening for it.
Further, datastream error handling if (datastream == NULL) { will incorrectly leave last good response, usually "200 Type set to A", in tmp_line which will then show as in Warning:
PHP Warning: opendir(): connect() failed: Network is unreachable in test.php on line 2
PHP Warning: opendir(ftp://example.com/asd): failed to open dir: FTP server reports 200 Type set to A.
in test.php on line 2
Handling of the error condition from php_fopen_do_pasv() seems to be suffering from the same problem, but I digress.
There was a problem hiding this comment.
I looked around a bit today... The RFC is, as usual, underspecified. It looks like DJB has some more practical coverage on the topic.
For NLST/LIST it says (emphasis mine):
A LIST or NLST request asks the server to send the contents of a directory over the data connection already established by the client.
Also, the page on PASV says:
The server normally accepts PASV with code 227. Its response is a single line showing the IP address of the server and the TCP port number where the server is accepting connections.
Normally the client will connect to this TCP port, from the same IP address that the client is using for the FTP connection, and then send a RETR request. However, the client may send some other requests first, such as REST. The server must continue to read and respond to requests while it accepts connections. Most operating systems handle this automatically.
Both of these seem to imply that the data connection is supposed to be opened before the RETR/NLST/... request is sent. The example does this as well. I also checked the ext/ftp code and unless I missed something, it also establishes the data connection before sending the request. See in particular this part of ftp_genlist: https://github.com/php/php-src/blob/master/ext/ftp/ftp.c#L1805 (NLST goes through here)
Maybe the fix is to open the data connection earlier (before sending the request) instead of later? Or am I just misunderstanding something?
There was a problem hiding this comment.
Opening the data connection immediately after the PASV works as well.
| /* Could not retrieve or send the file | ||
| * this data will only be sent to us after connection on the data port was initiated. | ||
| */ | ||
| goto opendir_errexit; |
There was a problem hiding this comment.
Was the removal of the datastream closing here intentional?
There was a problem hiding this comment.
It wasn't; thanks for catching it.
|
Merged as cce457c into PHP 5.6 and up. Thanks! |
Check status from control channel before attempting to open the data stream.
included test cases for opendir() with ftp:// and ftps:// wrappers. Test cases
re-use ext/ftp/tests/server.inc